From owner-svn-src-projects@FreeBSD.ORG Sun Sep 16 01:01:16 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 7553F1065672; Sun, 16 Sep 2012 01:01:16 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lpp01m010-f54.google.com (mail-lpp01m010-f54.google.com [209.85.215.54]) by mx1.freebsd.org (Postfix) with ESMTP id DBF828FC14; Sun, 16 Sep 2012 01:01:14 +0000 (UTC) Received: by lage12 with SMTP id e12so4226777lag.13 for ; Sat, 15 Sep 2012 18:01:13 -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=Lec9PxwDTSCYpPEHvQ4/2uKSb2XQPN77uMDHBtHZqaE=; b=qSyEO8ZoiOQNhAcvttgcMlfCp+F7jAB+f2MWkG7zaFLyuRezJmdLb2mnDMIoDFUCnw Z0XSCRmzcZkGe8mWy58WBa5SgcDijk9msTG89v/8nSQmGj0O3pNk0psnL1cbUCvqWeYv BEB1JsxayUFdk5wd6/EpF1CZROP86qR3GLk6hOsFmDf1jhC/C4WkNSKW3vQ7EcqQ3OXb wk/zCtHVZE3F85FhYEeO/43ejE31CX3GxEzdUj2ZfHEhbCo/Xv2rLaZtZjevssm7etdZ dmLIaD1if3ANOPsVNW+2VcJ5VMHzDEeH/4i8Dw9WPXL4nRGibtU18xfAdDd5fH/Z4oyU JqJQ== MIME-Version: 1.0 Received: by 10.112.83.97 with SMTP id p1mr2616404lby.94.1347757273519; Sat, 15 Sep 2012 18:01:13 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Sat, 15 Sep 2012 18:01:13 -0700 (PDT) In-Reply-To: <50551BCA.4020303@FreeBSD.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209130910.50876.jhb@freebsd.org> <201209131132.21103.jhb@freebsd.org> <505514D5.90800@FreeBSD.org> <50551BCA.4020303@FreeBSD.org> Date: Sun, 16 Sep 2012 02:01:13 +0100 X-Google-Sender-Auth: NCd_5gITAjRSGQAsjZNe4iXa0dI 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 01:01:16 -0000 On Sun, Sep 16, 2012 at 1:22 AM, John Baldwin wrote: > 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. This is the patch to have _rmlock.h in pcpu.h. If nobody has objections I can commit monday night. I think this or the previous version are both good to go. Thanks, Attilio Index: sys/sys/_rmlock.h =================================================================== --- sys/sys/_rmlock.h (revision 240545) +++ sys/sys/_rmlock.h (working copy) @@ -32,17 +32,17 @@ #ifndef _SYS__RMLOCK_H_ #define _SYS__RMLOCK_H_ -/* - * XXXUPS remove as soon as we have per cpu variable - * linker sets and can define rm_queue in _rm_lock.h -*/ -#include /* * Mostly reader/occasional writer lock. */ LIST_HEAD(rmpriolist,rm_priotracker); +struct rm_queue { + struct rm_queue *volatile rmq_next; + struct rm_queue *volatile rmq_prev; +}; + struct rmlock { struct lock_object lock_object; volatile cpuset_t rm_writecpus; Index: sys/sys/pcpu.h =================================================================== --- sys/sys/pcpu.h (revision 240545) +++ sys/sys/pcpu.h (working copy) @@ -38,7 +38,11 @@ #endif #include +#include +#include +#include #include +#include #include #include #include @@ -137,15 +141,6 @@ extern uintptr_t dpcpu_off[]; #endif /* _KERNEL */ -/* - * XXXUPS remove as soon as we have per cpu variable - * linker sets and can define rm_queue in _rm_lock.h - */ -struct rm_queue { - struct rm_queue* volatile rmq_next; - struct rm_queue* volatile rmq_prev; -}; - /* * This structure maps out the global data that needs to be kept on a * per-cpu basis. The members are accessed via the PCPU_GET/SET/PTR @@ -169,15 +164,7 @@ struct pcpu { void *pc_netisr; /* netisr SWI cookie */ int pc_dnweight; /* vm_page_dontneed() */ int pc_domain; /* Memory domain. */ - - /* - * Stuff for read mostly lock - * - * XXXUPS remove as soon as we have per cpu variable - * linker sets. - */ - struct rm_queue pc_rm_queue; - + struct rm_queue pc_rm_queue; /* rmlock list of trackers */ uintptr_t pc_dynamic; /* Dynamic per-cpu data area */ /*