From owner-svn-src-projects@FreeBSD.ORG Thu Sep 13 16:20:58 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 09EEF106564A; Thu, 13 Sep 2012 16:20:58 +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 7C6D28FC0C; Thu, 13 Sep 2012 16:20:56 +0000 (UTC) Received: by lbbgg13 with SMTP id gg13so2549592lbb.13 for ; Thu, 13 Sep 2012 09:20:55 -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=W5d7rOjlV+fsas4SGbQgRnviWEJAOh657PvcJYzDpcI=; b=Sd0S0iBqL3Tou4kDfBqdJQ1tAqAz+ziSEo7qlUqV6++fWDtyFlrr3CJeVmIfuLfwvN Jz3A4giL/ILi8NzcX7BSZWVR3wtIKSzu5ZpY7Eheyc1CL+DpxSfIPs7w187LF764JEXV UJrqSWPcP+UIP2b++quDe6Li5TyKhN2bmx0M3rT7+otKSZfLDCDJVjvCTPNUPdw1wBXh uGdJB/UXzfj9zYlQV/xmsGec5OQ62xb7QhGU1p01ui2XJFuYd2QHSTluAsxMve4VVI2K hyRmWmbQn3MFNeOj2EYYHWod1PJRFTJkMQJro0MWkENHBP2HOcosltlBs0RgndBSp+d6 S/tA== MIME-Version: 1.0 Received: by 10.112.17.163 with SMTP id p3mr71378lbd.83.1347553255247; Thu, 13 Sep 2012 09:20:55 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Thu, 13 Sep 2012 09:20:54 -0700 (PDT) In-Reply-To: <201209131132.21103.jhb@freebsd.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209130910.50876.jhb@freebsd.org> <201209131132.21103.jhb@freebsd.org> Date: Thu, 13 Sep 2012 17:20:54 +0100 X-Google-Sender-Auth: ZcL04eqF14HRlR4cERA2elyKGU0 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: Thu, 13 Sep 2012 16:20:58 -0000 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. That one of the reasons why I'm asking for advices here actually. I would like to understand if we prefer a cleaner approach or avoid one further indirection with a super-optimized path. Attilio -- Peace can only be achieved by understanding - A. Einstein