svn commit: r234074 - in head/sys: amd64/amd64 i386/i386

[Marius Strobl]

On Tue, Apr 10, 2012 at 01:55:31PM +0100, Attilio Rao wrote:
> Il 10 aprile 2012 12:41, Marius Strobl <marius at alchemy.franken.de> ha scritto:
> > On Tue, Apr 10, 2012 at 01:03:56AM +0100, Attilio Rao wrote:
> >> Il 10 aprile 2012 00:09, Marius Strobl <marius at alchemy.franken.de> ha scritto:
> >> > On Mon, Apr 09, 2012 at 10:41:19PM +0000, Attilio Rao wrote:
> >> >> Author: attilio
> >> >> Date: Mon Apr ??9 22:41:19 2012
> >> >> New Revision: 234074
> >> >> URL: http://svn.freebsd.org/changeset/base/234074
> >> >>
> >> >> Log:
> >> >> ?? BSP is not added to the mask of valid target CPUs for interrupts
> >> >> ?? in set_apic_interrupt_ids(). Besides, set_apic_interrupts_ids() is not
> >> >> ?? called in the !SMP case too.
> >> >> ?? Fix this by:
> >> >> ?? - Adding the BSP as an interrupt target directly in cpu_startup().
> >> >> ?? - Remove an obsolete optimization where the BSP are skipped in
> >> >> ?? ?? set_apic_interrupt_ids().
> >> >>
> >> >> ?? Reported by: ?? ?? ?? ??jh
> >> >> ?? Reviewed by: ?? ?? ?? ??jhb
> >> >> ?? MFC after: ??3 days
> >> >> ?? X-MFC: ?? ?? ?? ?? ?? ?? ??r233961
> >> >> ?? Pointy hat to: ?? ?? ??me
> >> >>
> >> >> Modified:
> >> >> ?? head/sys/amd64/amd64/machdep.c
> >> >> ?? head/sys/amd64/amd64/mp_machdep.c
> >> >> ?? head/sys/i386/i386/machdep.c
> >> >> ?? head/sys/i386/i386/mp_machdep.c
> >> >>
> >> >> Modified: head/sys/amd64/amd64/machdep.c
> >> >> ==============================================================================
> >> >> --- head/sys/amd64/amd64/machdep.c ?? ??Mon Apr ??9 22:01:43 2012 ?? ?? ?? ??(r234073)
> >> >> +++ head/sys/amd64/amd64/machdep.c ?? ??Mon Apr ??9 22:41:19 2012 ?? ?? ?? ??(r234074)
> >> >> @@ -295,6 +295,11 @@ cpu_startup(dummy)
> >> >> ?? ?? ?? vm_pager_bufferinit();
> >> >>
> >> >> ?? ?? ?? cpu_setregs();
> >> >> +
> >> >> + ?? ?? /*
> >> >> + ?? ?? ??* Add BSP as an interrupt target.
> >> >> + ?? ?? ??*/
> >> >> + ?? ?? intr_add_cpu(0);
> >> >> ??}
> >> >
> >> > If I'm not mistaken, intr_add_cpu() is under #ifdef SMP, so it should be
> >> > here as well.
> >>
> >> You are right, sorry, I did forgot to test without SMP.
> >> I think we still need intr_add_cpu() on cpu_startup() because of the
> >> case smp_disabled = 1.
> >> I think the attached patch should make its dirty job, opinion?
> >
> > I currently fail to see why the latter approach would be necessary,
> > i.e. IMO wrapping the intr_add_cpu() calls in cpu_startup() should
> > be sufficient. In case the kernel is compiled without SMP support,
> > interrupt balancing support isn't available in the first place and
> > the BSP is always the only available target (see the UP version of
> > intr_next_cpu() at the end of x86/x86/intr_machdep.c), so there's
> > no need to add the BSP as a valid target. If an SMP kernel is run
> > on a UP machine or with SMP disabled, interrupt balancing support
> > is available but the intr_add_cpu() calls in cpu_startup() will add
> > the BSP as (the only) target, so everything should be fine. Maybe
> > you can elaborate on why you think an SMP kernel with SMP disabled
> > needs special handling.
> 
> I do not understand what you mean.

Well, so we are at least in the same boat :)

> Right now there is a compile time issue where for !SMP kernel it won't
> compile and this is what I'm trying to fix now, so I don't understand
> what do you mean here.
> 

AFAICT the below patch should take care of UP in both the compile-time
and run-time cases.

Marius

Index: amd64/amd64/machdep.c
===================================================================
--- amd64/amd64/machdep.c	(revision 234095)
+++ amd64/amd64/machdep.c	(working copy)
@@ -296,10 +296,12 @@ cpu_startup(dummy)
 
 	cpu_setregs();
 
+#ifdef SMP
 	/*
 	 * Add BSP as an interrupt target.
 	 */
 	intr_add_cpu(0);
+#endif
 }
 
 /*
Index: i386/i386/machdep.c
===================================================================
--- i386/i386/machdep.c	(revision 234095)
+++ i386/i386/machdep.c	(working copy)
@@ -337,10 +337,12 @@ cpu_startup(dummy)
 	cpu_setregs();
 #endif
 
+#ifdef SMP
 	/*
 	 * Add BSP as an interrupt target.
 	 */
 	intr_add_cpu(0);
+#endif
 }
 
 /*