From owner-freebsd-current@FreeBSD.ORG Mon Mar 10 14:46:53 2008 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 70E8C106567B for ; Mon, 10 Mar 2008 14:46:53 +0000 (UTC) (envelope-from ups@freebsd.org) Received: from smtpout04-01.prod.mesa1.secureserver.net (smtpout04-01.prod.mesa1.secureserver.net [64.202.165.196]) by mx1.freebsd.org (Postfix) with SMTP id 63EF28FC2F for ; Mon, 10 Mar 2008 14:46:53 +0000 (UTC) (envelope-from ups@freebsd.org) Received: (qmail 2762 invoked from network); 10 Mar 2008 14:20:12 -0000 Received: from unknown (66.23.216.53) by smtpout04-04.prod.mesa1.secureserver.net (64.202.165.199) with ESMTP; 10 Mar 2008 14:20:12 -0000 Message-ID: <47D543C5.9050008@freebsd.org> Date: Mon, 10 Mar 2008 10:20:53 -0400 From: Stephan Uphoff User-Agent: Thunderbird 2.0.0.9 (Macintosh/20071031) MIME-Version: 1.0 To: Julian Elischer References: <47D4D534.9050902@ironport.com> In-Reply-To: <47D4D534.9050902@ironport.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: FreeBSD Current Subject: Re: critical_exit() X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Mar 2008 14:46:53 -0000 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