From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751547AbaHQTEv (ORCPT ); Sun, 17 Aug 2014 15:04:51 -0400 Received: from mho-03-ewr.mailhop.org ([204.13.248.66]:33659 "EHLO mho-01-ewr.mailhop.org" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751398AbaHQTEt (ORCPT ); Sun, 17 Aug 2014 15:04:49 -0400 X-Mail-Handler: Dyn Standard SMTP by Dyn X-Originating-IP: 96.249.243.124 X-Report-Abuse-To: abuse@dyndns.com (see http://www.dyndns.com/services/sendlabs/outbound_abuse.html for abuse reporting information) X-MHO-User: U2FsdGVkX1/I+932t8WDmLiDVQ7qS+JIDqvXNzvk9uU= X-DKIM: OpenDKIM Filter v2.0.1 titan 0B9AD5C813C Date: Sun, 17 Aug 2014 15:04:34 -0400 From: Jason Cooper To: Russell King - ARM Linux Cc: Stephen Boyd , linux-arm-msm@vger.kernel.org, Thomas Gleixner , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Nicolas Pitre Subject: Re: [PATCH v4] irqchip: gic: Allow gic_arch_extn hooks to call into scheduler Message-ID: <20140817190434.GJ12769@titan.lakedaemon.net> References: <1407938238-21413-1-git-send-email-sboyd@codeaurora.org> <20140817173236.GG12769@titan.lakedaemon.net> <20140817185522.GX30401@n2100.arm.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140817185522.GX30401@n2100.arm.linux.org.uk> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Russell, On Sun, Aug 17, 2014 at 07:55:23PM +0100, Russell King - ARM Linux wrote: > On Sun, Aug 17, 2014 at 01:32:36PM -0400, Jason Cooper wrote: > > On Wed, Aug 13, 2014 at 06:57:18AM -0700, Stephen Boyd wrote: > > > Commit 1a6b69b6548c (ARM: gic: add CPU migration support, > > > 2012-04-12) introduced an acquisition of the irq_controller_lock > > > in gic_raise_softirq() which can lead to a spinlock recursion if > > > the gic_arch_extn hooks call into the scheduler (via complete() > > > or wake_up(), etc.). This happens because gic_arch_extn hooks are > > > normally called with the irq_controller_lock held and calling > > > into the scheduler may cause us to call smp_send_reschedule() > > > which will grab the irq_controller_lock again. Here's an example > > > from a vendor kernel (note that the gic_arch_extn hook code here > > > isn't actually in mainline): > > > > > > BUG: spinlock recursion on CPU#0, swapper/0/1 > > > lock: irq_controller_lock+0x0/0x18, .magic: dead4ead, .owner: sw > > > er_cpu: 0 > > > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.10-00430-g3d433c4e > > > > > > Call trace: > > > [] dump_backtrace+0x0/0x140 > > > [] show_stack+0x10/0x1c > > > [] dump_stack+0x74/0xc4 > > > [] spin_dump+0x78/0x88 > > > [] spin_bug+0x24/0x34 > > > [] do_raw_spin_lock+0x58/0x148 > > > [] _raw_spin_lock_irqsave+0x24/0x38 > > > [] gic_raise_softirq+0x2c/0xbc > > > [] smp_send_reschedule+0x34/0x40 > > > [] try_to_wake_up+0x224/0x288 > > > [] default_wake_function+0xc/0x18 > > > [] __wake_up_common+0x50/0x8c > > > [] __wake_up_locked+0x10/0x1c > > > [] complete+0x3c/0x5c > > > [] msm_mpm_enable_irq_exclusive+0x1b8/0x1c8 > > > [] __msm_mpm_enable_irq+0x4c/0x7c > > > [] msm_mpm_enable_irq+0xc/0x18 > > > [] gic_unmask_irq+0x40/0x7c > > > [] irq_enable+0x2c/0x48 > > > [] irq_startup+0x4c/0x74 > > > [] __setup_irq+0x264/0x3f0 > > > [] request_threaded_irq+0xcc/0x11c > > > [] devm_request_threaded_irq+0x68/0xb4 > > > [] msm_iommu_ctx_probe+0x124/0x2d4 > > > [] platform_drv_probe+0x20/0x54 > > > [] driver_probe_device+0x158/0x340 > > > [] __driver_attach+0x60/0x90 > > > [] bus_for_each_dev+0x6c/0x8c > > > [] driver_attach+0x1c/0x28 > > > [] bus_add_driver+0x120/0x204 > > > [] driver_register+0xbc/0x10c > > > [] __platform_driver_register+0x5c/0x68 > > > [] msm_iommu_driver_init+0x54/0x7c > > > [] do_one_initcall+0xa4/0x130 > > > [] kernel_init_freeable+0x138/0x1dc > > > [] kernel_init+0xc/0xd4 > > > > > > We really just want to synchronize the sending of an SGI with the > > > update of the gic_cpu_map[], so introduce a new SGI lock that we > > > can use to synchronize the two code paths. Three main events are > > > happening that we have to consider: > > > > > > 1. We're updating the gic_cpu_mask to point to an > > > incoming CPU > > > > > > 2. We're (potentially) sending an SGI to the outgoing CPU > > > > > > 3. We're redirecting any pending SGIs for the outgoing > > > CPU to the incoming CPU. > > > > > > Events 1 and 3 are already ordered within the same CPU by means > > > of program order and use of I/O accessors. Events 1 and 2 don't > > > need to be ordered, but events 2 and 3 do because any SGIs for > > > the outgoing CPU need to be pending before we can redirect them. > > > Synchronize by acquiring a new lock around event 2 and before > > > event 3. Use smp_mb__after_unlock_lock() before event 3 to ensure > > > that event 1 is seen before event 3 on other CPUs that may be > > > executing event 2. We put this all behind the b.L switcher config > > > option so that if we're not using this feature we don't have to > > > acquire any locks at all in the IPI path. > > > > > > Cc: Nicolas Pitre > > > Signed-off-by: Stephen Boyd > > > --- > > > drivers/irqchip/irq-gic.c | 23 +++++++++++++++++++++-- > > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > Applied to irqchip/urgent with Nico's Ack. > > Interesting, so I'm discussing this patch, and it gets applied anyway... > yes, that's great. Quoting Nico: "Of course it would be good to clarify things wrt Russell's remark independently from this patch." I took 'independently' to mean "This patch is ok, *and* we need to address Russell's concerns in a follow-up patch." Nico's Reviewed-by with that comment was sent August 13th. The most recent activity on this thread was also August 13th. After four days, I reasoned there were no objections to his comment. thx, Jason.