Date: Tue, 11 Sep 2012 02:11:22 +0100 From: Attilio Rao <attilio@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: Konstantin Belousov <kostikbel@gmail.com>, Davide Italiano <davide@freebsd.org>, src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern Message-ID: <CAJ-FndCW91Y=SB3GFXDxtXGQdQzUzyF5KzKqjtuFYGs0W0-w6g@mail.gmail.com> In-Reply-To: <504CF1FB.9090106@FreeBSD.org> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com> <CAJ-FndDnO7wjnWPV0tTu%2BUGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com> <201207301732.33474.jhb@freebsd.org> <CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com> <CAJ-FndARWZGwdiLeaQcnM%2BM%2Bm8zmBLuMrTkgoCFeesXPR=08pA@mail.gmail.com> <504CEAE0.704@FreeBSD.org> <CAJ-FndCuQz8mJwLMUM3j9rAfvkH3848U6t7wv-c=8YerTKUdOw@mail.gmail.com> <504CF1FB.9090106@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Sep 9, 2012 at 8:46 PM, John Baldwin <jhb@freebsd.org> wrote:
> On 9/9/12 3:23 PM, Attilio Rao wrote:
>> On Sun, Sep 9, 2012 at 8:15 PM, John Baldwin <jhb@freebsd.org> wrote:
>>> On 9/9/12 11:03 AM, Attilio Rao wrote:
>>>> On 8/2/12, Attilio Rao <attilio@freebsd.org> wrote:
>>>>> On 7/30/12, John Baldwin <jhb@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.
Speaking of which, I think it is time for curthread != NULL checks in
the locking primitives to go, or there is a good reason I really don't
understand to keep them?
Attilio
--
Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndCW91Y=SB3GFXDxtXGQdQzUzyF5KzKqjtuFYGs0W0-w6g>