Date: Mon, 9 Apr 2012 18:59:13 +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-FndCgOs-HayqNk_pggJECkb%2B6Eas_AnnObvPn7_x4BEftqQ@mail.gmail.com> In-Reply-To: <201204091314.20775.jhb@freebsd.org> References: <201204062119.q36LJTKR026564@svn.freebsd.org> <201204091234.43106.jhb@freebsd.org> <CAJ-FndB_oWjd3VsJ%2BD9p3987MdfCXuAUHXX-mkjLmmRnnTireQ@mail.gmail.com> <201204091314.20775.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 revis=
ion
>> >> > =C2=A0 222813, that left all un-pinned interrupts assigned to CPU 0=
.
>> >> >
>> >> > =C2=A0 sys/x86/x86/intr_machdep.c:
>> >> > =C2=A0 =C2=A0 In intr_shuffle_irqs(), remove CPU_SETOF() call that =
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 res=
ults of calls
>> >> > =C2=A0 =C2=A0 the intr_add_cpu() that occur much earlier in the boo=
t process.
>> >> > =C2=A0 =C2=A0 Since "intr_cpus" is statically initialized to the em=
pty set, and
>> >> > =C2=A0 =C2=A0 all processors, including the BSP, already add themse=
lves 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 la=
st
>> >> lines from a verbose boot:
>> >>
>> >> SMP: AP CPU #1 Launched!
>> >> cpu1 AP:
>> >> =C2=A0 =C2=A0 =C2=A0ID: 0x01000000 =C2=A0 VER: 0x00050014 LDR: 0x0000=
0000 DFR: 0xffffffff
>> >> =C2=A0 lint0: 0x00010700 lint1: 0x00000400 TPR: 0x00000000 SVR: 0x000=
001ff
>> >> =C2=A0 timer: 0x000100ef therm: 0x00010000 err: 0x000000f0 pmc: 0x000=
10400
>> >>
>> >> 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=
,PGE,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 think A=
ttilio 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=A0T=
his used to
>> > work because bit 0 was assigned statically. =C2=A0Also, in a UP machin=
e
>> > 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 the s=
ysinit 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 case c=
heck
> 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 wh=
ich is
> why that would have been harmful.
I'm not sure I follow, a separate count for what?
So do you consider the following patch as a real commit candidate?
Thanks,
Attilio
Index: sys/i386/i386/mp_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/mp_machdep.c (revisione 234064)
+++ sys/i386/i386/mp_machdep.c (copia locale)
@@ -819,8 +819,6 @@ init_secondary(void)
* We tell the I/O APIC code about all the CPUs we want to receive
* interrupts. If we don't want certain CPUs to receive IRQs we
* can simply not tell the I/O APIC code about them in this function.
- * We also do not tell it about the BSP since it tells itself about
- * the BSP internally to work with UP kernels and on UP machines.
*/
static void
set_interrupt_apic_ids(void)
@@ -831,8 +829,6 @@ set_interrupt_apic_ids(void)
apic_id =3D cpu_apic_ids[i];
if (apic_id =3D=3D -1)
continue;
- if (cpu_info[apic_id].cpu_bsp)
- continue;
if (cpu_info[apic_id].cpu_disabled)
continue;
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 (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);
}
/*
Index: sys/amd64/amd64/mp_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/amd64/amd64/mp_machdep.c (revisione 234064)
+++ sys/amd64/amd64/mp_machdep.c (copia locale)
@@ -785,8 +785,6 @@ init_secondary(void)
* We tell the I/O APIC code about all the CPUs we want to receive
* interrupts. If we don't want certain CPUs to receive IRQs we
* can simply not tell the I/O APIC code about them in this function.
- * We also do not tell it about the BSP since it tells itself about
- * the BSP internally to work with UP kernels and on UP machines.
*/
static void
set_interrupt_apic_ids(void)
@@ -797,8 +795,6 @@ set_interrupt_apic_ids(void)
apic_id =3D cpu_apic_ids[i];
if (apic_id =3D=3D -1)
continue;
- if (cpu_info[apic_id].cpu_bsp)
- continue;
if (cpu_info[apic_id].cpu_disabled)
continue;
Index: sys/amd64/amd64/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/amd64/amd64/machdep.c (revisione 234064)
+++ sys/amd64/amd64/machdep.c (copia locale)
@@ -295,6 +295,11 @@ cpu_startup(dummy)
vm_pager_bufferinit();
cpu_setregs();
+
+ /*
+ * Add BSP interrupt bitmask.
+ */
+ intr_add_cpu(0);
}
/*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndCgOs-HayqNk_pggJECkb%2B6Eas_AnnObvPn7_x4BEftqQ>