Re: critical_exit()

Stephan Uphoff wrote:
> Julian Elischer wrote:
>>
>> Why would the following:
>> void
>> critical_exit(void)
>> {
>>        struct thread *td;
>>
>>        td = curthread;
>>        KASSERT(td->td_critnest != 0,
>>            ("critical_exit: td_critnest == 0"));
>>
>>        if (td->td_critnest == 1) {
>>                td->td_critnest = 0;
>>                if (td->td_owepreempt) {
>>                        td->td_critnest = 1;
>>                        thread_lock(td);
>>                        td->td_critnest--;
>>                        SCHED_STAT_INC(switch_owepreempt);
>>                        mi_switch(SW_INVOL|SW_PREEMPT, NULL);
>>                        thread_unlock(td);
>>                }
>>        } else
>>                td->td_critnest--;
>>
>>        CTR4(KTR_CRITICAL, "critical_exit by thread %p (%ld, %s) to 
>> %d", td,
>>            (long)td->td_proc->p_pid, td->td_name, td->td_critnest);
>> }
>>
>>
>> not be expressed:
>>
>> void
>> critical_exit(void)
>> {
>>        struct thread *td;
>>
>>        td = curthread;
>>        KASSERT(td->td_critnest != 0,
>>            ("critical_exit: td_critnest == 0"));
>>
>>        if (td->td_critnest == 1) {
>>                if (td->td_owepreempt) {
>>                        thread_lock(td);
>>                        td->td_critnest = 0;
>>                        SCHED_STAT_INC(switch_owepreempt);
>>                        mi_switch(SW_INVOL|SW_PREEMPT, NULL);
>>                        thread_unlock(td);
>>                } else {
> XXXXX If preemption happens here td_owepreempt will be set to preempt 
> the current thread
> XXXXX since td_critnest != 0 . However  td_owepreempt is not checked 
> again so we will not
> XXXXX preempt on td_critnest =  0;

jeff's comment was that it could be expresssed as:

if (--(td->td_critnest) == 0) {
              if (td->td_owepreempt) {
                       thread_lock(td);
                       td->td_critnest = 0;
                       SCHED_STAT_INC(switch_owepreempt);
                       mi_switch(SW_INVOL|SW_PREEMPT, NULL);
                       thread_unlock(td);
              }
 }

This has the same race.. but as you say, it probably doesn't matter.
In fact the race is probably required to ensure that pre-emption Does 
occur one way or another.




>>                        td_critnest =  0;
>>                }
>>        } else
>>                td->td_critnest--;
>>
>>        CTR4(KTR_CRITICAL, "critical_exit by thread %p (%ld, %s) to 
>> %d", td,
>>            (long)td->td_proc->p_pid, td->td_name, td->td_critnest);
>> }
>>
>> It seems to me there is a race in the current version, where the
>> critical count is temporarily 0, where the thread could be pre-empted
>> when it shouldn't be..
>
> Yes - there is a race where the thread could be preempted twice.
> However this is fairly harmless in comparison to not being preempted 
> at all.
> This being said it may be worthwhile to see if that race can be fixed  
> now after
> the thread lock changes.
>
>>
>> (prompted by a comment by jeffr that made me go look at this code)..
>>
>
> Stephan