Date: Mon, 9 Apr 2012 23:32:55 +0100 From: Attilio Rao <attilio@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, Jaakko Heinonen <jh@freebsd.org>, svn-src-all@freebsd.org, "Justin T. Gibbs" <gibbs@freebsd.org>, src-committers@freebsd.org Subject: Re: svn commit: r233961 - head/sys/x86/x86 Message-ID: <CAJ-FndD019kq94AHosmZnEeJD2Bh-LE95ySHqAR4GDOVit-fgw@mail.gmail.com> In-Reply-To: <201204091435.31893.jhb@freebsd.org> References: <201204062119.q36LJTKR026564@svn.freebsd.org> <201204091314.20775.jhb@freebsd.org> <CAJ-FndCgOs-HayqNk_pggJECkb%2B6Eas_AnnObvPn7_x4BEftqQ@mail.gmail.com> <201204091435.31893.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Il 09 aprile 2012 19:35, John Baldwin <jhb@freebsd.org> ha scritto: > On Monday, April 09, 2012 1:59:13 pm Attilio Rao wrote: >> Il 09 aprile 2012 18:14, John Baldwin <jhb@freebsd.org> ha scritto: >> > On Monday, April 09, 2012 12:58:07 pm Attilio Rao wrote: >> >> Il 09 aprile 2012 17:34, John Baldwin <jhb@freebsd.org> 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<FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PG= E,MCA,CMOV,PAT,PSE36,CLFLUSH,DTS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE> >> >> >> =C2=A0 Features2=3D0x4400<CNXT-ID,xTPR> >> >> >> real memory =C2=A0=3D 2147483648 (2048 MB) >> >> >> avail memory =3D 2085228544 (1988 MB) >> >> >> Event timer "LAPIC" quality 400 >> >> >> ACPI APIC Table: <A M I =C2=A0OEMAPIC > >> >> >> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndD019kq94AHosmZnEeJD2Bh-LE95ySHqAR4GDOVit-fgw>