Date: Thu, 13 Sep 2012 17:20:54 +0100 From: Attilio Rao <attilio@freebsd.org> To: John Baldwin <jhb@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: <CAJ-FndByCLNpGoFFELQVmC61YdBFn4USunVHB1c7=ZHFoZ9V2g@mail.gmail.com> In-Reply-To: <201209131132.21103.jhb@freebsd.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209130910.50876.jhb@freebsd.org> <CAJ-FndASH1=i4ozwP=YepF58iC_5%2Bnf4L4MCu3%2B2-xB9FVzyvg@mail.gmail.com> <201209131132.21103.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 9/13/12, John Baldwin <jhb@freebsd.org> wrote: > 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. Well, the namespace pollution cannot be avoided without using the dynamic approach, and that is the important part of the patch. > 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. That one of the reasons why I'm asking for advices here actually. I would like to understand if we prefer a cleaner approach or avoid one further indirection with a super-optimized path. Attilio -- Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndByCLNpGoFFELQVmC61YdBFn4USunVHB1c7=ZHFoZ9V2g>