From owner-svn-src-projects@FreeBSD.ORG Sun Sep 16 00:12:22 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 7A3AD106566C; Sun, 16 Sep 2012 00:12:22 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by mx1.freebsd.org (Postfix) with ESMTP id F00408FC08; Sun, 16 Sep 2012 00:12:20 +0000 (UTC) Received: by lbbgg13 with SMTP id gg13so4270554lbb.13 for ; Sat, 15 Sep 2012 17:12:19 -0700 (PDT) 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=R5qTMj6oMtP+ynIZrVL23BGJNqCp6SCa0UuruvIJ0a4=; b=UmsLxPTeIuZhTT4v89LeB6ZjNZ+CsTcVJTzGSPmXO2QL46Qj1+ZPxN0tToSWihliw7 RCVPlCIG95sxQbyDC3dw8lcrKAk9TvVnym0eHrZQLoJ0oT1AoH0bQdIQkd9E/TrWmtIc RK1pQb9Jv5Fu1KxMpQb82/n79/ce+uffQu8yX0WhpuC2cWYu28fEQYS7m5A1rGyNmcP2 YSS2oBuNuba21WYrEqog/Zf2ambo9zcX79DtcMWuoTeW3pqJwMemUe+UAItLqrAbjao+ jUl+kVGruQ+3cqej68v7ADv/4pDTb+zjJ5Fjc9FYnkQSDBZo3mkKtAkNLjVrUI+L5CPw y2jw== MIME-Version: 1.0 Received: by 10.152.131.68 with SMTP id ok4mr6280736lab.47.1347754339760; Sat, 15 Sep 2012 17:12:19 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Sat, 15 Sep 2012 17:12:19 -0700 (PDT) In-Reply-To: <505514D5.90800@FreeBSD.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209130910.50876.jhb@freebsd.org> <201209131132.21103.jhb@freebsd.org> <505514D5.90800@FreeBSD.org> Date: Sun, 16 Sep 2012 01:12:19 +0100 X-Google-Sender-Auth: cvN7fZW8slH6BfDInflu3G0SmrY Message-ID: From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 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 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: Sun, 16 Sep 2012 00:12:22 -0000 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. Attilio -- Peace can only be achieved by understanding - A. Einstein