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 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 @@
> > > > >  #define        PMC_FN_CSW_IN                   2
> > > > >  #define        PMC_FN_CSW_OUT                  3
> > > > >  #define        PMC_FN_DO_SAMPLES               4
> > > > > -#define        PMC_FN_KLD_LOAD                 5
> > > > > -#define        PMC_FN_KLD_UNLOAD               6
> > > > > -#define        PMC_FN_MMAP                     7
> > > > > -#define        PMC_FN_MUNMAP                   8
> > > > > -#define        PMC_FN_USER_CALLCHAIN           9
> > > > > -#define        PMC_FN_USER_CALLCHAIN_SOFT      10
> > > > > -#define        PMC_FN_SOFT_SAMPLING            11
> > > > > +#define        PMC_FN_MMAP                     5
> > > > > +#define        PMC_FN_MUNMAP                   6
> > > > > +#define        PMC_FN_USER_CALLCHAIN           7
> > > > > +#define        PMC_FN_USER_CALLCHAIN_SOFT      8
> > > > > +#define        PMC_FN_SOFT_SAMPLING            9
> > > > >
> > > > 
> > > > 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