From owner-svn-src-all@FreeBSD.ORG Mon Apr 9 22:32:57 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id BD157106566B; Mon, 9 Apr 2012 22:32:57 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by mx1.freebsd.org (Postfix) with ESMTP id 40C138FC12; Mon, 9 Apr 2012 22:32:56 +0000 (UTC) Received: by lbbgj3 with SMTP id gj3so192446lbb.13 for ; Mon, 09 Apr 2012 15:32:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=uHEukkGe16XM2DcKtT/CsN2qfSqOuIqcg93jR5eXfV0=; b=aVR4TXU+nADctZYWq8gSYIvgYFAi/7LjqgADaKOCR9Qn8bDpbcn58JeNgwAQjsxYvQ ymYaD2iXAQIPPu/XIE6gZV+X8DND4N87vYUPOIMiS8vRJNYF9x0txEKEmrXnQ2FAqPZJ ZrlMIXAQgOaTL60dQXXn1Ubx+0lMKg+yXaiur+dwZ+6kMWWPGAWErS9My8BB5gzTTzhY oNATMqxZua8jZJJkys5Plfxv8YakQBrBsawp0zY3gJLv2GWh7a098FxydwQhKyO80h0S NxYDMHvg0Fu4SSVkMdT8n1iYZyItKjPyHETPHRartcKNHD1xlIFl8bhX3BVKf1dCT5hg T6Lw== MIME-Version: 1.0 Received: by 10.152.147.202 with SMTP id tm10mr13471172lab.49.1334010775083; Mon, 09 Apr 2012 15:32:55 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.93.138 with HTTP; Mon, 9 Apr 2012 15:32:55 -0700 (PDT) In-Reply-To: <201204091435.31893.jhb@freebsd.org> References: <201204062119.q36LJTKR026564@svn.freebsd.org> <201204091314.20775.jhb@freebsd.org> <201204091435.31893.jhb@freebsd.org> Date: Mon, 9 Apr 2012 23:32:55 +0100 X-Google-Sender-Auth: YEsRMbsX__pAerU9VyNPCYt7Q0I Message-ID: From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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-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, 09 Apr 2012 22:32:57 -0000 Il 09 aprile 2012 19:35, John Baldwin ha scritto: > 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: >> >> >> > =C2=A0 Fix interrupt load balancing regression, introduced in re= vision >> >> >> > =C2=A0 222813, that left all un-pinned interrupts assigned to CP= U 0. >> >> >> > >> >> >> > =C2=A0 sys/x86/x86/intr_machdep.c: >> >> >> > =C2=A0 =C2=A0 In intr_shuffle_irqs(), remove CPU_SETOF() call th= at initialized >> >> >> > =C2=A0 =C2=A0 the "intr_cpus" cpuset to only contain CPU0. >> >> >> > >> >> >> > =C2=A0 =C2=A0 This initialization is too late and nullifies the = results of calls >> >> >> > =C2=A0 =C2=A0 the intr_add_cpu() that occur much earlier in the = boot process. >> >> >> > =C2=A0 =C2=A0 Since "intr_cpus" is statically initialized to the= empty set, and >> >> >> > =C2=A0 =C2=A0 all processors, including the BSP, already add the= mselves to >> >> >> > =C2=A0 =C2=A0 "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: >> >> >> =C2=A0 =C2=A0 =C2=A0ID: 0x01000000 =C2=A0 VER: 0x00050014 LDR: 0x0= 0000000 DFR: 0xffffffff >> >> >> =C2=A0 lint0: 0x00010700 lint1: 0x00000400 TPR: 0x00000000 SVR: 0x= 000001ff >> >> >> =C2=A0 timer: 0x000100ef therm: 0x00010000 err: 0x000000f0 pmc: 0x= 00010400 >> >> >> >> >> >> The system boots with r233960. >> >> >> >> >> >> Some information: >> >> >> >> >> >> CPU: Intel(R) Pentium(R) 4 CPU 2.60GHz (2605.96-MHz 686-class CPU) >> >> >> =C2=A0 Origin =3D "GenuineIntel" =C2=A0Id =3D 0xf29 =C2=A0Family = =3D f =C2=A0Model =3D 2 =C2=A0Stepping =3D >> >> >> 9 >> >> >> >> >> > > Features=3D0xbfebfbff >> >> >> =C2=A0 Features2=3D0x4400 >> >> >> real memory =C2=A0=3D 2147483648 (2048 MB) >> >> >> avail memory =3D 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 >> >> >> =C2=A0cpu0 (BSP): APIC ID: =C2=A00 >> >> >> =C2=A0cpu1 (AP/HT): APIC ID: =C2=A01 >> >> > >> >> > I suspect in your case intr_add_cpu() is never called. =C2=A0I thin= k 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(). =C2= =A0This used to >> >> > work because bit 0 was assigned statically. =C2=A0Also, in a UP mac= hine >> >> > 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. =C2=A0What I would really prefer is to not need th= e sysinit at >> > all and be able to do something like the original pre-cpuset code: >> > >> > static cpuset_t intr_cpus =3D 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 cas= e check >> > for the BSP from set_apic_interrupt_ids() as it doesn't hurt to set it >> > multiple times. =C2=A0 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. =C2=A0That is why duplica= te calls > to intr_add_cpu() used to be bad. =C2=A0However, 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 >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> --- sys/i386/i386/machdep.c =C2=A0 =C2=A0 (revisione 234064) >> +++ sys/i386/i386/machdep.c =C2=A0 =C2=A0 (copia locale) >> @@ -336,6 +336,11 @@ cpu_startup(dummy) >> =C2=A0#ifndef XEN >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 cpu_setregs(); >> =C2=A0#endif >> + >> + =C2=A0 =C2=A0 =C2=A0 /* >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0* Add BSP interrupt bitmask. >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0*/ >> + =C2=A0 =C2=A0 =C2=A0 intr_add_cpu(0); >> =C2=A0} > > I would make this a single line comment and say: > "Add BSP as an interrupt target." Please note that all the other comments in cpu_startup() is 3 lines even when single so I'd stick with that. I will change the wording as you wish though. Attilio --=20 Peace can only be achieved by understanding - A. Einstein