Date: Mon, 10 Mar 2008 10:40:10 -0700 From: Julian Elischer <julian@ironport.com> To: Stephan Uphoff <ups@freebsd.org> Cc: FreeBSD Current <current@freebsd.org> Subject: Re: critical_exit() Message-ID: <47D5727A.7000504@ironport.com> In-Reply-To: <47D543C5.9050008@freebsd.org> References: <47D4D534.9050902@ironport.com> <47D543C5.9050008@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------060807060502040804010705
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
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
--------------060807060502040804010705--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47D5727A.7000504>