From owner-svn-src-head@FreeBSD.ORG Mon Jun 8 18:30:09 2015 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 1B5B8FF4; Mon, 8 Jun 2015 18:30:09 +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 E57FF1BDD; Mon, 8 Jun 2015 18:30:08 +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 D6F06B997; Mon, 8 Jun 2015 14:30:07 -0400 (EDT) From: John Baldwin To: Ruslan Bukin Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r284153 - head/sys/kern Date: Mon, 08 Jun 2015 14:04:45 -0400 Message-ID: <1554833.IUNnl2bGYK@ralph.baldwin.cx> User-Agent: KMail/4.14.3 (FreeBSD/10.1-STABLE; KDE/4.14.3; amd64; ; ) In-Reply-To: <20150608144629.GA37834@bsdpad.com> References: <201506081406.t58E6mvA033492@svn.freebsd.org> <20150608144629.GA37834@bsdpad.com> 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); Mon, 08 Jun 2015 14:30:07 -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: Mon, 08 Jun 2015 18:30:09 -0000 On Monday, June 08, 2015 03:46:29 PM Ruslan Bukin wrote: > For some reason it hangs for me after 'random' lines on arm64 Are you using dtrace? It looks like sdt was using the public symbol before but in a context where the caller held the lock. I will revert this for now. I think I can perhaps make it 'automatic' by having it acquire a read lock (possibly recursing) if it doesn't already hold a write lock. > FreeBSD clang version 3.6.1 (tags/RELEASE_361/final 237755) 20150525 > CPU: ARM Cortex-A57 r1p0 > IMPLEMENT ME: dtrace_toxic_ranges > random: entropy device infrastructure driver > random: selecting highest priority adaptor > > On Mon, Jun 08, 2015 at 02:06:48PM +0000, John Baldwin wrote: > > Author: jhb > > Date: Mon Jun 8 14:06:47 2015 > > New Revision: 284153 > > URL: https://svnweb.freebsd.org/changeset/base/284153 > > > > Log: > > Add an internal "locked" variant of linker_file_lookup_set() and change > > the public function to acquire the global linker lock directly. This > > permits linker_file_lookup_set() to be safely used from other modules. > > > > Modified: > > head/sys/kern/kern_linker.c > > > > Modified: head/sys/kern/kern_linker.c > > ============================================================================== > > --- head/sys/kern/kern_linker.c Mon Jun 8 13:23:56 2015 (r284152) > > +++ head/sys/kern/kern_linker.c Mon Jun 8 14:06:47 2015 (r284153) > > @@ -137,6 +137,8 @@ static int linker_file_add_dependency(li > > linker_file_t dep); > > static caddr_t linker_file_lookup_symbol_internal(linker_file_t file, > > const char* name, int deps); > > +static int linker_file_lookup_set_locked(linker_file_t file, > > + const char *name, void *firstp, void *lastp, int *countp); > > static int linker_load_module(const char *kldname, > > const char *modname, struct linker_file *parent, > > const struct mod_depend *verinfo, struct linker_file **lfpp); > > @@ -189,7 +191,8 @@ linker_file_sysinit(linker_file_t lf) > > > > sx_assert(&kld_sx, SA_XLOCKED); > > > > - if (linker_file_lookup_set(lf, "sysinit_set", &start, &stop, NULL) != 0) > > + if (linker_file_lookup_set_locked(lf, "sysinit_set", &start, &stop, > > + NULL) != 0) > > return; > > /* > > * Perform a bubble sort of the system initialization objects by > > @@ -237,7 +240,7 @@ linker_file_sysuninit(linker_file_t lf) > > > > sx_assert(&kld_sx, SA_XLOCKED); > > > > - if (linker_file_lookup_set(lf, "sysuninit_set", &start, &stop, > > + if (linker_file_lookup_set_locked(lf, "sysuninit_set", &start, &stop, > > NULL) != 0) > > return; > > > > @@ -288,7 +291,8 @@ linker_file_register_sysctls(linker_file > > > > sx_assert(&kld_sx, SA_XLOCKED); > > > > - if (linker_file_lookup_set(lf, "sysctl_set", &start, &stop, NULL) != 0) > > + if (linker_file_lookup_set_locked(lf, "sysctl_set", &start, &stop, > > + NULL) != 0) > > return; > > > > sx_xunlock(&kld_sx); > > @@ -309,7 +313,8 @@ linker_file_unregister_sysctls(linker_fi > > > > sx_assert(&kld_sx, SA_XLOCKED); > > > > - if (linker_file_lookup_set(lf, "sysctl_set", &start, &stop, NULL) != 0) > > + if (linker_file_lookup_set_locked(lf, "sysctl_set", &start, &stop, > > + NULL) != 0) > > return; > > > > sx_xunlock(&kld_sx); > > @@ -332,7 +337,7 @@ linker_file_register_modules(linker_file > > > > sx_assert(&kld_sx, SA_XLOCKED); > > > > - if (linker_file_lookup_set(lf, "modmetadata_set", &start, > > + if (linker_file_lookup_set_locked(lf, "modmetadata_set", &start, > > &stop, NULL) != 0) { > > /* > > * This fallback should be unnecessary, but if we get booted > > @@ -742,8 +747,8 @@ linker_file_add_dependency(linker_file_t > > * This function is used in this file so we can avoid having lots of (void **) > > * casts. > > */ > > -int > > -linker_file_lookup_set(linker_file_t file, const char *name, > > +static int > > +linker_file_lookup_set_locked(linker_file_t file, const char *name, > > void *firstp, void *lastp, int *countp) > > { > > > > @@ -751,6 +756,19 @@ linker_file_lookup_set(linker_file_t fil > > return (LINKER_LOOKUP_SET(file, name, firstp, lastp, countp)); > > } > > > > +int > > +linker_file_lookup_set(linker_file_t file, const char *name, > > + void *firstp, void *lastp, int *countp) > > +{ > > + int error; > > + > > + sx_slock(&kld_sx); > > + error = linker_file_lookup_set_locked(file, name, firstp, lastp, > > + countp); > > + sx_sunlock(&kld_sx); > > + return (error); > > +} > > + > > /* > > * List all functions in a file. > > */ > > @@ -1469,8 +1487,8 @@ linker_preload(void *arg) > > /* > > * First get a list of stuff in the kernel. > > */ > > - if (linker_file_lookup_set(linker_kernel_file, MDT_SETNAME, &start, > > - &stop, NULL) == 0) > > + if (linker_file_lookup_set_locked(linker_kernel_file, MDT_SETNAME, > > + &start, &stop, NULL) == 0) > > linker_addmodules(linker_kernel_file, start, stop, 1); > > > > /* > > @@ -1479,7 +1497,7 @@ linker_preload(void *arg) > > */ > > restart: > > TAILQ_FOREACH(lf, &loaded_files, loaded) { > > - error = linker_file_lookup_set(lf, MDT_SETNAME, &start, > > + error = linker_file_lookup_set_locked(lf, MDT_SETNAME, &start, > > &stop, NULL); > > /* > > * First, look to see if we would successfully link with this > > @@ -1573,7 +1591,7 @@ restart: > > panic("cannot add dependency"); > > } > > lf->userrefs++; /* so we can (try to) kldunload it */ > > - error = linker_file_lookup_set(lf, MDT_SETNAME, &start, > > + error = linker_file_lookup_set_locked(lf, MDT_SETNAME, &start, > > &stop, NULL); > > if (!error) { > > for (mdp = start; mdp < stop; mdp++) { > > @@ -1610,7 +1628,7 @@ restart: > > goto fail; > > } > > linker_file_register_modules(lf); > > - if (linker_file_lookup_set(lf, "sysinit_set", &si_start, > > + if (linker_file_lookup_set_locked(lf, "sysinit_set", &si_start, > > &si_stop, NULL) == 0) > > sysinit_add(si_start, si_stop); > > linker_file_register_sysctls(lf); > > @@ -2042,7 +2060,7 @@ linker_load_dependencies(linker_file_t l > > if (error) > > return (error); > > } > > - if (linker_file_lookup_set(lf, MDT_SETNAME, &start, &stop, > > + if (linker_file_lookup_set_locked(lf, MDT_SETNAME, &start, &stop, > > &count) != 0) > > return (0); > > for (mdp = start; mdp < stop; mdp++) { > > > -- John Baldwin