Date: Tue, 20 Aug 2013 08:14:26 -0400 From: John Baldwin <jhb@freebsd.org> To: Mark Johnston <markj@freebsd.org> Cc: Davide Italiano <davide@freebsd.org>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: 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 Message-ID: <201308200814.26924.jhb@freebsd.org> In-Reply-To: <20130819201445.GB4765@charmander.sandvine.com> References: <201308140042.r7E0gMtf054550@svn.freebsd.org> <201308191542.30797.jhb@freebsd.org> <20130819201445.GB4765@charmander.sandvine.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201308200814.26924.jhb>