arm: gic: fix gic_set_affinity

Commit Message

Ming Lei Aug. 31, 2011, 2:48 p.m. UTC

From: Ming Lei <[e-mail]>

Current gic_set_affinity doesn't support to route irq to all cpu,
so fix it.

Also remove the unnecessary check for 'cpu' since cpu_online_mask
is already ADDed to produce correct one.

Signed-off-by: Ming Lei <[e-mail]>
---
 arch/arm/common/gic.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

Comments

Russell King - ARM Linux Aug. 31, 2011, 7:04 p.m. UTC | #1

On Wed, Aug 31, 2011 at 10:48:06PM +0800, [e-mail] wrote:
> Current gic_set_affinity doesn't support to route irq to all cpu,
> so fix it.

That is correct - we don't support routing an IRQ to _all_ CPUs because
that's silly - by doing so you end up with a galloping hurd problem.  As
soon as such an interrupt is triggered, it will be delivered to all CPUs,
and all CPUs will be woken up.  All CPUs will then enter the kernel, and
one will win the spinlock race.  The remainder will spin on the lock while
the winner sorts out what to do.

That is inefficient and needlessly causes other CPUs to wake up.  The
point of the current code is that we route IRQs to exactly one CPU at a
time to avoid that problem.

> Also remove the unnecessary check for 'cpu' since cpu_online_mask
> is already ADDed to produce correct one.

Not everywhere - take a look at migrate_one_irq() in arch/arm/kernel/irq.c,
where we want to change the current routing but without actually changing
the user set affinity.

Ming Lei Sept. 1, 2011, 5:10 a.m. UTC | #2

Hi,

On Thu, Sep 1, 2011 at 3:04 AM, Russell King - ARM Linux
<[e-mail]> wrote:
> On Wed, Aug 31, 2011 at 10:48:06PM +0800, [e-mail] wrote:
>> Current gic_set_affinity doesn't support to route irq to all cpu,
>> so fix it.
>
> That is correct - we don't support routing an IRQ to _all_ CPUs because
> that's silly - by doing so you end up with a galloping hurd problem.  As
> soon as such an interrupt is triggered, it will be delivered to all CPUs,
> and all CPUs will be woken up.  All CPUs will then enter the kernel, and

Yes, the irq will make 'WFI' exit from all CPUs, which will be wake up, but
the irq is only routed into one of CPUs for irq handling. Looks like WFI is
stupid designed, :-) Buf for the CPU waken up by irq handled on other
CPU, it will run wfi and sleep immediately.

> one will win the spinlock race.  The remainder will spin on the lock while
> the winner sorts out what to do.

I have added a trace point in the entry of arch_irq_handler_default, and
observed one irq is only routed into one of CPUs, so only one CPU will
handle the irq even if gic is set to route an IRQ to _all_ CPUs.

>
> That is inefficient and needlessly causes other CPUs to wake up.  The
> point of the current code is that we route IRQs to exactly one CPU at a
> time to avoid that problem.

Maybe the current implementation should allow user to route IRQ to all
CPUs, and no cause to just forbid it, but the default_smp_affinity can be
set as only routing to one of CPUs.

>
>> Also remove the unnecessary check for 'cpu' since cpu_online_mask
>> is already ADDed to produce correct one.
>
> Not everywhere - take a look at migrate_one_irq() in arch/arm/kernel/irq.c,
> where we want to change the current routing but without actually changing
> the user set affinity.
>


thanks,

Patch

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 3227ca9..20890f4 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -173,14 +173,14 @@  static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 {
 	void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
 	unsigned int shift = (d->irq % 4) * 8;
-	unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
-	u32 val, mask, bit;
+	unsigned int cpu;
+	u32 val, mask, bit, cpuset = 0;
 
-	if (cpu >= 8 || cpu >= nr_cpu_ids)
-		return -EINVAL;
+	for_each_cpu_and(cpu, mask_val, cpu_online_mask)
+		cpuset |= 1 << cpu;
 
 	mask = 0xff << shift;
-	bit = 1 << (cpu + shift);
+	bit = cpuset << shift;
 
 	spin_lock(&irq_controller_lock);
 	val = readl_relaxed(reg) & ~mask;