svn commit: r238907 - projects/calloutng/sys/kern

[Attilio Rao]

On Sun, Sep 16, 2012 at 12:52 AM, John Baldwin <jhb at 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 at freebsd.org> wrote:
>>> On 9/13/12, John Baldwin <jhb at 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 at 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 at freebsd.org> wrote:
>>>>>>>> On Thursday, August 02, 2012 4:56:03 pm Attilio Rao wrote:
>>>>>>>>> On 7/30/12, John Baldwin <jhb at 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.

Attilio


-- 
Peace can only be achieved by understanding - A. Einstein