Messages in this thread | | | Date | Wed, 5 Jul 2017 11:20:52 +0800 | From | Chen Yu <> | Subject | Re: [PATCH][RFC] x86: Fix the irq affinity in fixup_cpus |
| |
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 > >
| |