From owner-svn-src-head@FreeBSD.ORG Mon Apr 9 18:38:10 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id B24FB1065674; Mon, 9 Apr 2012 18:38:10 +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 59E818FC17; Mon, 9 Apr 2012 18:38:10 +0000 (UTC) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id C65C0B915; Mon, 9 Apr 2012 14:38:09 -0400 (EDT) From: John Baldwin To: Attilio Rao Date: Mon, 9 Apr 2012 14:35:31 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p10; KDE/4.5.5; amd64; ; ) References: <201204062119.q36LJTKR026564@svn.freebsd.org> <201204091314.20775.jhb@freebsd.org> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201204091435.31893.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 09 Apr 2012 14:38:09 -0400 (EDT) Cc: svn-src-head@freebsd.org, Jaakko Heinonen , svn-src-all@freebsd.org, "Justin T. Gibbs" , src-committers@freebsd.org Subject: Re: svn commit: r233961 - head/sys/x86/x86 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Apr 2012 18:38:10 -0000 On Monday, April 09, 2012 1:59:13 pm Attilio Rao wrote: > Il 09 aprile 2012 18:14, John Baldwin ha scritto: > > On Monday, April 09, 2012 12:58:07 pm Attilio Rao wrote: > >> Il 09 aprile 2012 17:34, John Baldwin ha scritto: > >> > On Monday, April 09, 2012 11:45:11 am Jaakko Heinonen wrote: > >> >> > >> >> Hi, > >> >> > >> >> On 2012-04-06, Justin T. Gibbs wrote: > >> >> > Fix interrupt load balancing regression, introduced in revision > >> >> > 222813, that left all un-pinned interrupts assigned to CPU 0. > >> >> > > >> >> > sys/x86/x86/intr_machdep.c: > >> >> > In intr_shuffle_irqs(), remove CPU_SETOF() call that initialized > >> >> > the "intr_cpus" cpuset to only contain CPU0. > >> >> > > >> >> > This initialization is too late and nullifies the results of calls > >> >> > the intr_add_cpu() that occur much earlier in the boot process. > >> >> > Since "intr_cpus" is statically initialized to the empty set, and > >> >> > all processors, including the BSP, already add themselves to > >> >> > "intr_cpus" no special initialization for the BSP is necessary. > >> >> > >> >> My Pentium 4 system hangs on boot after this commit. These are the last > >> >> lines from a verbose boot: > >> >> > >> >> SMP: AP CPU #1 Launched! > >> >> cpu1 AP: > >> >> ID: 0x01000000 VER: 0x00050014 LDR: 0x00000000 DFR: 0xffffffff > >> >> lint0: 0x00010700 lint1: 0x00000400 TPR: 0x00000000 SVR: 0x000001ff > >> >> timer: 0x000100ef therm: 0x00010000 err: 0x000000f0 pmc: 0x00010400 > >> >> > >> >> The system boots with r233960. > >> >> > >> >> Some information: > >> >> > >> >> CPU: Intel(R) Pentium(R) 4 CPU 2.60GHz (2605.96-MHz 686-class CPU) > >> >> Origin = "GenuineIntel" Id = 0xf29 Family = f Model = 2 Stepping = > >> >> 9 > >> >> > >> > Features=0xbfebfbff > >> >> Features2=0x4400 > >> >> real memory = 2147483648 (2048 MB) > >> >> avail memory = 2085228544 (1988 MB) > >> >> Event timer "LAPIC" quality 400 > >> >> ACPI APIC Table: > >> >> FreeBSD/SMP: Multiprocessor System Detected: 2 CPUs > >> >> FreeBSD/SMP: 1 package(s) x 1 core(s) x 2 HTT threads > >> >> cpu0 (BSP): APIC ID: 0 > >> >> cpu1 (AP/HT): APIC ID: 1 > >> > > >> > I suspect in your case intr_add_cpu() is never called. I think Attilio is not > >> > correct in that it is not called for the BSP. > >> > > >> > Yes, it is not called for the BSP in set_interrupt_apic_ids(). This used to > >> > work because bit 0 was assigned statically. Also, in a UP machine > >> > set_interrupt_apic_ids() is not called at all. > >> > >> But why there is a front-end check for the BSP in set_interrupt_apic_ids()? > >> > >> Anyway, I think a better fix would be like the attached patch. > > > > This would be fine. What I would really prefer is to not need the sysinit at > > all and be able to do something like the original pre-cpuset code: > > > > static cpuset_t intr_cpus = CPU_INITIAILIZER(0); > > This is more difficult to do because it would require static array > initializations and it would pollute too much the code with compile > time, MAXCPU-dependant details. > > > Also, with the cpuset variant, I think we could remove the special case check > > for the BSP from set_apic_interrupt_ids() as it doesn't hurt to set it > > multiple times. IIRC, the pre-cpuset code kept a separate count which is > > why that would have been harmful. > > I'm not sure I follow, a separate count for what? The pre-cpuset code used a separate count IIRC. That is why duplicate calls to intr_add_cpu() used to be bad. However, they are no longer bad. > So do you consider the following patch as a real commit candidate? Yes, modulo a nit: > Index: sys/i386/i386/machdep.c > =================================================================== > --- sys/i386/i386/machdep.c (revisione 234064) > +++ sys/i386/i386/machdep.c (copia locale) > @@ -336,6 +336,11 @@ cpu_startup(dummy) > #ifndef XEN > cpu_setregs(); > #endif > + > + /* > + * Add BSP interrupt bitmask. > + */ > + intr_add_cpu(0); > } I would make this a single line comment and say: "Add BSP as an interrupt target." (More closely matches the original comment from intr_machdep.c). -- John Baldwin