Date: Thu, 13 Sep 2012 11:32:20 -0400 From: John Baldwin <jhb@freebsd.org> To: attilio@freebsd.org Cc: Davide Italiano <davide@freebsd.org>, mlaier@freebsd.org, svn-src-projects@freebsd.org, Konstantin Belousov <kostikbel@gmail.com>, src-committers@freebsd.org, Stephan Uphoff <ups@freebsd.org> Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern Message-ID: <201209131132.21103.jhb@freebsd.org> In-Reply-To: <CAJ-FndASH1=i4ozwP=YepF58iC_5%2Bnf4L4MCu3%2B2-xB9FVzyvg@mail.gmail.com> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209130910.50876.jhb@freebsd.org> <CAJ-FndASH1=i4ozwP=YepF58iC_5%2Bnf4L4MCu3%2B2-xB9FVzyvg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday, September 13, 2012 10:38:54 am Attilio Rao wrote: > On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin <jhb@freebsd.org> wrote: > > On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote: > >> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin <jhb@freebsd.org> wrote: > >> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote: > >> >> On 7/30/12, John Baldwin <jhb@freebsd.org> wrote: > >> >> > --- //depot/projects/smpng/sys/kern/kern_rmlock.c 2012-03-25 > >> >> > 18:45:29.000000000 0000 > >> >> > +++ //depot/user/jhb/lock/kern/kern_rmlock.c 2012-06-18 21:20:58.000000000 > >> >> > 0000 > >> >> > @@ -70,6 +70,9 @@ > >> >> > } > >> >> > > >> >> > static void assert_rm(const struct lock_object *lock, int what); > >> >> > +#ifdef DDB > >> >> > +static void db_show_rm(const struct lock_object *lock); > >> >> > +#endif > >> >> > static void lock_rm(struct lock_object *lock, int how); > >> >> > #ifdef KDTRACE_HOOKS > >> >> > static int owner_rm(const struct lock_object *lock, struct thread > >> >> > **owner); > >> >> > >> >> While here, did you consider also: > >> >> - Abstracting compiler_memory_barrier() into a MI, compiler dependent function? > >> >> - Fix rm_queue with DCPU possibly > >> > > >> > Mostly I just wanted to fill in missing functionality and fixup the > >> > RM_SLEEPABLE bits a bit. > >> > >> So what do you think about the following patch? If you agree I will > >> send to pho@ for testing in a batch with other patches. > > > > It's not super clear to me that having it be static vs dynamic is all that > > big of a deal. However, your approach in general is better, and it certainly > > should have been using PCPU_GET() for the curcpu case all along rather than > > inlining pcpu_find(). > > You mean what is the performance difference between static vs dynamic? > Or you mean, why we want such patch at all? > In the former question there is a further indirection (pc_dynamic > access), for the latter question the patched code avoids namespace > pollution at all and makes the code more readable. More why we want it. I think most of your readability fixes would work just as well if it remained static and we used PCPU_GET(). However, I think your changes are fine. FYI, much of subr_rmlock.c goes out of its way to optimize for performance (such as inlining critical_enter(), critical_exit(), and pcpu_find()), so adding the new indirection goes against the grain of that. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201209131132.21103.jhb>