From owner-freebsd-current@FreeBSD.ORG Mon Mar 10 22:57:38 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 68EDD1065676 for ; Mon, 10 Mar 2008 22:57:38 +0000 (UTC) (envelope-from kip.macy@gmail.com) Received: from wr-out-0506.google.com (wr-out-0506.google.com [64.233.184.224]) by mx1.freebsd.org (Postfix) with ESMTP id 06FA78FC19 for ; Mon, 10 Mar 2008 22:57:37 +0000 (UTC) (envelope-from kip.macy@gmail.com) Received: by wr-out-0506.google.com with SMTP id c49so1247244wra.19 for ; Mon, 10 Mar 2008 15:57:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:to:subject:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; bh=6/pVjb8nN5ZipMTh57jg1n5ACb09LjbMbZlnJ6PVhm0=; b=HyoLTS/ksChodxfJ0lnYa3ce71972cVUae7bvNcTFK7TntrRvbcANt7oKe9gZ3ciqTGJGJFbqUpUez2HTo3LXl+E0yKFg6i8mDz8ES0zYk/zncirq9/mtCxs2WjCGz/DtzVZYeX9Gz90nAVxZ8U2gZjKIECAi1BLXnMggaJsac0= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:to:subject:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; b=sffWM32Y+nf17jdCVs+VY6xJ1FRYQADYL8PFaKn95G7r7+WMF8qMz2Sb7oN7pp1SzbvJ8b7kH1cIzBkBOnPYhUK8M6hqz4ON5LsqiPk3qwKj11BCEOjSwVA7NXbBTUBwFPQ7toHshGlbh9mkHJX03Oj9lj5FhUmHY85+iwvo1SQ= Received: by 10.114.53.1 with SMTP id b1mr3936024waa.86.1205189856362; Mon, 10 Mar 2008 15:57:36 -0700 (PDT) Received: by 10.115.22.10 with HTTP; Mon, 10 Mar 2008 15:57:36 -0700 (PDT) Message-ID: Date: Mon, 10 Mar 2008 15:57:36 -0700 From: "Kip Macy" To: "Jeff Roberson" , "Julian Elischer" , "Stephan Uphoff" , "FreeBSD Current" In-Reply-To: <20080310111256.N1091@desktop> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <47D4D534.9050902@ironport.com> <47D543C5.9050008@freebsd.org> <47D5727A.7000504@ironport.com> <20080310111256.N1091@desktop> Cc: 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 22:57:38 -0000 I've done that in my tree, but there it was more to reduce pmc noise (critical_{enter,exit} would account for 5-8% of cycles). -Kip On 3/10/08, Jeff Roberson wrote: > On Mon, 10 Mar 2008, Julian Elischer wrote: > > > 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); > > } > > } > > if (--(td->td_critnest) == 0) { > if (td->td_owepreempt) { > thread_lock(td); > if (td->td_owepreempt) { > SCHED_STAT_INC(switch_owepreempt); > mi_switch(SW_INVOL|SW_PREEMPT, NULL); > } > thread_unlock(td); > } > } > > Wouldn't that do just fine? If a preemption occurred before you disabled > interrupts in thread_lock you'd skip the switch after acquiring it. > > I don't see a way that we could miss owepreempt with the above code. I'd > also like to make critical_enter/critical_exit inlines with a > _critical_exit() that does the switch. with thread_lock we now do a lot > more nested spinlocking etc. All of these non-contiguous instruction > pointers add up. > > > > > 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 > > > > > _______________________________________________ > freebsd-current@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-current > To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org" >