From owner-svn-src-all@FreeBSD.ORG Mon Apr 16 14:18:02 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E8E631065673; Mon, 16 Apr 2012 14:18:02 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id A25828FC1B; Mon, 16 Apr 2012 14:18:02 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id DF20BB94A; Mon, 16 Apr 2012 10:18:01 -0400 (EDT) From: John Baldwin To: "Justin T. Gibbs" Date: Mon, 16 Apr 2012 09:54:00 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p10; KDE/4.5.5; amd64; ; ) References: <201204092241.q39MfJZn081610@svn.freebsd.org> <20120410114118.GB93449@alchemy.franken.de> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201204160954.00991.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 16 Apr 2012 10:18:02 -0400 (EDT) Cc: Attilio Rao , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Marius Strobl Subject: Re: svn commit: r234074 - in head/sys: amd64/amd64 i386/i386 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Apr 2012 14:18:03 -0000 On Saturday, April 14, 2012 7:05:35 pm Justin T. Gibbs wrote: > > On Apr 10, 2012, at 5:41 AM, Marius Strobl wrote: > > > On Tue, Apr 10, 2012 at 01:03:56AM +0100, Attilio Rao wrote: > >> Il 10 aprile 2012 00:09, Marius Strobl ha scritto: > >>> On Mon, Apr 09, 2012 at 10:41:19PM +0000, Attilio Rao wrote: > >>>> Author: attilio > >>>> Date: Mon Apr ??9 22:41:19 2012 > >>>> New Revision: 234074 > >>>> URL: http://svn.freebsd.org/changeset/base/234074 > >>>> > >>>> Log: > >>>> ?? BSP is not added to the mask of valid target CPUs for interrupts > >>>> ?? in set_apic_interrupt_ids(). Besides, set_apic_interrupts_ids() is not > >>>> ?? called in the !SMP case too. > >>>> ?? Fix this by: > >>>> ?? - Adding the BSP as an interrupt target directly in cpu_startup(). > >>>> ?? - Remove an obsolete optimization where the BSP are skipped in > >>>> ?? ?? set_apic_interrupt_ids(). > >>>> > >>>> ?? Reported by: ?? ?? ?? ??jh > >>>> ?? Reviewed by: ?? ?? ?? ??jhb > >>>> ?? MFC after: ??3 days > >>>> ?? X-MFC: ?? ?? ?? ?? ?? ?? ??r233961 > >>>> ?? Pointy hat to: ?? ?? ??me > >>>> > >>>> Modified: > >>>> ?? head/sys/amd64/amd64/machdep.c > >>>> ?? head/sys/amd64/amd64/mp_machdep.c > >>>> ?? head/sys/i386/i386/machdep.c > >>>> ?? head/sys/i386/i386/mp_machdep.c > >>>> > >>>> Modified: head/sys/amd64/amd64/machdep.c > >>>> ============================================================================== > >>>> --- head/sys/amd64/amd64/machdep.c ?? ??Mon Apr ??9 22:01:43 2012 ?? ?? ?? ??(r234073) > >>>> +++ head/sys/amd64/amd64/machdep.c ?? ??Mon Apr ??9 22:41:19 2012 ?? ?? ?? ??(r234074) > >>>> @@ -295,6 +295,11 @@ cpu_startup(dummy) > >>>> ?? ?? ?? vm_pager_bufferinit(); > >>>> > >>>> ?? ?? ?? cpu_setregs(); > >>>> + > >>>> + ?? ?? /* > >>>> + ?? ?? ??* Add BSP as an interrupt target. > >>>> + ?? ?? ??*/ > >>>> + ?? ?? intr_add_cpu(0); > >>>> ??} > >>> > >>> If I'm not mistaken, intr_add_cpu() is under #ifdef SMP, so it should be > >>> here as well. > >> > >> You are right, sorry, I did forgot to test without SMP. > >> I think we still need intr_add_cpu() on cpu_startup() because of the > >> case smp_disabled = 1. > >> I think the attached patch should make its dirty job, opinion? > > > > I currently fail to see why the latter approach would be necessary, > > i.e. IMO wrapping the intr_add_cpu() calls in cpu_startup() should > > be sufficient. In case the kernel is compiled without SMP support, > > interrupt balancing support isn't available in the first place and > > the BSP is always the only available target (see the UP version of > > intr_next_cpu() at the end of x86/x86/intr_machdep.c), so there's > > no need to add the BSP as a valid target. If an SMP kernel is run > > on a UP machine or with SMP disabled, interrupt balancing support > > is available but the intr_add_cpu() calls in cpu_startup() will add > > the BSP as (the only) target, so everything should be fine. Maybe > > you can elaborate on why you think an SMP kernel with SMP disabled > > needs special handling. > > > > Marius > > While functionally correct, I believe that wrapping intr_add_cpu() > in machdep.c in "SMP ifdefs" is inferior to calling it in all cases. > It invites questions like, "In the UP case, don't we have to ensure > that CPU0 is a valid interrupt target?" This is because casual > visitors to this file don't know that intr_add_cpu() only impacts > interrupt distribution. Of course, this is just an artifact of the > current implementation. #ifdefs should be as close to the implemenation > as possible. This simplifies the task of making future enhancments. > This is why I'd prefer to see these within the body of intr_add_cpu() > than where they are now. > > I also think the comment could be improved to be something like: > > /* > * The BSP/CPU0 is always an interrupt target even if > * our probe of MP hardware fails or MP mode is disabled. > */ > intr_add_cpu(0); This is why my original patch had this all self-contained inside the #ifdef SMP in sys/x86/x86/intr_machdep.c via a new SYSINIT(). -- John Baldwin