Date: Fri, 14 Sep 2012 23:32:35 +0100 From: Attilio Rao <attilio@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: Davide Italiano <davide@freebsd.org>, mlaier@freebsd.org, svn-src-projects@freebsd.org, Konstantin Belousov <kostikbel@gmail.com>, src-committers@freebsd.org, Stephan Uphoff <ups@freebsd.org> Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern Message-ID: <CAJ-FndBvs1F%2BbXfvL-U2yTi313mebuZ6KidtDqh_CfchxX7dAg@mail.gmail.com> In-Reply-To: <CAJ-FndByCLNpGoFFELQVmC61YdBFn4USunVHB1c7=ZHFoZ9V2g@mail.gmail.com> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201209130910.50876.jhb@freebsd.org> <CAJ-FndASH1=i4ozwP=YepF58iC_5%2Bnf4L4MCu3%2B2-xB9FVzyvg@mail.gmail.com> <201209131132.21103.jhb@freebsd.org> <CAJ-FndByCLNpGoFFELQVmC61YdBFn4USunVHB1c7=ZHFoZ9V2g@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Sep 13, 2012 at 5:20 PM, Attilio Rao <attilio@freebsd.org> wrote:
> On 9/13/12, John Baldwin <jhb@freebsd.org> wrote:
>> On Thursday, September 13, 2012 10:38:54 am Attilio Rao wrote:
>>> On Thu, Sep 13, 2012 at 2:10 PM, John Baldwin <jhb@freebsd.org> wrote:
>>> > On Wednesday, September 12, 2012 9:36:58 pm Attilio Rao wrote:
>>> >> On Thu, Aug 2, 2012 at 10:07 PM, John Baldwin <jhb@freebsd.org> wrote:
>>> >> > On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote:
>>> >> >> On 7/30/12, John Baldwin <jhb@freebsd.org> 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 <attilio@pcbsd-2488.(none)>
---
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 <sys/pcpu.h>
/*
* 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 */
/*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndBvs1F%2BbXfvL-U2yTi313mebuZ6KidtDqh_CfchxX7dAg>