Re: proposed smp_rendezvous change

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)
>>> _at__at_ -192,15 +192,22 _at__at_
>>> 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