Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 9 Apr 2012 14:35:31 -0400
From:      John Baldwin <jhb@freebsd.org>
To:        Attilio Rao <attilio@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:  <201204091435.31893.jhb@freebsd.org>
In-Reply-To: <CAJ-FndCgOs-HayqNk_pggJECkb%2B6Eas_AnnObvPn7_x4BEftqQ@mail.gmail.com>
References:  <201204062119.q36LJTKR026564@svn.freebsd.org> <201204091314.20775.jhb@freebsd.org> <CAJ-FndCgOs-HayqNk_pggJECkb%2B6Eas_AnnObvPn7_x4BEftqQ@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
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:
> >> >> >   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."

(More closely matches the original comment from intr_machdep.c).

-- 
John Baldwin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201204091435.31893.jhb>