Date: Sat, 14 Apr 2012 17:05:35 -0600 From: "Justin T. Gibbs" <gibbs@scsiguy.com> To: Marius Strobl <marius@alchemy.franken.de> Cc: Attilio Rao <attilio@FreeBSD.org>, svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, John Baldwin <jhb@FreeBSD.org> Subject: Re: svn commit: r234074 - in head/sys: amd64/amd64 i386/i386 Message-ID: <FAEF2278-3418-4CB6-9FCF-B4EF46EA0351@scsiguy.com> In-Reply-To: <20120410114118.GB93449@alchemy.franken.de> References: <201204092241.q39MfJZn081610@svn.freebsd.org> <20120409230949.GB68111@alchemy.franken.de> <CAJ-FndDzh50_Xb%2B08EnhAVBnu1vFLo0d5MX%2BQwAOTcq_ewwDJQ@mail.gmail.com> <20120410114118.GB93449@alchemy.franken.de>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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 >>>>=20 >>>> 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(). >>>>=20 >>>> ?? Reported by: ?? ?? ?? ??jh >>>> ?? Reviewed by: ?? ?? ?? ??jhb >>>> ?? MFC after: ??3 days >>>> ?? X-MFC: ?? ?? ?? ?? ?? ?? ??r233961 >>>> ?? Pointy hat to: ?? ?? ??me >>>>=20 >>>> 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 >>>>=20 >>>> Modified: head/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=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>> --- 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(); >>>>=20 >>>> ?? ?? ?? cpu_setregs(); >>>> + >>>> + ?? ?? /* >>>> + ?? ?? ??* Add BSP as an interrupt target. >>>> + ?? ?? ??*/ >>>> + ?? ?? intr_add_cpu(0); >>>> ??} >>>=20 >>> If I'm not mistaken, intr_add_cpu() is under #ifdef SMP, so it = should be >>> here as well. >>=20 >> 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 =3D 1. >> I think the attached patch should make its dirty job, opinion? >=20 > 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. >=20 > 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); -- Justin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?FAEF2278-3418-4CB6-9FCF-B4EF46EA0351>