Re: critical_exit()

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;
>                        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