Re: 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
On Monday, August 19, 2013 4:14:45 pm Mark Johnston wrote: > On Mon, Aug 19, 2013 at 03:42:30PM -0400, John Baldwin wrote: > > On Friday, August 16, 2013 1:41:31 pm Mark Johnston wrote: > > > On Fri, Aug 16, 2013 at 07:13:16PM +0200, Davide Italiano wrote: > > > > [trim old mails] > > > > > > > > > diff --git a/sys/sys/pmckern.h b/sys/sys/pmckern.h > > > > > index e3e18a6..90585de 100644 > > > > > --- a/sys/sys/pmckern.h > > > > > +++ b/sys/sys/pmckern.h > > > > > @@ -51,13 +51,11 @@ > > > > > #definePMC_FN_CSW_IN 2 > > > > > #definePMC_FN_CSW_OUT 3 > > > > > #definePMC_FN_DO_SAMPLES 4 > > > > > -#definePMC_FN_KLD_LOAD 5 > > > > > -#definePMC_FN_KLD_UNLOAD 6 > > > > > -#definePMC_FN_MMAP 7 > > > > > -#definePMC_FN_MUNMAP 8 > > > > > -#definePMC_FN_USER_CALLCHAIN 9 > > > > > -#definePMC_FN_USER_CALLCHAIN_SOFT 10 > > > > > -#definePMC_FN_SOFT_SAMPLING11 > > > > > +#definePMC_FN_MMAP 5 > > > > > +#definePMC_FN_MUNMAP 6 > > > > > +#definePMC_FN_USER_CALLCHAIN 7 > > > > > +#definePMC_FN_USER_CALLCHAIN_SOFT 8 > > > > > +#definePMC_FN_SOFT_SAMPLING9 > > > > > > > > > > > > > I've skimmed over your patch quickly so I could miss something, but I > > > > worry about this change breaking the KBI. > > > > Does this make sense for you? > > > > > > I think you're right. I considered this last night, but it didn't occur > > > to me that external modules might try to invoke these hooks. I'm not > > > sure if such modules exist, but it's better to be safe. I updated the > > > patch here: > > > > > > http://people.freebsd.org/~markj/patches/hwpmc-eh/hwpmc-eh-3.diff > > > > This generally looks correct. I would probably not call it > > KLD_LOCK_ASSERT_READ() as it asserts any lock (not specifically a > > read lock). Normally I would use macros like this: > > > > KLD_WLOCK/WUNLOCK > > KLD_RLOCK/RUNLOCK > > KLD_ASSERT_LOCKED/KLD_ASSERT_WLOCKED > > > > However, the existing macros all were named to not assume read > > locking and then some read locking was added on the side. I'm > > not sure exactly what macro name makes the most sense, and would > > in fact be tempted to just retire all the KLD_* macros for locking > > and use sx(9) directly, but that is a fairly minor point. (And if > > done should be a separate commit). > > Yeah, KLD_LOCK_ASSERT_READ() is awkward, but I couldn't come up with > anything better without renaming the existing macros. I'm fine with > getting rid of them and using the sx_* calls directly; it'll be the same > amount of churn as changing the macro names anyway. > > So how about this: I'll get rid of the macros in one commit, change the > locking rules for linker_file_lookup_set() using my existing patch but > without the KLD lock macros, and then convert the hwpmc(4) hooks to use > the new event handlers (making sure to avoid breaking the KBI as Davide > pointed out). That sounds great to me. Thanks! -- John Baldwin ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: 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
On Mon, Aug 19, 2013 at 03:42:30PM -0400, John Baldwin wrote: > On Friday, August 16, 2013 1:41:31 pm Mark Johnston wrote: > > On Fri, Aug 16, 2013 at 07:13:16PM +0200, Davide Italiano wrote: > > > [trim old mails] > > > > > > > diff --git a/sys/sys/pmckern.h b/sys/sys/pmckern.h > > > > index e3e18a6..90585de 100644 > > > > --- a/sys/sys/pmckern.h > > > > +++ b/sys/sys/pmckern.h > > > > @@ -51,13 +51,11 @@ > > > > #definePMC_FN_CSW_IN 2 > > > > #definePMC_FN_CSW_OUT 3 > > > > #definePMC_FN_DO_SAMPLES 4 > > > > -#definePMC_FN_KLD_LOAD 5 > > > > -#definePMC_FN_KLD_UNLOAD 6 > > > > -#definePMC_FN_MMAP 7 > > > > -#definePMC_FN_MUNMAP 8 > > > > -#definePMC_FN_USER_CALLCHAIN 9 > > > > -#definePMC_FN_USER_CALLCHAIN_SOFT 10 > > > > -#definePMC_FN_SOFT_SAMPLING11 > > > > +#definePMC_FN_MMAP 5 > > > > +#definePMC_FN_MUNMAP 6 > > > > +#definePMC_FN_USER_CALLCHAIN 7 > > > > +#definePMC_FN_USER_CALLCHAIN_SOFT 8 > > > > +#definePMC_FN_SOFT_SAMPLING9 > > > > > > > > > > I've skimmed over your patch quickly so I could miss something, but I > > > worry about this change breaking the KBI. > > > Does this make sense for you? > > > > I think you're right. I considered this last night, but it didn't occur > > to me that external modules might try to invoke these hooks. I'm not > > sure if such modules exist, but it's better to be safe. I updated the > > patch here: > > > > http://people.freebsd.org/~markj/patches/hwpmc-eh/hwpmc-eh-3.diff > > This generally looks correct. I would probably not call it > KLD_LOCK_ASSERT_READ() as it asserts any lock (not specifically a > read lock). Normally I would use macros like this: > > KLD_WLOCK/WUNLOCK > KLD_RLOCK/RUNLOCK > KLD_ASSERT_LOCKED/KLD_ASSERT_WLOCKED > > However, the existing macros all were named to not assume read > locking and then some read locking was added on the side. I'm > not sure exactly what macro name makes the most sense, and would > in fact be tempted to just retire all the KLD_* macros for locking > and use sx(9) directly, but that is a fairly minor point. (And if > done should be a separate commit). Yeah, KLD_LOCK_ASSERT_READ() is awkward, but I couldn't come up with anything better without renaming the existing macros. I'm fine with getting rid of them and using the sx_* calls directly; it'll be the same amount of churn as changing the macro names anyway. So how about this: I'll get rid of the macros in one commit, change the locking rules for linker_file_lookup_set() using my existing patch but without the KLD lock macros, and then convert the hwpmc(4) hooks to use the new event handlers (making sure to avoid breaking the KBI as Davide pointed out). -Mark ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: 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
On Friday, August 16, 2013 1:41:31 pm Mark Johnston wrote: > On Fri, Aug 16, 2013 at 07:13:16PM +0200, Davide Italiano wrote: > > [trim old mails] > > > > > diff --git a/sys/sys/pmckern.h b/sys/sys/pmckern.h > > > index e3e18a6..90585de 100644 > > > --- a/sys/sys/pmckern.h > > > +++ b/sys/sys/pmckern.h > > > @@ -51,13 +51,11 @@ > > > #definePMC_FN_CSW_IN 2 > > > #definePMC_FN_CSW_OUT 3 > > > #definePMC_FN_DO_SAMPLES 4 > > > -#definePMC_FN_KLD_LOAD 5 > > > -#definePMC_FN_KLD_UNLOAD 6 > > > -#definePMC_FN_MMAP 7 > > > -#definePMC_FN_MUNMAP 8 > > > -#definePMC_FN_USER_CALLCHAIN 9 > > > -#definePMC_FN_USER_CALLCHAIN_SOFT 10 > > > -#definePMC_FN_SOFT_SAMPLING11 > > > +#definePMC_FN_MMAP 5 > > > +#definePMC_FN_MUNMAP 6 > > > +#definePMC_FN_USER_CALLCHAIN 7 > > > +#definePMC_FN_USER_CALLCHAIN_SOFT 8 > > > +#definePMC_FN_SOFT_SAMPLING9 > > > > > > > I've skimmed over your patch quickly so I could miss something, but I > > worry about this change breaking the KBI. > > Does this make sense for you? > > I think you're right. I considered this last night, but it didn't occur > to me that external modules might try to invoke these hooks. I'm not > sure if such modules exist, but it's better to be safe. I updated the > patch here: > > http://people.freebsd.org/~markj/patches/hwpmc-eh/hwpmc-eh-3.diff This generally looks correct. I would probably not call it KLD_LOCK_ASSERT_READ() as it asserts any lock (not specifically a read lock). Normally I would use macros like this: KLD_WLOCK/WUNLOCK KLD_RLOCK/RUNLOCK KLD_ASSERT_LOCKED/KLD_ASSERT_WLOCKED However, the existing macros all were named to not assume read locking and then some read locking was added on the side. I'm not sure exactly what macro name makes the most sense, and would in fact be tempted to just retire all the KLD_* macros for locking and use sx(9) directly, but that is a fairly minor point. (And if done should be a separate commit). Thanks for doing this! -- John Baldwin ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: 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
On Fri, Aug 16, 2013 at 07:13:16PM +0200, Davide Italiano wrote: > [trim old mails] > > > diff --git a/sys/sys/pmckern.h b/sys/sys/pmckern.h > > index e3e18a6..90585de 100644 > > --- a/sys/sys/pmckern.h > > +++ b/sys/sys/pmckern.h > > @@ -51,13 +51,11 @@ > > #definePMC_FN_CSW_IN 2 > > #definePMC_FN_CSW_OUT 3 > > #definePMC_FN_DO_SAMPLES 4 > > -#definePMC_FN_KLD_LOAD 5 > > -#definePMC_FN_KLD_UNLOAD 6 > > -#definePMC_FN_MMAP 7 > > -#definePMC_FN_MUNMAP 8 > > -#definePMC_FN_USER_CALLCHAIN 9 > > -#definePMC_FN_USER_CALLCHAIN_SOFT 10 > > -#definePMC_FN_SOFT_SAMPLING11 > > +#definePMC_FN_MMAP 5 > > +#definePMC_FN_MUNMAP 6 > > +#definePMC_FN_USER_CALLCHAIN 7 > > +#definePMC_FN_USER_CALLCHAIN_SOFT 8 > > +#definePMC_FN_SOFT_SAMPLING9 > > > > I've skimmed over your patch quickly so I could miss something, but I > worry about this change breaking the KBI. > Does this make sense for you? I think you're right. I considered this last night, but it didn't occur to me that external modules might try to invoke these hooks. I'm not sure if such modules exist, but it's better to be safe. I updated the patch here: http://people.freebsd.org/~markj/patches/hwpmc-eh/hwpmc-eh-3.diff Thanks! -Mark ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: 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
[trim old mails] > diff --git a/sys/sys/pmckern.h b/sys/sys/pmckern.h > index e3e18a6..90585de 100644 > --- a/sys/sys/pmckern.h > +++ b/sys/sys/pmckern.h > @@ -51,13 +51,11 @@ > #definePMC_FN_CSW_IN 2 > #definePMC_FN_CSW_OUT 3 > #definePMC_FN_DO_SAMPLES 4 > -#definePMC_FN_KLD_LOAD 5 > -#definePMC_FN_KLD_UNLOAD 6 > -#definePMC_FN_MMAP 7 > -#definePMC_FN_MUNMAP 8 > -#definePMC_FN_USER_CALLCHAIN 9 > -#definePMC_FN_USER_CALLCHAIN_SOFT 10 > -#definePMC_FN_SOFT_SAMPLING11 > +#definePMC_FN_MMAP 5 > +#definePMC_FN_MUNMAP 6 > +#definePMC_FN_USER_CALLCHAIN 7 > +#definePMC_FN_USER_CALLCHAIN_SOFT 8 > +#definePMC_FN_SOFT_SAMPLING9 > I've skimmed over your patch quickly so I could miss something, but I worry about this change breaking the KBI. Does this make sense for you? > #definePMC_HR 0 /* Hardware ring buffer */ > #definePMC_SR 1 /* Software ring buffer */ > Thanks, -- Davide "There are no solved problems; there are only problems that are more or less solved" -- Henri Poincare ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: 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
On Wed, Aug 14, 2013 at 08:19:13AM -0400, John Baldwin 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.
Ok, I have a couple of patches which do this. The first diff fixes the
locking issue in linker_file_lookup_set(). It ensures that this function
is always called with the linker lock held; it's only called from
kern_linker.c and sdt.c. The assertion in linker_file_lookup_set()
checks for !cold to avoid adding some unnecessary locking in
linker_preload().
The second diff below renames the kld_unload event to kld_unload_try,
adds a kld_unload eventhandler, and converts hwpmc(4) to use kld_load
and kld_unload. So now the kld_load and kld_unload handlers are invoked
with the shared lock held, and the kld_unload_try handlers are invoked
with the exclusive lock held.
I've done a bunch of testing with module loads/unloads and made sure
that the hwpmc and DTrace handlers are being called correctly. Would you
be able to review the diff when you have a chance? The combined diff is
also here:
http://people.freebsd.org/~markj/patches/hwpmc-eh/hwpmc-eh-2.diff
Thanks!
-Mark
diff 1:
diff --git a/sys/kern/kern_linker.c b/sys/kern/kern_linker.c
index 3e500f3..528666c 100644
--- a/sys/kern/kern_linker.c
+++ b/sys/kern/kern_linker.c
@@ -81,6 +81,10 @@ TUNABLE_INT("debug.kld_debug", &kld_debug);
if (!cold) \
sx_assert(&kld_sx, SX_XLOCKED); \
} while (0)
+#defineKLD_LOCK_READ_ASSERT() do {
\
+ if (!cold) \
+ sx_assert(&kld_sx, SX_LOCKED); \
+} while (0)
/*
* static char *linker_search_path(const char *name, struct mod_depend
@@ -208,6 +212,8 @@ linker_file_sysinit(linker_file_t lf)
KLD_DPF(FILE, ("linker_file_sysinit: calling SYSINITs for %s\n",
lf->filename));
+ KLD_LOCK_ASSERT();
+
if (linker_file_lookup_set(lf, "sysinit_set", &start, &stop, NULL) != 0)
return;
/*
@@ -233,6 +239,7 @@ linker_file_sysinit(linker_file_t lf)
* Traverse the (now) ordered list of system initialization tasks.
* Perform each task, and continue on to the next task.
*/
+ KLD_UNLOCK();
mtx_lock(&Giant);
for (sipp = start; sipp < stop; sipp++) {
if ((*sipp)->subsystem == SI_SUB_DUMMY)
@@ -242,6 +249,7 @@ linker_file_sysinit(linker_file_t lf)
(*((*sipp)->func)) ((*sipp)->udata);
}
mtx_unlock(&Giant);
+ KLD_LOCK();
}
static void
@@ -252,6 +260,8 @@ linker_file_sysuninit(linker_file_t lf)
KLD_DPF(FILE, ("linker_file_sysuninit: calling SYSUNINITs for %s\n",
lf->filename));
+ KLD_LOCK_ASSERT();
+
if (linker_file_lookup_set(lf, "sysuninit_set", &start, &stop,
NULL) != 0)
return;
@@ -279,6 +289,7 @@ linker_file_sysuninit(linker_file_t lf)
* Traverse the (now) ordered list of system initialization tasks.
* Perform each task, and continue on to the next task.
*/
+ KLD_UNLOCK();
mtx_lock(&Giant);
for (sipp = start; sipp < stop; sipp++) {
if ((*sipp)->subsystem == SI_SUB_DUMMY)
@@ -288,6 +299,7 @@ linker_file_sysuninit(linker_file_t lf)
(*((*sipp)->func)) ((*sipp)->udata);
}
mtx_unlock(&Giant);
+ KLD_LOCK();
}
static void
@@ -299,13 +311,17 @@ linker_file_register_sysctls(linker_file_t lf)
("linker_file_register_sysctls: registering SYSCTLs for %s\n",
lf->filename));
+ KLD_LOCK_ASSERT();
+
if (linker_file_lookup_set(lf, "sysctl_set", &start, &stop, NULL) != 0)
return;
+ KLD_UNLOCK();
sysctl_lock();
for (oidp = start; oidp < stop; oidp++)
sysctl_register_oid(*oidp);
sysctl_unlock();
+ KLD_LO
Re: 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
On Wednesday, August 14, 2013 5:06:06 pm Mark Johnston wrote:
> On Wed, Aug 14, 2013 at 3:48 PM, John Baldwin wrote:
>
> > On Wednesday, August 14, 2013 2:14:53 pm Mark Johnston wrote:
> > > On Wed, Aug 14, 2013 at 8:19 AM, John Baldwin 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
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: 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
On Wed, Aug 14, 2013 at 3:48 PM, John Baldwin wrote:
> On Wednesday, August 14, 2013 2:14:53 pm Mark Johnston wrote:
> > On Wed, Aug 14, 2013 at 8:19 AM, John Baldwin 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.
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: 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
On Wednesday, August 14, 2013 2:14:53 pm Mark Johnston wrote:
> On Wed, Aug 14, 2013 at 8:19 AM, John Baldwin 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.
--
John Baldwin
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: 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
On Wed, Aug 14, 2013 at 8:19 AM, John Baldwin 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.
-Mark
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: 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
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.
--
John Baldwin
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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
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
Modified:
head/share/man/man9/EVENTHANDLER.9
head/sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
head/sys/cddl/dev/dtrace/dtrace_load.c
head/sys/cddl/dev/dtrace/dtrace_unload.c
head/sys/cddl/dev/sdt/sdt.c
head/sys/kern/kern_linker.c
head/sys/sys/eventhandler.h
Modified: head/share/man/man9/EVENTHANDLER.9
==
--- head/share/man/man9/EVENTHANDLER.9 Tue Aug 13 22:41:24 2013
(r254308)
+++ head/share/man/man9/EVENTHANDLER.9 Wed Aug 14 00:42:21 2013
(r254309)
@@ -199,10 +199,10 @@ Callbacks invoked when a new network int
Callbacks invoked when a network interface is taken down.
.It Vt bpf_track
Callbacks invoked when a BPF listener attaches to/detaches from network
interface.
-.It Vt mod_load
-Callbacks invoked after a module has been loaded.
-.It Vt mod_unload
-Callbacks invoked before a module is about to be unloaded.
+.It Vt kld_load
+Callbacks invoked after a linker file has been loaded.
+.It Vt kld_unload
+Callbacks invoked before a linker file is about to be unloaded.
These callbacks may be used to return an error and prevent the unload from
proceeding.
.It Vt power_profile_change
Modified: head/sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
==
--- head/sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.cTue Aug
13 22:41:24 2013(r254308)
+++ head/sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.cWed Aug
14 00:42:21 2013(r254309)
@@ -241,8 +241,8 @@ int dtrace_in_probe;/* non-zero if exe
#if defined(__i386__) || defined(__amd64__) || defined(__mips__) ||
defined(__powerpc__)
uintptr_t dtrace_in_probe_addr; /* Address of invop when already in
probe */
#endif
-static eventhandler_tagdtrace_modload_tag;
-static eventhandler_tagdtrace_modunload_tag;
+static eventhandler_tagdtrace_kld_load_tag;
+static eventhandler_tagdtrace_kld_unload_tag;
#endif
/*
@@ -15344,14 +15344,14 @@ dtrace_module_unloaded(modctl_t *ctl, in
#if !defined(sun)
static void
-dtrace_mod_load(void *arg __unused, linker_file_t lf)
+dtrace_kld_load(void *arg __unused, linker_file_t lf)
{
dtrace_module_loaded(lf);
}
static void
-dtrace_mod_unload(void *arg __unused, linker_file_t lf, int *error)
+dtrace_kld_unload(void *arg __unused, linker_file_t lf, int *error)
{
if (*error != 0)
Modified: head/sys/cddl/dev/dtrace/dtrace_load.c
==
--- head/sys/cddl/dev/dtrace/dtrace_load.c Tue Aug 13 22:41:24 2013
(r254308)
+++ head/sys/cddl/dev/dtrace/dtrace_load.c Wed Aug 14 00:42:21 2013
(r254309)
@@ -56,11 +56,11 @@ dtrace_load(void *dummy)
/* Hang our hook for exceptions. */
dtrace_invop_init();
- /* Register callbacks for module load and unload events. */
- dtrace_modload_tag = EVENTHANDLER_REGISTER(mod_load,
- dtrace_mod_load, NULL, EVENTHANDLER_PRI_ANY);
- dtrace_modunload_tag = EVENTHANDLER_REGISTER(mod_unload,
- dtrace_mod_unload, NULL, EVENTHANDLER_PRI_ANY);
+ /* Register callbacks for linker file load and unload events. */
+ dtrace_kld_load_tag = EVENTHANDLER_REGISTER(kld_load,
+ dtrace_kld_load, NULL, EVENTHANDLER_PRI_ANY);
+ dtrace_kld_unload_tag = EVENTHANDLER_REGISTER(kld_unload,
+ dtrace_kld_unload, NULL, EVENTHANDLER_PRI_ANY);
/*
* Initialise the mutexes without 'witness' because the dtrace
Modified: head/sys/cddl/dev/dtrace/dtrace_unload.c
==
--- head/sys/cddl/dev/dtrace/dtrace_unload.cTue Aug 13 22:41:24 2013
(r254308)
+++ head/sys/cddl/dev/dtrace/dtrace_unload.cWed Aug 14 00:42:21 2013
(r254309)
@@ -67,8 +67,8 @@ dtrace_unload()
}
dtrace_provider = NULL;
- EVENTHANDLER_DEREGISTER(mod_load, dtrace_modload_tag);
- EVENTHANDLER_DEREGISTER(mod_unload, dtrace_modunload_tag);
+ EVENTHANDLER_DEREGISTER(kld_load, dtrace_kld_load_tag);
+ EVENTHANDLER_DEREGISTER(kld_unload, dtrace_kld_unload_tag);
if ((state = dtrace_anon_grab()) != NULL) {
/*
Modified: head/sys/cddl/dev/sdt/sdt.c
==
--- head/sys/cddl/dev/sdt/sdt.c Tue Aug 13 22:41:24 2013(r254308)
+++ head/sys/cddl/dev/sdt/sdt.c Wed Aug 14 00:42:21 2013(r254309)
@@ -58,8 +58,8 @@ static void sdt_l
