Date: Sun, 16 Sep 2012 02:01:13 +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-FndByG9pX0rxkzKiM4xU2OV_JdXSPBkg8jOmeCEdZ0dZmSA@mail.gmail.com> In-Reply-To: <50551BCA.4020303@FreeBSD.org> 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> <CAJ-FndBvs1F%2BbXfvL-U2yTi313mebuZ6KidtDqh_CfchxX7dAg@mail.gmail.com> <505514D5.90800@FreeBSD.org> <CAJ-FndAaBDjdXzT6vjiJAVgOSmA_YNxbQAyxu0z5EnvCb37Sjw@mail.gmail.com> <50551BCA.4020303@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Sep 16, 2012 at 1:22 AM, John Baldwin <jhb@freebsd.org> wrote:
> On 9/15/12 8:12 PM, Attilio Rao wrote:
>> On Sun, Sep 16, 2012 at 12:52 AM, John Baldwin <jhb@freebsd.org> wrote:
>>> On 9/14/12 6:32 PM, Attilio Rao wrote:
>>>> 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?
>>>
>>> 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 <sys/pcpu.h>
/*
* 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 <sys/_cpuset.h>
+#include <sys/_lock.h>
+#include <sys/_mutex.h>
+#include <sys/_sx.h>
#include <sys/queue.h>
+#include <sys/_rmlock.h>
#include <sys/vmmeter.h>
#include <sys/resource.h>
#include <machine/pcpu.h>
@@ -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 */
/*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndByG9pX0rxkzKiM4xU2OV_JdXSPBkg8jOmeCEdZ0dZmSA>