svn commit: r234074 - in head/sys: amd64/amd64 i386/i386
John Baldwin
jhb at freebsd.org
Mon Apr 16 14:18:03 UTC 2012
On Saturday, April 14, 2012 7:05:35 pm Justin T. Gibbs wrote:
>
> On Apr 10, 2012, at 5:41 AM, Marius Strobl wrote:
>
> > 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.
> >
> > Marius
>
> While functionally correct, I believe that wrapping intr_add_cpu()
> in machdep.c in "SMP ifdefs" is inferior to calling it in all cases.
> It invites questions like, "In the UP case, don't we have to ensure
> that CPU0 is a valid interrupt target?" This is because casual
> visitors to this file don't know that intr_add_cpu() only impacts
> interrupt distribution. Of course, this is just an artifact of the
> current implementation. #ifdefs should be as close to the implemenation
> as possible. This simplifies the task of making future enhancments.
> This is why I'd prefer to see these within the body of intr_add_cpu()
> than where they are now.
>
> I also think the comment could be improved to be something like:
>
> /*
> * The BSP/CPU0 is always an interrupt target even if
> * our probe of MP hardware fails or MP mode is disabled.
> */
> intr_add_cpu(0);
This is why my original patch had this all self-contained inside the
#ifdef SMP in sys/x86/x86/intr_machdep.c via a new SYSINIT().
--
John Baldwin
More information about the svn-src-all
mailing list