From owner-svn-src-projects@FreeBSD.ORG Sun Sep 16 00:22:34 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D6E94106564A; Sun, 16 Sep 2012 00:22:34 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 96F008FC15; Sun, 16 Sep 2012 00:22:34 +0000 (UTC) Received: from John-Baldwins-MacBook-Air.local (c-68-39-198-164.hsd1.de.comcast.net [68.39.198.164]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id CBC1BB949; Sat, 15 Sep 2012 20:22:33 -0400 (EDT) Message-ID: <50551BCA.4020303@FreeBSD.org> Date: Sat, 15 Sep 2012 20:22:34 -0400 From: John Baldwin User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20120824 Thunderbird/15.0 MIME-Version: 1.0 To: attilio@FreeBSD.org References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209130910.50876.jhb@freebsd.org> <201209131132.21103.jhb@freebsd.org> <505514D5.90800@FreeBSD.org> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Sat, 15 Sep 2012 20:22:34 -0400 (EDT) Cc: Davide Italiano , mlaier@freebsd.org, svn-src-projects@freebsd.org, Konstantin Belousov , src-committers@freebsd.org, Stephan Uphoff Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 16 Sep 2012 00:22:34 -0000 On 9/15/12 8:12 PM, Attilio Rao wrote: > On Sun, Sep 16, 2012 at 12:52 AM, John Baldwin wrote: >> On 9/14/12 6:32 PM, Attilio Rao wrote: >>> On Thu, Sep 13, 2012 at 5:20 PM, Attilio Rao wrote: >>>> On 9/13/12, John Baldwin wrote: >>>>> On Thursday, September 13, 2012 10:38:54 am Attilio Rao wrote: >>>>>> On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin wrote: >>>>>>> On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote: >>>>>>>> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin wrote: >>>>>>>>> On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote: >>>>>>>>>> On 7/30/12, John Baldwin 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. > > Did you read the git commit log? _rmlock.h brings along a lot of other > dependencies so it will result anyway in (a different type) of > namespace pollution. It brings in a few structs, yes. However, I don't think we have considered that level of pollution harmful. That is why we have a _rmlock.h separate from rmlock.h. -- John Baldwin