Date: Sat, 15 Sep 2012 19:52:53 -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: <505514D5.90800@FreeBSD.org> In-Reply-To: <CAJ-FndBvs1F%2BbXfvL-U2yTi313mebuZ6KidtDqh_CfchxX7dAg@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> <201209131132.21103.jhb@freebsd.org> <CAJ-FndByCLNpGoFFELQVmC61YdBFn4USunVHB1c7=ZHFoZ9V2g@mail.gmail.com> <CAJ-FndBvs1F%2BbXfvL-U2yTi313mebuZ6KidtDqh_CfchxX7dAg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 9/14/12 6:32 PM, Attilio Rao wrote: > On Thu, Sep 13, 2012 at 5:20 PM, Attilio Rao <attilio@freebsd.org> wrote: >> 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. >> > > I've thought about it and I think that avoiding the indirection is > sensitive in that codepath. I've then came up with this patch which > should avoid namespace pollution and the indirection. > > What do you think about it? Why not just move rm_queue to _rmlock.h and make pcpu.h include that? Barring that, make a _rmlock_queue.h and have both headers include that. However, I think that having _rmlock.h in pcpu.h is fine. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?505514D5.90800>