From owner-svn-src-projects@FreeBSD.ORG Fri Sep 14 22:32:40 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 1F026106566C; Fri, 14 Sep 2012 22:32:40 +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 8CA158FC0A; Fri, 14 Sep 2012 22:32:37 +0000 (UTC) Received: by lage12 with SMTP id e12so3718141lag.13 for ; Fri, 14 Sep 2012 15:32:36 -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=vV9KhfTokeTtPnj5wNiUOgBCih5T2t5gmoeMSyMU3p8=; b=qaW5MovH6XX81eyekUhUQCU1ILJX270pSNQuMoDetlSNKPWJENlxFCypfBW/A80in/ XnyfOASwtyYGRj/WjZW/IRuhha81XPrILDwzPXLzliogebOlBkVIUHJDvmO8TICfezNE k2yFiA/+GIaERN/HBTzO+gUTG4fAZMQKlBmcirtKdxqMHFNpMaZMfaRsn0cN3uXxOKNq fv7lsui0FWgStpHy0vCXk0P1WHkPvHKw9T9C3BGGAtuIOn6NDX0E1tkhTUSqdXA1MJdZ 61RJ11+lN9aK6PelHJyQO99oiTODzpBLqWWjiFjnWqus5qpqI0ZGJN74iV9JGZj9ZLP4 1m0w== MIME-Version: 1.0 Received: by 10.152.131.37 with SMTP id oj5mr3795243lab.14.1347661956528; Fri, 14 Sep 2012 15:32:36 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Fri, 14 Sep 2012 15:32:35 -0700 (PDT) In-Reply-To: References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209130910.50876.jhb@freebsd.org> <201209131132.21103.jhb@freebsd.org> Date: Fri, 14 Sep 2012 23:32:35 +0100 X-Google-Sender-Auth: mx4oo8WVkKm-09_CQAOD5N_uyQ8 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: Fri, 14 Sep 2012 22:32:40 -0000 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? Thanks, Attilio Subject: [PATCH 9/9] Avoid namespace pollution of sys/sys/pcpu.h in sys/sys/_rmlock.h. pc_rm_queue should really be implemented as a dynamic per-cpu object. but, the read-mode operation should be kept as fast as possible, so in order to avoid the extra-indirection from accessing pc_dynamic, just make it a static part of the per-cpu area. Avoid further the namespace pollution of pcpu.h in _rmlock.h by defining a verbatim copy of struct rm_queue as it is needed. There could be a CTASSERT() in the headers in the case the struct is already defined in order to see if the size matches, but generally this should not be too important as the verbatim can be easilly found by grepping. Also, note that an inclusion of _rmlock.h into pcpu.h would be problematic because _rmlock.h also requires _sx.h which in turn requires _lock.h (and this may not be the end) which results in another namespace pollution. Signed-off-by: Attilio Rao --- sys/sys/_rmlock.h | 15 ++++++++++----- sys/sys/pcpu.h | 25 +++++++++++++------------ 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/sys/sys/_rmlock.h b/sys/sys/_rmlock.h index 15d6c49..f2e713f 100644 --- a/sys/sys/_rmlock.h +++ b/sys/sys/_rmlock.h @@ -32,15 +32,20 @@ #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. */ +/* Check the comment present in sys/pcpu.h. */ +#ifndef RMLOCK_DEFINE_RM_QUEUE +#define RMLOCK_DEFINE_RM_QUEUE + +struct rm_queue { + struct rm_queue* volatile rmq_next; + struct rm_queue* volatile rmq_prev; +}; +#endif + LIST_HEAD(rmpriolist,rm_priotracker); struct rmlock { diff --git a/sys/sys/pcpu.h b/sys/sys/pcpu.h index 4a4ec00..6536255 100644 --- a/sys/sys/pcpu.h +++ b/sys/sys/pcpu.h @@ -137,14 +137,23 @@ 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 +#ifndef RMLOCK_DEFINE_RM_QUEUE +#define RMLOCK_DEFINE_RM_QUEUE + +/* + * pc_rm_queue should really be implemented as a dynamic per-cpu object. + * Anyway, the read-mode operation should be kept as fast as possible, + * so in order to avoid the extra-indirection from accessing pc_dynamic, + * just make it a static part of the per-cpu area. + * Also, the definition of the struct rm_queue must be present in both + * sys/sys/_rmlock.h and sys/sys/pcpu.h. In order to avoid namespace + * pollutions, however, in a way and the other, use a verbatim definition. */ struct rm_queue { struct rm_queue* volatile rmq_next; struct rm_queue* volatile rmq_prev; }; +#endif /* * This structure maps out the global data that needs to be kept on a @@ -169,15 +178,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 */ /*