Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751436AbaKXVBi (ORCPT ); Mon, 24 Nov 2014 16:01:38 -0500 Received: from mail-wi0-f173.google.com ([209.85.212.173]:50426 "EHLO mail-wi0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751144AbaKXVBg (ORCPT ); Mon, 24 Nov 2014 16:01:36 -0500 Message-ID: <54739CA8.2080406@linaro.org> Date: Mon, 24 Nov 2014 21:01:28 +0000 From: Daniel Thompson User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Thomas Gleixner CC: linaro-kernel@lists.linaro.org, Russell King , Jason Cooper , patches@linaro.org, Marc Zyngier , linux-kernel@vger.kernel.org, Daniel Drake , Dmitry Pervushin , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 3.18-rc3 v8 1/4] irqchip: gic: Make gic_raise_softirq() FIQ-safe References: <1415183260-6389-1-git-send-email-daniel.thompson@linaro.org> <1415968543-29469-1-git-send-email-daniel.thompson@linaro.org> <1415968543-29469-2-git-send-email-daniel.thompson@linaro.org> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 24/11/14 20:38, Thomas Gleixner wrote: > On Mon, 24 Nov 2014, Thomas Gleixner wrote: >> On Mon, 24 Nov 2014, Thomas Gleixner wrote: >>> On Fri, 14 Nov 2014, Daniel Thompson wrote: >>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>> index 38493ff28fa5..0db62a6f1ee3 100644 >>>> --- a/drivers/irqchip/irq-gic.c >>>> +++ b/drivers/irqchip/irq-gic.c >>>> @@ -73,6 +73,13 @@ struct gic_chip_data { >>>> static DEFINE_RAW_SPINLOCK(irq_controller_lock); >>>> >>>> /* >>>> + * This lock may be locked for reading by FIQ handlers. Thus although >>>> + * read locking may be used liberally, write locking must only take >>>> + * place only when local FIQ handling is disabled. >>>> + */ >>>> +static DEFINE_RWLOCK(fiq_safe_cpu_map_lock); >>>> + >>>> +/* >>>> * The GIC mapping of CPU interfaces does not necessarily match >>>> * the logical CPU numbering. Let's use a mapping as returned >>>> * by the GIC itself. >>>> @@ -624,7 +631,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) >>>> int cpu; >>>> unsigned long flags, map = 0; >>>> >>>> - raw_spin_lock_irqsave(&irq_controller_lock, flags); >>>> + read_lock_irqsave(&fiq_safe_cpu_map_lock, flags); >>> >>> Just for the record: >>> >>> You might have noticed that you replace a raw lock with a non raw >>> one. That's not an issue on mainline, but that pretty much renders >>> that code broken for RT. >>> >>> Surely nothing I worry too much about given the current state of RT. >> >> And having a second thought here. Looking at the protection scope >> independent of the spin vs. rw lock >> >> gic_raise_softirq() >> >> lock(); >> >> /* Does not need any protection */ >> for_each_cpu(cpu, mask) >> map |= gic_cpu_map[cpu]; >> >> /* >> * Can be outside the lock region as well as it makes sure >> * that previous writes (usually the IPI data) are visible >> * before the write to the SOFTINT register. >> */ >> dmb(ishst); >> >> /* Why needs this protection? */ >> write(map, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT)); >> >> unlock(); >> >> gic_migrate_target() >> >> .... >> lock(); >> >> /* Migrate all peripheral interrupts */ >> >> unlock(); >> >> So what's the point of that protection? >> >> gic_raise_softirq() is used to send IPIs, which are PPIs on the target >> CPUs so they are not affected from the migration of the peripheral >> interrupts at all. >> >> The write to the SOFTINT register in gic_migrate_target() is not >> inside the lock region. So what's serialized by the lock in >> gic_raise_softirq() at all? >> >> Either I'm missing something really important here or this locking >> exercise in gic_raise_softirq() and therefor the rwlock conversion is >> completely pointless. > > Thanks to Marc I figured it out now what I'm missing. That stuff is > part of the bl switcher horror. Well documented as all of that ... > > So the lock protects against an IPI being sent to the current cpu > while the target map is redirected and the pending state of the > current cpu is migrated to another cpu. > > It's not your fault, that the initial authors of that just abused > irq_controller_lock for that purpose instead of introducing a seperate > lock with a clear description of the protection scope in the first > place. > > Now you came up with the rw lock to handle the following FIQ related > case: > gic_raise_softirq() > lock(x); > ---> FIQ > handle_fiq() > gic_raise_softirq() > lock(x); <-- Live lock > > Now the rwlock lets you avoid that, and it only lets you avoid that > because rwlocks are not fair. > > So while I cannot come up with a brilliant replacement, it would be > really helpful documentation wise if you could do the following: > > 1) Create a patch which introduces irq_migration_lock as a raw > spinlock and replaces the usage of irq_controller_lock in > gic_raise_softirq() and gic_migrate_target() along with a proper > explanation in the code and the changelog of course. Replace irq_controller_lock or augment it with a new one? irq_raise_softirq() can share a single r/w lock with irq_set_affinity() because irq_set_affinity() would have to lock it for writing and that would bring the deadlock back for a badly timed FIQ. Thus if we want calls to gic_raise_softirq() to be FIQ-safe there there must be two locks taken in gic_migrate_target(). We can eliminate irq_controller_lock but we cannot replace it with one r/w lock. > 2) Make the rwlock conversion on top of that with a proper > documentation in the code of the only relevant reason (See above). > > The protection scope which prevents IPIs being sent while switching > over is still the same and not affected. > > That's not the first time that I stumble over this bl switcher mess > which got boltet into the kernel mindlessly. > > If the scope of the issue would have been clear up front, I wouldn't > have complained about the RT relevance for this as it is simple to > either disable FIQs for RT or just handle the above case differently. > > Thanks, > > tglx > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/