On 08/21/14 02:47, Russell King - ARM Linux wrote:
> What would make more sense is if this were a read-write lock, then
> gic_raise_softirq() could run concurrently on several CPUs without
> interfering with each other, yet still be safe with gic_migrate_target().
>
> I'd then argue that we wouldn't need the ifdeffery, we might as well
> keep the locking in place - it's overhead is likely small (when lockdep
> is disabled) when compared to everything else which goes on in this
> path.
Ok.
>> @@ -690,6 +700,7 @@ void gic_migrate_target(unsigned int new_cpu_id)
>> ror_val = (cur_cpu_id - new_cpu_id) & 31;
>>
>> raw_spin_lock(&irq_controller_lock);
>> + raw_spin_lock(&gic_sgi_lock);
>>
>> /* Update the target interface for this logical CPU */
>> gic_cpu_map[cpu] = 1 << new_cpu_id;
>> @@ -709,6 +720,7 @@ void gic_migrate_target(unsigned int new_cpu_id)
>> }
>> }
>>
>> + raw_spin_unlock(&gic_sgi_lock);
>> raw_spin_unlock(&irq_controller_lock);
> I would actually suggest we go a bit further. Use gic_sgi_lock to only
> lock gic_cpu_map[] itself, and not have it beneath any other lock.
> That's an advantage because right now, lockdep learns from the above that
> there's a dependency between irq_controller_lock and gic_sgi_lock.
> Reasonably keeping lock dependencies to a minimum is always a good idea.
>
> The places where gic_cpu_map[] is used are:
>
> static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
> bool force)
> {
> ...
> raw_spin_lock(&irq_controller_lock);
> mask = 0xff << shift;
> bit = gic_cpu_map[cpu] << shift;
> val = readl_relaxed(reg) & ~mask;
> writel_relaxed(val | bit, reg);
> raw_spin_unlock(&irq_controller_lock);
>
> So, we can move:
>
> mask = 0xff << shift;
> bit = gic_cpu_map[cpu] << shift;
>
> out from under irq_controller_lock and put it under gic_sgi_lock. The
> "mask" bit doesn't need to be under any lock at all.
>
> There's gic_cpu_init():
>
> cpu_mask = gic_get_cpumask(gic);
> gic_cpu_map[cpu] = cpu_mask;
>
> /*
> * Clear our mask from the other map entries in case they're
> * still undefined.
> */
> for (i = 0; i < NR_GIC_CPU_IF; i++)
> if (i != cpu)
> gic_cpu_map[i] &= ~cpu_mask;
>
> which better had be stable after boot - if not, this needs locking.
> Remember that the above will be called on hotplug too.
>
Perhaps you'd like to send this patch? It isn't clear to me from your
description how this would work. What happens if we update the
gic_cpu_map between the time we read the map and acquire the
irq_controller_lock in gic_set_affinity()? I think we would program the
affinity for the wrong CPU?
Either way, here's the patch I think you described.
----8<-----
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e606fe8..d159590461c7 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -39,6 +39,7 @@
#include <linux/slab.h>
#include <linux/irqchip/chained_irq.h>
#include <linux/irqchip/arm-gic.h>
+#include <linux/rwlock.h>
#include <asm/cputype.h>
#include <asm/irq.h>
@@ -79,6 +80,7 @@ static DEFINE_RAW_SPINLOCK(irq_controller_lock);
*/
#define NR_GIC_CPU_IF 8
static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly;
+static DEFINE_RWLOCK(gic_cpu_map_lock);
/*
* Supported arch specific GIC irq extension.
@@ -233,9 +235,13 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids)
return -EINVAL;
- raw_spin_lock(&irq_controller_lock);
mask = 0xff << shift;
+
+ read_lock(&gic_cpu_map_lock);
bit = gic_cpu_map[cpu] << shift;
+ read_unlock(&gic_cpu_map_lock);
+
+ raw_spin_lock(&irq_controller_lock);
val = readl_relaxed(reg) & ~mask;
writel_relaxed(val | bit, reg);
raw_spin_unlock(&irq_controller_lock);
@@ -605,7 +611,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(&gic_cpu_map_lock, flags);
/* Convert our logical CPU mask into a physical one. */
for_each_cpu(cpu, mask)
@@ -620,7 +626,7 @@ static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
/* this always happens on GIC0 */
writel_relaxed(map << 16 | irq, gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT);
- raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
+ read_unlock_irqrestore(&gic_cpu_map_lock, flags);
}
#endif
@@ -689,11 +695,12 @@ void gic_migrate_target(unsigned int new_cpu_id)
cur_target_mask = 0x01010101 << cur_cpu_id;
ror_val = (cur_cpu_id - new_cpu_id) & 31;
- raw_spin_lock(&irq_controller_lock);
-
+ write_lock(&gic_cpu_map_lock);
/* Update the target interface for this logical CPU */
gic_cpu_map[cpu] = 1 << new_cpu_id;
+ write_unlock(&gic_cpu_map_lock);
+ raw_spin_lock(&irq_controller_lock);
/*
* Find all the peripheral interrupts targetting the current
* CPU interface and migrate them to the new CPU interface.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
