From owner-svn-src-head@FreeBSD.ORG Fri Jun 5 17:45:30 2015 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 519C4548; Fri, 5 Jun 2015 17:45:30 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 2B5591FED; Fri, 5 Jun 2015 17:45:30 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (pool-173-54-116-245.nwrknj.fios.verizon.net [173.54.116.245]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 755A7B915; Fri, 5 Jun 2015 13:45:28 -0400 (EDT) From: John Baldwin To: Sean Bruno Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r284029 - head/sys/kern Date: Fri, 05 Jun 2015 12:55:08 -0400 Message-ID: <3940850.1R9GTC4sC6@ralph.baldwin.cx> User-Agent: KMail/4.14.3 (FreeBSD/10.1-STABLE; KDE/4.14.3; amd64; ; ) In-Reply-To: <201506051621.t55GLiKp095005@svn.freebsd.org> References: <201506051621.t55GLiKp095005@svn.freebsd.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Fri, 05 Jun 2015 13:45:28 -0400 (EDT) X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Jun 2015 17:45:30 -0000 On Friday, June 05, 2015 04:21:44 PM Sean Bruno wrote: > Author: sbruno > Date: Fri Jun 5 16:21:43 2015 > New Revision: 284029 > URL: https://svnweb.freebsd.org/changeset/base/284029 > > Log: > This change uses a reader count instead of holding the mutex for the > interpreter list to avoid the problem of holding a non-sleep lock during > a page fault as reported by witness. In addition, it consistently uses > memset()/memcpy() instead of bzero()/bcopy() except in the case where > bcopy() is required (i.e. overlapping copy). > > Differential Revision: https://reviews.freebsd.org/D2123 > Submitted by: sson > MFC after: 2 weeks > Relnotes: Yes > > Modified: > head/sys/kern/imgact_binmisc.c > > Modified: head/sys/kern/imgact_binmisc.c > ============================================================================== > --- head/sys/kern/imgact_binmisc.c Fri Jun 5 16:02:24 2015 (r284028) > +++ head/sys/kern/imgact_binmisc.c Fri Jun 5 16:21:43 2015 (r284029) > @@ -1,5 +1,5 @@ > /*- > - * Copyright (c) 2013, Stacey D. Son > + * Copyright (c) 2013-15, Stacey D. Son > * All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > @@ -41,6 +41,9 @@ __FBSDID("$FreeBSD$"); > #include > #include > #include > +#include > + > +#include > > /** > * Miscellaneous binary interpreter image activator. > @@ -95,11 +98,68 @@ static SLIST_HEAD(, imgact_binmisc_entry > SLIST_HEAD_INITIALIZER(interpreter_list); > > static int interp_list_entry_count = 0; > - > +static int interp_list_rcount = 0; This should probably be volatile? > static struct mtx interp_list_mtx; > > int imgact_binmisc_exec(struct image_params *imgp); > > +static inline void > +interp_list_rlock() > +{ > + > + /* Don't use atomic_add() here. We must block if the mutex is held. */ > + mtx_lock(&interp_list_mtx); > + interp_list_rcount++; > + mtx_unlock(&interp_list_mtx); It's not really safe to not use an atomic here but use one in runlock. What happens if the compiler uses separate load - add - store here and another thread does the atomic subtract after the load but before the store? > +} > + > +static inline void > +interp_list_runlock() > +{ > + > + /* Decrement the reader count and wake up one writer, if any. */ > + atomic_subtract_int(&interp_list_rcount, 1); > + if (interp_list_rcount == 0) > + wakeup_one(&interp_list_rcount); Reading the variable after subtract is also racey. You can use atomic_fetchadd() instead (which is what refcount_release does). -- John Baldwin