From owner-freebsd-current@FreeBSD.ORG Mon Mar 10 17:40:01 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 8A5371065671 for ; Mon, 10 Mar 2008 17:40:01 +0000 (UTC) (envelope-from prvs=julian=9481976d6@ironport.com) Received: from smtp2-outbound.ironport.com (smtp2-outbound.ironport.com [63.251.108.116]) by mx1.freebsd.org (Postfix) with ESMTP id 73B6D8FC21 for ; Mon, 10 Mar 2008 17:40:01 +0000 (UTC) (envelope-from prvs=julian=9481976d6@ironport.com) DomainKey-Signature: s=key512; d=ironport.com; c=nofws; q=dns; h=Received:Message-ID:Date:From:User-Agent:MIME-Version:To: CC:Subject:References:In-Reply-To:Content-Type; b=ZJUQqK2j9le/DHsBjMUNx0xO8EPDskvFP4CC748LsJw2CGU7x+V/xY8S kOLSZlGDWVgLnDlG1TJIEyGU0tsgOQ==; Received: from jelischer-laptop.sfo.ironport.com (HELO julian-mac.elischer.org) ([10.251.22.38]) by smtp2-outbound.ironport.com with ESMTP; 10 Mar 2008 10:40:00 -0700 Message-ID: <47D5727A.7000504@ironport.com> Date: Mon, 10 Mar 2008 10:40:10 -0700 From: Julian Elischer User-Agent: Thunderbird 2.0.0.12 (Macintosh/20080213) MIME-Version: 1.0 To: Stephan Uphoff References: <47D4D534.9050902@ironport.com> <47D543C5.9050008@freebsd.org> In-Reply-To: <47D543C5.9050008@freebsd.org> Content-Type: multipart/mixed; boundary="------------060807060502040804010705" X-Mailman-Approved-At: Mon, 10 Mar 2008 17:50:53 +0000 X-Content-Filtered-By: Mailman/MimeDel 2.1.5 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 17:40:01 -0000 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--