Il 09 aprile 2012 19:35, John Baldwin <j...@freebsd.org> ha scritto: > On Monday, April 09, 2012 1:59:13 pm Attilio Rao wrote: >> Il 09 aprile 2012 18:14, John Baldwin <j...@freebsd.org> ha scritto: >> > On Monday, April 09, 2012 12:58:07 pm Attilio Rao wrote: >> >> Il 09 aprile 2012 17:34, John Baldwin <j...@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: >> >> >> > 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<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> >> >> >> Features2=0x4400<CNXT-ID,xTPR> >> >> >> real memory = 2147483648 (2048 MB) >> >> >> avail memory = 2085228544 (1988 MB) >> >> >> Event timer "LAPIC" quality 400 >> >> >> ACPI APIC Table: <A M I OEMAPIC > >> >> >> 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."
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 -- Peace can only be achieved by understanding - A. Einstein _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
