Date: Fri, 05 Jun 2015 12:55:08 -0400 From: John Baldwin <jhb@freebsd.org> To: Sean Bruno <sbruno@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r284029 - head/sys/kern Message-ID: <3940850.1R9GTC4sC6@ralph.baldwin.cx> In-Reply-To: <201506051621.t55GLiKp095005@svn.freebsd.org> References: <201506051621.t55GLiKp095005@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <sys/malloc.h> > #include <sys/mutex.h> > #include <sys/sysctl.h> > +#include <sys/sx.h> > + > +#include <machine/atomic.h> > > /** > * 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3940850.1R9GTC4sC6>