Date: Mon, 10 Mar 2008 10:20:53 -0400 From: Stephan Uphoff <ups@freebsd.org> To: Julian Elischer <julian@ironport.com> Cc: FreeBSD Current <current@freebsd.org> Subject: Re: critical_exit() Message-ID: <47D543C5.9050008@freebsd.org> In-Reply-To: <47D4D534.9050902@ironport.com> References: <47D4D534.9050902@ironport.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47D543C5.9050008>