From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752376AbdGEDUh (ORCPT ); Tue, 4 Jul 2017 23:20:37 -0400 Received: from mga11.intel.com ([192.55.52.93]:18761 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752285AbdGEDUg (ORCPT ); Tue, 4 Jul 2017 23:20:36 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,310,1496127600"; d="scan'208";a="988763185" Date: Wed, 5 Jul 2017 11:20:52 +0800 From: Chen Yu To: Thomas Gleixner Cc: Ingo Molnar , "H. Peter Anvin" , Borislav Petkov , Len Brown , "Rafael J . Wysocki" , x86@kernel.org, LKML Subject: Re: [PATCH][RFC] x86: Fix the irq affinity in fixup_cpus Message-ID: <20170705032052.GA10286@yu-desktop-1.sh.intel.com> References: <1493020011-18536-1-git-send-email-yu.c.chen@intel.com> <20170703135109.GA26935@yu-desktop-1.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >