Date: Wed, 5 Dec 2012 23:14:49 +0000 From: Attilio Rao <attilio@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: src-committers@freebsd.org, John Baldwin <jhb@freebsd.org>, Jeff Roberson <jeff@freebsd.org>, Florian Smeets <flo@freebsd.org>, Bruce Evans <bde@freebsd.org>, svn-src-projects@freebsd.org Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern Message-ID: <CAJ-FndCnj9hKV9f%2BRmF-OCijtRpKUWv88AktvrsBYp5_hFUmVA@mail.gmail.com> In-Reply-To: <CAJ-FndCZKpHaRcRMBT5XTRh0nRDjK3f-aVT3rjRzKpMRLU%2BDzA@mail.gmail.com> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com> <CAJ-FndDnO7wjnWPV0tTu%2BUGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com> <201207301732.33474.jhb@freebsd.org> <CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com> <CAJ-FndCRg0UCThFkatp=tw7rUWWCvhsApLE=iztLpxpGBC1F9w@mail.gmail.com> <CAJ-FndBqV2uD8Th9ePtxyJwhMAPzY3AXA5cQ7HszLp=%2BfSpHTA@mail.gmail.com> <CAJ-FndDPLmkpAJeGVN2wgbhdgHYezyUV-PPvH9e-CA7Go7HG3A@mail.gmail.com> <20121029155136.O943@besplex.bde.org> <CAJ-FndDpB2vQ7nBgYS4mNHb3_G4sXi3SfU7Y7kdVHMQZYKapig@mail.gmail.com> <20121109034942.C5338@besplex.bde.org> <CAJ-FndCZKpHaRcRMBT5XTRh0nRDjK3f-aVT3rjRzKpMRLU%2BDzA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Nov 24, 2012 at 3:43 PM, Attilio Rao <attilio@freebsd.org> wrote: > On Thu, Nov 8, 2012 at 5:26 PM, Bruce Evans <brde@optusnet.com.au> wrote: >> On Fri, 2 Nov 2012, Attilio Rao wrote: >> >>> On 10/29/12, Bruce Evans <brde@optusnet.com.au> wrote: >>>> >>>> On Mon, 29 Oct 2012, Attilio Rao wrote: >>>> >>>>> Now that sched_pin()/sched_unpin() are fixed I would like to introduce >>>>> this new patch, making critical_enter()/critical_exit() inline: >>>>> http://www.freebsd.org/~attilio/inline_critical.patch >>>> >>>> ... >>>> >>>> My version goes the other way and uninlines mtx_lock_spin() and >>>> mtx_unlock_spin(). Then it inlines (open codes) critical_enter() and >>>> ... >>> >>> >>> So, I thought more about this and I think that inlining >>> critical_exit() is not really going to bring any benefit here but >>> bloat. >>> This is because nested critical sections are rare rather not, which >> >> >> Rather rare !not? :-) >> >> >>> means you will end up in doing the function call most of the time and >>> plus we have the possible pessimization introduced by the memory >>> clobber (__compiler_membar()) and as you note possible deficiency >>> caming from the branch misprediction*. >> >> >> This seems best. >> >> I see a point about the rareness of the branch in critical_exit(). >> Not sure if it is the one you are making: since the nested case is >> rare, then the branch will normally be correctly predicted. If the >> function remains uninlined, then the branch still has a good chance >> of being correctly predicted. This depends on the nested case being >> so rare across all callers, that the non-nested case doesn't mess >> up the prediction by happening often. The branch predictor only >> has to maintain history for 1 branch for this. However, if the >> call is inlined and there are many callers, there might be too many >> to maintain history for them all. > > Yes, that's basically the same conclusion I came up with. > > It seems you are not opposed to this version of the patch. > I made some in-kernel benchmarks but they didn't really show a > performance improvements, bigger than 1-2%, which on SMP system is > basically accountable to thirdy-part effects. > > However we already employ the same code for sched_pin() and I then > think that we can just commit this patch as-is. I made up my mind on instead not committing this patch, as I cannot prove a real performance gain, as also Jeff agreed with privately. Instead, I would like to commit this small comment explaining why it is not inlined (see below). Let me know what you think. Thanks, Attilio Index: sys/kern/kern_switch.c =================================================================== --- sys/kern/kern_switch.c (revision 243911) +++ sys/kern/kern_switch.c (working copy) @@ -176,6 +176,11 @@ retry: /* * Kernel thread preemption implementation. Critical sections mark * regions of code in which preemptions are not allowed. + * It would seem a good idea to inline such function but, in order + * to prevent instructions reordering by the compiler, a __compiler_membar() + * must be used here (look at sched_pin() case). The performance penalty + * imposed by the membar could, then, produce slower code than + * the function call itself, for most cases. */ void critical_enter(void)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndCnj9hKV9f%2BRmF-OCijtRpKUWv88AktvrsBYp5_hFUmVA>