Re: [PATCH][RFC] x86: Fix the irq affinity in fixup_cpus

[Chen Yu]

On Tue, Jul 04, 2017 at 10:50:33AM +0200, Thomas Gleixner wrote:
> On Mon, 3 Jul 2017, Chen Yu wrote:
> > On Sun, Jun 04, 2017 at 10:04:53PM +0200, Thomas Gleixner wrote:
> > > After looking at the callsites, it's safe to change
> > > irq_set_affinity_locked() so that it uses the direct affinity setter
> > > function when force == true.
> > > 
> > Sorry it took me sometime to understand this point(this is why I did not reply
> > to you at the first time :-)
> > I thought the defination of the word 'safe' here means, we should
> > not adjust the irq affinity in the process context if the ISR is
> > still running, otherwise there might be a race condition.
> > 
> > Currently, there are four drivers would set the force flag to true(AKA,
> > invoking irq_force_affinity()).
> > 
> > 1. exynos4_mct_starting_cpu()
> >    The irq affinity is set before the clockevent is registered,
> >    so there would be no interrupt triggered when adjusting
> >    the irq affinity in the process context. Safe.
> > 
> > 2. sirfsoc_local_timer_starting_cpu()
> >    The same as above. Safe.
> > 
> > 3. arm_perf_starting_cpu()
> >    During cpu offline, the pmu interrupt(non percpu pmu interrupt)
> >    might be migrated to other online cpus. Then once the same cpu
> >    is put online, the interrupt will be set back to this cpu again
> >    by invoking irq_force_affinity(), but currently the pmu interrupt
> >    might be still running on other cpus, so it would be unsafe to adjust
> >    its irq affinity in the process context?
> 
> No, that's not an issue. The ability to move interrupts in process context,
> or better said in any context, has nothing to do with a concurrent
> interrupt. The normal mechanics for most architectures/interrupt
> controllers is just to program the new affinity setting which will take
> effect with the next delivered interrupt.
> 
> We just have these ill designed hardware implementations which do not allow
> that. They require to change the interrupt affinity at the point when the
> interrupt is handled on the original target CPU. But that's hard to achieve
> when the CPU is about to go offline, because we might wait forever for an
> interrupt to be raised. So in that case we need to forcefully move them
> away and take the risk of colliding with an actual interrupt being raised
> in hardware concurrently which has the potential to confuse the interrupt
> chip.
>
Thanks for the detailed explaination! Got it.
> > 4. sunhv_migrate_hvcons_irq()
> >    The cpu who encountered a panic needs to migrate the hvcons irq to the
> >    current alive cpu, and send ipi to stop other cpus. So at the time to
> >    adjust the irq affinity for the hvcons, the interrupt of the latter might
> >    be running and it might be unsafe to adjust the irq affinity in the
> >    process context?
> 
> None of these are related to that problem. All of these architectures can
> move interrupts in process context unconditionally. It's also not relevant
> which callsites invoke irq_set_affinity_locked() with force=true.
> 
Okay.
> The point is whether we can change the semantics of irq_set_affinity_locked()
> without breaking something.
> 
Yes, this might change the semantics of force flag.
> But please answer my other mail in that thread [1] first before we start
> about changing anything in that area. The affinity related changes are in
> Linus tree now, so please retest against that as well.
Okay, I'll switch to that thread.
Here's the test result for affinity:
# uname -r
4.12.0+
# cat /proc/irq/32/smp_affinity
00000000,80000000
# echo 0 > /sys/devices/system/cpu/cpu31/online 
# cat /proc/irq/32/smp_affinity
00000000,ffffffff
Looks like cpu31 is till included in the irq mask.

Thanks,
Yu
> 
> Thanks,
> 
> 	tglx
> 
> [1] http://lkml.kernel.org/r/alpine.DEB.2.20.1706282036330.1890@nanos
> 
>