svn commit: r254309 - in head: share/man/man9 sys/cddl/contrib/opensolaris/uts/common/dtrace sys/cddl/dev/dtrace sys/cddl/dev/sdt sys/kern sys/sys

[John Baldwin]

On Wednesday, August 14, 2013 5:06:06 pm Mark Johnston wrote:
> On Wed, Aug 14, 2013 at 3:48 PM, John Baldwin <jhb at freebsd.org> wrote:
> 
> > On Wednesday, August 14, 2013 2:14:53 pm Mark Johnston wrote:
> > > On Wed, Aug 14, 2013 at 8:19 AM, John Baldwin <jhb at freebsd.org> wrote:
> > >
> > > > On Tuesday, August 13, 2013 8:42:22 pm Mark Johnston wrote:
> > > > > Author: markj
> > > > > Date: Wed Aug 14 00:42:21 2013
> > > > > New Revision: 254309
> > > > > URL: http://svnweb.freebsd.org/changeset/base/254309
> > > > >
> > > > > Log:
> > > > >   Use kld_{load,unload} instead of mod_{load,unload} for the linker
> > file
> > > > load
> > > > >   and unload event handlers added in r254266.
> > > > >
> > > > >   Reported by:        jhb
> > > > >   X-MFC with: r254266
> > > >
> > > > Thanks!  BTW, it would be really nice to replace HWPMC_HOOKS in
> > > > kern_linker.c with
> > > > EVENTHANDLER calls.  I think kld_load would just work (though you might
> > > > need to
> > > > downgrade the lock before you run it).  For kld_unload it seems you
> > want
> > > > two events,
> > > > a kld_unload_try for your newly added event (since it can reject a
> > > > kld_unload), and
> > > > perhaps kld_unload at the end where the current HWPMC_HOOK is.  Just an
> > > > idea if
> > > > someone is looking for something to do.  I know there are other modules
> > > > that need
> > > > to hook into linker events like this, and making HWPMC_HOOKS more
> > generic
> > > > would be
> > > > a big help.
> > > >
> > >
> > > I will look into doing this. The DTrace SDT kld_load handler will not
> > work
> > > properly if the
> > > linker lock is downgraded first because of the following code in
> > > linker_file_lookup_set():
> > >
> > > locked = KLD_LOCK();
> > > if (!locked)
> > >         KLD_LOCK();
> > >
> > > In particular, it checks to see if the kld lock is exclusively held and
> > > locks it if not, which
> > > obviously causes problems if the thread holds the shared lock.
> > >
> > > The answer might be to just run the hwpmc handler with the exclusive lock
> > > held. Or perhaps
> > > we just need a linker_file_lookup_set_locked(), assuming that it's ok to
> > > look up a linker set
> > > with the shared lock held.
> >
> > It is probably fine to do a lookup with a shared lock.  We could also fix
> > the
> > linker code to only lock if there is not an existing shared or exclusive
> > lock.
> >
> 
> Sorry if I'm being dense, but I thought it wasn't generally possible to
> determine
> whether curthread holds a given shared lock.

Fair enough.  We could probably either add _locked variant that asserts
either one (you can do an assert for a shared lock), or just require all
callers to get the lock and make the non _locked variant basically be
the _locked variant.  I would prefer the latter if it is not too invasive.

-- 
John Baldwin