From owner-svn-src-projects@FreeBSD.ORG Wed Dec 5 23:14:52 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 05B177AB; Wed, 5 Dec 2012 23:14:52 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-vc0-f182.google.com (mail-vc0-f182.google.com [209.85.220.182]) by mx1.freebsd.org (Postfix) with ESMTP id 250988FC14; Wed, 5 Dec 2012 23:14:50 +0000 (UTC) Received: by mail-vc0-f182.google.com with SMTP id fo14so6582414vcb.13 for ; Wed, 05 Dec 2012 15:14:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:reply-to:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=AJn89QzHKcJeZEqvGCY2T8U16VBkNmEo0Mw6lJ/EW7E=; b=LJLgvA0fFsq9pIqrvVPSzVJU1GfQkPnigx5AnGWjWiX6bGorNujqXBN8axXS/RwQqO E32a7sfSzQcAgeDXOQmvmS4JtzrBcvVaTYqslIxEUjZKgWhLGKV5fbQH5W5n7f8cfepo N4sZlA+rV1guFSRbhQJsMJ+tp/w7KxecgWkgje/R8oIAz3m/+POaFp8DUiOcCW+xOsUJ 9FHV0zbDgeXumIkFopC8Z/rqqu1oe04kgbL7pLYYEeT64Su+L0adYarN33m1Q4wMfquV O63i3iqHB+w24uCOtAkoteJobQ7CqEuCn0L2/Q0BYUUnq7gOb09h+21MPFWUR7gueEqN KX2g== MIME-Version: 1.0 Received: by 10.52.70.46 with SMTP id j14mr14366074vdu.99.1354749290289; Wed, 05 Dec 2012 15:14:50 -0800 (PST) Sender: asmrookie@gmail.com Received: by 10.220.6.7 with HTTP; Wed, 5 Dec 2012 15:14:49 -0800 (PST) In-Reply-To: References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> <20121029155136.O943@besplex.bde.org> <20121109034942.C5338@besplex.bde.org> Date: Wed, 5 Dec 2012 23:14:49 +0000 X-Google-Sender-Auth: xZkIJ50vQj9R_ycf2YHN-S0_iP0 Message-ID: Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern From: Attilio Rao To: Bruce Evans Content-Type: text/plain; charset=UTF-8 Cc: src-committers@freebsd.org, John Baldwin , Jeff Roberson , Florian Smeets , Bruce Evans , svn-src-projects@freebsd.org X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: attilio@FreeBSD.org List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Dec 2012 23:14:52 -0000 On Sat, Nov 24, 2012 at 3:43 PM, Attilio Rao wrote: > On Thu, Nov 8, 2012 at 5:26 PM, Bruce Evans wrote: >> On Fri, 2 Nov 2012, Attilio Rao wrote: >> >>> On 10/29/12, Bruce Evans 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)