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 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).

Thanks for doing this!

-- 
John Baldwin