Date: Tue, 17 May 2011 12:16:45 -0700 From: Max Laier <max@love2party.net> To: John Baldwin <jhb@freebsd.org> Cc: neel@freebsd.org, Andriy Gapon <avg@freebsd.org>, Attilio Rao <attilio@freebsd.org>, FreeBSD current <freebsd-current@freebsd.org>, Stephan Uphoff <ups@freebsd.org>, Peter Grehan <grehan@freebsd.org> Subject: Re: proposed smp_rendezvous change Message-ID: <4DD2C99D.50203@love2party.net> In-Reply-To: <201105171256.41091.jhb@freebsd.org> References: <4DCD357D.6000109@FreeBSD.org> <4DD26720.3000001@FreeBSD.org> <4DD2A058.6050400@love2party.net> <201105171256.41091.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 05/17/2011 09:56 AM, John Baldwin wrote:
> On Tuesday, May 17, 2011 12:20:40 pm Max Laier wrote:
>> On 05/17/2011 05:16 AM, John Baldwin wrote:
>> ...
>>> Index: kern/kern_switch.c
>>> ===================================================================
>>> --- kern/kern_switch.c (revision 221536)
>>> +++ kern/kern_switch.c (working copy)
>>> @@ -192,15 +192,22 @@
>>> critical_exit(void)
>>> {
>>> struct thread *td;
>>> - int flags;
>>> + int flags, owepreempt;
>>>
>>> td = curthread;
>>> KASSERT(td->td_critnest != 0,
>>> ("critical_exit: td_critnest == 0"));
>>>
>>> if (td->td_critnest == 1) {
>>> + owepreempt = td->td_owepreempt;
>>> + td->td_owepreempt = 0;
>>> + /*
>>> + * XXX: Should move compiler_memory_barrier() from
>>> + * rmlock to a header.
>>> + */
>>
>> XXX: If we get an interrupt at this point and td_owepreempt was zero,
>> the new interrupt will re-set it, because td_critnest is still non-zero.
>>
>> So we still end up with a thread that is leaking an owepreempt *and*
>> lose a preemption.
>
> I don't see how this can still leak owepreempt. The nested interrupt should
> do nothing (except for possibly set owepreempt) until td_critnest is 0.
Exactly. The interrupt sets owepreempt and after we return here, we set
td_critnest to 0 and exit without clearing owepreempt. Hence we leak
the owepreempt.
> However, we can certainly lose preemptions.
>
> I wonder if we can abuse the high bit of td_critnest for the owepreempt flag
> so it is all stored in one cookie. We only set owepreempt while holding
> thread_lock() (so interrupts are disabled), so I think we would be ok and not
> need atomic ops.
>
> Hmm, actually, the top-half code would have to use atomic ops. Nuts. Let me
> think some more.
I think these two really belong into one single variable. Setting the
owepreempt flag can be a normal RMW. Increasing and decreasing critnest
must be atomic (otherwise we could lose the flag) and dropping the final
reference would work like this:
if ((curthread->td_critnest & TD_CRITNEST_MASK) == 1) {
unsigned int owe;
owe = atomic_readandclear(&curthread->td_critnest);
if (owe & TD_OWEPREEMPT_FLAG) {
/* do the switch */
}
That should do it ... I can put that into a patch, if we agree that's
the right thing to do.
Thanks,
Max
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4DD2C99D.50203>