Re: [PATCH 8/8] x86_64: Support for new UV apic

[Jack Steiner]

On Fri, Mar 28, 2008 at 09:45:33PM +0100, Ingo Molnar wrote:
> 
> * Jack Steiner <steiner@xxxxxxx> wrote:
> 
> > On Fri, Mar 28, 2008 at 09:15:32PM +0100, Ingo Molnar wrote:
> > > 
> > > * Jack Steiner <steiner@xxxxxxx> wrote:
> > > 
> > > > Index: linux/arch/x86/kernel/apic_64.c
> > > > ===================================================================
> > > > --- linux.orig/arch/x86/kernel/apic_64.c	2008-03-28 13:00:22.000000000 -0500
> > > > +++ linux/arch/x86/kernel/apic_64.c	2008-03-28 13:06:12.000000000 -0500
> > > > @@ -738,6 +738,7 @@ void __cpuinit setup_local_APIC(void)
> > > >  	unsigned int value;
> > > >  	int i, j;
> > > >  
> > > > +	preempt_disable();
> > > >  	value = apic_read(APIC_LVR);
> > > >  
> > > >  	BUILD_BUG_ON((SPURIOUS_APIC_VECTOR & 0x0f) != 0x0f);
> > > > @@ -831,6 +832,7 @@ void __cpuinit setup_local_APIC(void)
> > > >  	else
> > > >  		value = APIC_DM_NMI | APIC_LVT_MASKED;
> > > >  	apic_write(APIC_LVT1, value);
> > > > +	preempt_enable();
> > > >  }
> > > 
> > > hm, this looks a bit weird - why are all the preempt-disable/enable 
> > > calls needed?
> > 
> > The first patch had a preempt disable/enable in the function that 
> > reads apicid (see read_apic_id() in arch/x86/kernel/genapic_64.c). 
> > This seemed necessary since large system generate an apicid by reading 
> > the live id & concatenating it with extra bits.
> > 
> > One of the review comments suggested that I change the preempt to a 
> > WARN() since reading apic_id really should be done with preemtion 
> > disabled. The added code eliminates the warnings.
> 
> could you post the stacktraces of the warnings as they occured? All 
> those codepaths should be running with preemption disabled in some 
> natural way.
> 
> 	Ingo

Here is one of the pathes. I have not been able to reproduce the 
other (smp_sanity_check()).  It's possible there was cleanup that
eliminated the message, or I might have forgotten the receipe. I
know it was early in boot:



WARNING: at arch/x86/kernel/genapic_64.c:97 read_apic_id+0x30/0x6a()
Modules linked in:
Pid: 1, comm: swapper Not tainted 2.6.25-rc7-x86-latest.git-00829-gfb5f592-dirty #15

Call Trace:
 [<ffffffff80231e69>] warn_on_slowpath+0x51/0x63
 [<ffffffff80578d37>] _spin_lock+0xe/0x24
 [<ffffffff8022b289>] task_rq_lock+0x3d/0x6f
 [<ffffffff80579133>] _spin_unlock_irqrestore+0x12/0x2c
 [<ffffffff8022d7fd>] set_cpus_allowed+0xd9/0xe6
 [<ffffffff8057319c>] set_cpu_sibling_map+0x2f4/0x30c
 [<ffffffff8021f434>] read_apic_id+0x30/0x6a
 [<ffffffff807cf898>] native_smp_prepare_cpus+0xc4/0x2d4
 [<ffffffff807c580f>] kernel_init+0x4e/0x2cb
 [<ffffffff8020bee8>] child_rip+0xa/0x12


--- jack
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/