RE: [PATCH 1/8] x86, apic.c: Disable irq0 if CPU enables ARAT forlocal apic timer

[Thomas Gleixner]

On Wed, 5 Oct 2011, Yu, Fenghua wrote:

Please remove Zwane Mwaikambo <zwane@xxxxxxxxxxxxxxxx> from the cc
list, that address does not exist anymore.

> > 
> > > From: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > >
> > > irq0 won't generate any interrupt after local apic timers is enabled
> > and ARAT
> > > is enabled. Disable irq0 in this case. Thus irq0 won't block BSP
> > offline.
> > 
> > Why would it do so ?

> Irq0 is set as IRQF_NOBALANCING. And it's not used any more after
> boot time if CPU using local apic timer supports ARAT. Although irq0
> is useless, it blocks CPU0 offline.

Please use proper line breaks around 78

> That's why we need to treat irq0 specially for CPU0 offline.

That's utter nonsense. Your whole approach is broken. irq0 is not
special at all and any other interrupt could be marked
IRQF_NOBALANCING as well.

Special casing stuff is always a sign of bandaids and your whole patch
set is just a big cobbled together duct tape thing.
 
> > > +
> > > +	/* irq0 won't be used any more if CPU supports ARAT feature. */
> > > +	if (cpu == 0 && this_cpu_has(X86_FEATURE_ARAT))
> > > +		disable_irq(0);
> > 
> > This is completely wrong. If we want to shut that interrupt down, then
> > we do it in the clockevents set mode functions of PIT or HPET and not
> > at some random place in the apic timer code.
> 
> Agree with you.
> 
> Or my original irq0 handling is just ignoring irq0 when ARAT is
> enabled during CPU0 offline procedure. Is this way cleaner and
> limited to CPU0 offline path? I'm afraid shutting that interrupt
> down or disabling it may cause any other (legacy) issue?

That's still wrong and your offline check code is broken beyond repair
anyway.

Thanks,

	tglx
--
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/