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

[John Baldwin]

On 9/9/12 3:23 PM, Attilio Rao wrote:
> On Sun, Sep 9, 2012 at 8:15 PM, John Baldwin <jhb at freebsd.org> wrote:
>> On 9/9/12 11:03 AM, Attilio Rao wrote:
>>> On 8/2/12, Attilio Rao <attilio at freebsd.org> wrote:
>>>> On 7/30/12, John Baldwin <jhb at freebsd.org> wrote:
>>>
>>> [ trimm ]
>>>
>>>>> --- //depot/projects/smpng/sys/kern/subr_turnstile.c        2012-06-04
>>>>> 18:27:32.000000000 0000
>>>>> +++ //depot/user/jhb/lock/kern/subr_turnstile.c     2012-06-05
>>>>> 00:27:57.000000000 0000
>>>>> @@ -684,6 +684,7 @@
>>>>>     if (owner)
>>>>>             MPASS(owner->td_proc->p_magic == P_MAGIC);
>>>>>     MPASS(queue == TS_SHARED_QUEUE || queue == TS_EXCLUSIVE_QUEUE);
>>>>> +   KASSERT(!TD_IS_IDLETHREAD(td), ("idle threads cannot block on locks"));
>>>>>
>>>>>     /*
>>>>>      * If the lock does not already have a turnstile, use this thread's
>>>>
>>>> I'm wondering if we should also use similar checks in places doing
>>>> adaptive spinning (including the TD_NO_SLEEPING check). Likely yes.
>>>
>>> So what do you think about this?
>>
>> This is isn't really good enough then.  An idle thread should not
>> acquire any lock that isn't a spin lock.  Instead, you would be
>> better off removing the assert I added above and adding an assert to
>> mtx_lock(), rw_{rw}lock(), sx_{sx}lock(), lockmgr(), rm_{rw}lock() and
>> all the try variants of those.
> 
> While this is true, I thought about this route but I didn't want to go
> for it because it would pollute much more code than the current
> approach + patch I proposed, which would enough to find offending
> cases.
> I'm not sure I want to pollute all the kernel locking with checks for
> idlethread, yet I think the current code is not complete and thus I
> still think my patch is a reasonable compromise.

I don't quite agree.  We already pollute pretty much all of those with
'curthread != NULL' checks.  This isn't all that different from just
adding one of those.  Also, just about all of those functions above do
adaptive spinning and require a patch via your method, so it's really
not that much more pollution to just do the full check.

-- 
John Baldwin