irqchip/gic: Enable gic_set_affinity set more than one cpu
Subsystems (2)
PATCH 1/1 irqchip/gic: Enable gic_set_affinity set more than one cpu
GIC can distribute an interrupt to more than one cpu, but now, gic_set_affinity sets only one cpu to handle interrupt.
--- drivers/irqchip/irq-gic.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
drivers/irqchip/irq-gic.c +24 -4
@@ -328,18 +328,38 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,unsigned int cpu, shift = (gic_irq (d) %4 ) *8 ;u32 val, mask, bit;unsigned long flags; +u32 valid_mask; -if (!force) - cpu =cpumask_any_and (mask_val, cpu_online_mask); -else +if (!force) { + valid_mask =cpumask_bits (mask_val)[0 ]; + valid_mask &=cpumask_bits (cpu_online_mask)[0 ]; + + cpu =cpumask_any ((struct cpumask *)&valid_mask); + }else { cpu =cpumask_first (mask_val); + }if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids)return -EINVAL ;gic_lock_irqsave (flags); mask =0xff << shift; - bit = gic_cpu_map[cpu] << shift; + +if (!force) { + bit =0 ; + +for_each_cpu (cpu, (struct cpumask *)&valid_mask) { +if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids) +break ; + + bit |= gic_cpu_map[cpu]; + } + + bit = bit << shift; + }else { + bit = gic_cpu_map[cpu] << shift; + } + val =readl_relaxed (reg) & ~mask;writel_relaxed (val | bit, reg);gic_unlock_irqrestore (flags);
Signature · 1 line
2.4.11
Hi,
This patch has been tested on the SOC: ti AM572x and hisilicon hi35xx, it works. Please review this patch. Any suggestions will be welcome,thanks.
Cheng
Quoted 56 lines · Cheng Chao
GIC can distribute an interrupt to more than one cpu, but now,
gic_set_affinitysets only one cpu to handle interrupt.Signed-off-by Cheng Chao <[email protected]>--- drivers/irqchip/irq-gic.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)drivers/irqchip/irq-gic.c +24
-4@@ -328,18 +328,38 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,unsigned int cpu, shift = (gic_irq (d) %4 ) *8 ;u32 val, mask, bit;unsigned long flags; +u32 valid_mask; -if (!force) - cpu =cpumask_any_and (mask_val, cpu_online_mask); -else +if (!force) { + valid_mask =cpumask_bits (mask_val)[0 ]; + valid_mask &=cpumask_bits (cpu_online_mask)[0 ]; + + cpu =cpumask_any ((struct cpumask *)&valid_mask); + }else { cpu =cpumask_first (mask_val); + }if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids)return -EINVAL ;gic_lock_irqsave (flags); mask =0xff << shift; - bit = gic_cpu_map[cpu] << shift; + +if (!force) { + bit =0 ; + +for_each_cpu (cpu, (struct cpumask *)&valid_mask) { +if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids) +break ; + + bit |= gic_cpu_map[cpu]; + } + + bit = bit << shift; + }else { + bit = gic_cpu_map[cpu] << shift; + } + val =readl_relaxed (reg) & ~mask;writel_relaxed (val | bit, reg);gic_unlock_irqrestore (flags);
Quoted 2 lines · On Thu, 13 Oct 2016 18:57:14 +0800 Cheng Chao <[email protected]> wrote:
GIC can distribute an interrupt to more than one cpu, but now,
gic_set_affinitysets only one cpu to handle interrupt.
What makes you think this is a good idea? What purpose does it serves? I can only see drawbacks to this: You're waking up more than one CPU, wasting power, adding jitter and clobbering the cache.
I assume you see a benefit to that approach, so can you please spell it out?
Quoted 14 lines
Signed-off-by Cheng Chao <[email protected]>--- drivers/irqchip/irq-gic.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)drivers/irqchip/irq-gic.c +0
-0@@ -328,18 +328,38 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,unsigned int cpu, shift = (gic_irq (d) %4 ) *8 ;u32 val, mask, bit;unsigned long flags;
drivers/irqchip/irq-gic.c static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, · line 337
Quoted 10 lines
drivers/irqchip/irq-gic.c +6
-3+u32 valid_mask; -if (!force) - cpu =cpumask_any_and (mask_val, cpu_online_mask); -else +if (!force) { + valid_mask =cpumask_bits (mask_val)[0 ]; + valid_mask &=cpumask_bits (cpu_online_mask)[0 ]; + + cpu =cpumask_any ((struct cpumask *)&valid_mask);
What is wrong with with cpumask_any_and?
Quoted 7 lines
drivers/irqchip/irq-gic.c +2
-0+ }else { cpu =cpumask_first (mask_val); + }if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids)return -EINVAL ;
drivers/irqchip/irq-gic.c static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, · line 353
Quoted 10 lines
drivers/irqchip/irq-gic.c +7
-1gic_lock_irqsave (flags); mask =0xff << shift; - bit = gic_cpu_map[cpu] << shift; + +if (!force) { + bit =0 ; + +for_each_cpu (cpu, (struct cpumask *)&valid_mask) { +if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids) +break ;
Shouldn't that be an error?
drivers/irqchip/irq-gic.c static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, · line 365
Quoted 12 lines
drivers/irqchip/irq-gic.c +9
-0+ + bit |= gic_cpu_map[cpu]; + } + + bit = bit << shift; + }else { + bit = gic_cpu_map[cpu] << shift; + } + val =readl_relaxed (reg) & ~mask;writel_relaxed (val | bit, reg);gic_unlock_irqrestore (flags);
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny.
Marc,
Thanks for your comments.
Cheng
Quoted 6 lines · Marc Zyngier
Quoted 2 lines · On Thu, 13 Oct 2016 18:57:14 +0800 Cheng Chao <[email protected]> wrote:
GIC can distribute an interrupt to more than one cpu, but now,
gic_set_affinitysets only one cpu to handle interrupt.
Quoted 7 lines
What makes you think this is a good idea? What purpose does it serves? I can only see drawbacks to this: You're waking up more than one CPU, wasting power, adding jitter and clobbering the cache.
I assume you see a benefit to that approach, so can you please spell it out?
Ok, You are right, but the performance is another point that we should consider.
We use E1 device to transmit/receive video stream. we find that E1's interrupt is only on the one cpu that cause this cpu usage is almost 100%,
but other cpus is much lower load, so the performance is not good. the cpu is 4-core.
so add CONFIG_ARM_GIC_AFFINITY_SINGLE_CPU is better?
thus we can make a trade-off between the performance with the power etc.
Quoted 16 lines
Quoted 16 lines
Signed-off-by Cheng Chao <[email protected]>--- drivers/irqchip/irq-gic.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)drivers/irqchip/irq-gic.c +1
-0@@ -328,18 +328,38 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,unsigned int cpu, shift = (gic_irq (d) %4 ) *8 ;u32 val, mask, bit;unsigned long flags; +u32 valid_mask;
Quoted 11 lines
drivers/irqchip/irq-gic.c static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, · line 337
Quoted 8 lines
drivers/irqchip/irq-gic.c +5
-3-if (!force) - cpu =cpumask_any_and (mask_val, cpu_online_mask); -else +if (!force) { + valid_mask =cpumask_bits (mask_val)[0 ]; + valid_mask &=cpumask_bits (cpu_online_mask)[0 ]; + + cpu =cpumask_any ((struct cpumask *)&valid_mask);What is wrong with with
cpumask_any_and?
#define cpumask_any_and(mask1, mask2) cpumask_first_and((mask1), (mask2)) #define cpumask_any(srcp) cpumask_first(srcp)
There is no wrong with the cpumask_any_and.
Quoted 7 lines
Quoted 7 lines
drivers/irqchip/irq-gic.c +2
-0+ }else { cpu =cpumask_first (mask_val); + }if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids)return -EINVAL ;
Quoted 13 lines
drivers/irqchip/irq-gic.c static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, · line 353
Quoted 10 lines
drivers/irqchip/irq-gic.c +7
-1gic_lock_irqsave (flags); mask =0xff << shift; - bit = gic_cpu_map[cpu] << shift; + +if (!force) { + bit =0 ; + +for_each_cpu (cpu, (struct cpumask *)&valid_mask) { +if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids) +break ;Shouldn't that be an error?
tested, no error.
at the beginning, I code such like,
cpumask_var_t valid_mask; alloc_cpumask_var(&valid_mask, GFP_KERNEL); cpumask_and(valid_mask, mask_val, cpu_online_mask); for_each_cpu(cpu, valid_mask) {
}
but alloc_cpumask_var maybe fail, so
if (!alloc_cpumask_var(&valid_mask, GFP_KERNEL)) {
/* fail*/
} else {
}
a little more complex.
Quoted 17 lines
drivers/irqchip/irq-gic.c static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, · line 365
Quoted 12 lines
drivers/irqchip/irq-gic.c +9
-0+ + bit |= gic_cpu_map[cpu]; + } + + bit = bit << shift; + }else { + bit = gic_cpu_map[cpu] << shift; + } + val =readl_relaxed (reg) & ~mask;writel_relaxed (val | bit, reg);gic_unlock_irqrestore (flags);Thanks,
M.
Quoted 21 lines · Cheng Chao
Marc,
Thanks for your comments.
Cheng
Quoted 6 lines · Marc Zyngier
Quoted 2 lines · On Thu, 13 Oct 2016 18:57:14 +0800 Cheng Chao <[email protected]> wrote:
GIC can distribute an interrupt to more than one cpu, but now,
gic_set_affinitysets only one cpu to handle interrupt.Quoted 7 lines
What makes you think this is a good idea? What purpose does it serves? I can only see drawbacks to this: You're waking up more than one CPU, wasting power, adding jitter and clobbering the cache.
I assume you see a benefit to that approach, so can you please spell it out?
Quoted 6 lines
Ok, You are right, but the performance is another point that we should consider.
We use E1 device to transmit/receive video stream. we find that E1's interrupt is only on the one cpu that cause this cpu usage is almost 100%,
but other cpus is much lower load, so the performance is not good. the cpu is 4-core.
It looks to me like you're barking up the wrong tree. We have NAPI-enabled network drivers for this exact reason, and adding more interrupts to an already overloaded system doesn't strike me as going in the right direction. May I suggest that you look at integrating NAPI into your E1 driver?
Quoted 2 lines
so add
CONFIG_ARM_GIC_AFFINITY_SINGLE_CPUis better?
thus we can make a trade-off between the performance with the power etc.
No, that's pretty horrible, and I'm not even going to entertain the idea. I suggest you start investigating how to mitigate your interrupt rate instead of just taking more of them.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Quoted 22 lines · Marc Zyngier
Quoted 15 lines
Quoted 6 lines · Marc Zyngier
Quoted 2 lines · On Thu, 13 Oct 2016 18:57:14 +0800 Cheng Chao <[email protected]> wrote:
GIC can distribute an interrupt to more than one cpu, but now,
gic_set_affinitysets only one cpu to handle interrupt.Quoted 7 lines
What makes you think this is a good idea? What purpose does it serves? I can only see drawbacks to this: You're waking up more than one CPU, wasting power, adding jitter and clobbering the cache.
I assume you see a benefit to that approach, so can you please spell it out?
Quoted 6 lines
Ok, You are right, but the performance is another point that we should consider.
We use E1 device to transmit/receive video stream. we find that E1's interrupt is only on the one cpu that cause this cpu usage is almost 100%,
but other cpus is much lower load, so the performance is not good. the cpu is 4-core.
Quoted 6 lines
It looks to me like you're barking up the wrong tree. We have NAPI-enabled network drivers for this exact reason, and adding more interrupts to an already overloaded system doesn't strike me as going in the right direction. May I suggest that you look at integrating NAPI into your E1 driver?
great, NAPI maybe is a good option, I can try to use NAPI. thank you.
In other hand, gic_set_affinity sets only one cpu to handle interrupt,
that really makes me a little confused, why does GIC's driver not like the others(MPIC, APIC etc) to support many cpus to handle interrupt?
It seems that the GIC's driver constrain too much.
We can use /proc/irq/xx/smp_affinity to set what we expect. echo 1 > /proc/irq/xx/smp_affinity, the interrupt on the first cpu. echo 2 > /proc/irq/xx/smp_affinity, the interrupt on the second cpu.
but:
echo 3 > /proc/irq/xx/smp_affinity, the interrupt on the first cpu, no interrupt on the second cpu.
what? why does the second cpu has no interrupt?
regardless of:
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
What makes you think this is a good idea? What purpose does it serves? I can only see drawbacks to this: You're waking up more than one CPU, wasting power, adding jitter and clobbering the cache.
I think it is more reasonable to let user decide what to do.
If I care about the power etc, then I only echo single cpu to/proc/irq/xx/smp_affinity, but if I expect more than one cpu to handle one special interrupt, I can echo 'what I expect cpus' to /proc/irq/xx/smp_affinity.
Quoted 5 lines
Quoted 2 lines
so add
CONFIG_ARM_GIC_AFFINITY_SINGLE_CPUis better?
thus we can make a trade-off between the performance with the power etc.No, that's pretty horrible, and I'm not even going to entertain the idea.
Yes, in fact /proc/irq/xx/smp_affinity is enough.
Quoted 3 lines
I suggest you start investigating how to mitigate your interrupt rate instead of just taking more of them.
Ok, thanks again.
Quoted 4 lines
Thanks,
M.
Quoted 30 lines · Cheng Chao
Quoted 22 lines · Marc Zyngier
Quoted 15 lines
Quoted 6 lines · Marc Zyngier
Quoted 2 lines · On Thu, 13 Oct 2016 18:57:14 +0800 Cheng Chao <[email protected]> wrote:
GIC can distribute an interrupt to more than one cpu, but now,
gic_set_affinitysets only one cpu to handle interrupt.Quoted 7 lines
What makes you think this is a good idea? What purpose does it serves? I can only see drawbacks to this: You're waking up more than one CPU, wasting power, adding jitter and clobbering the cache.
I assume you see a benefit to that approach, so can you please spell it out?
Quoted 6 lines
Ok, You are right, but the performance is another point that we should consider.
We use E1 device to transmit/receive video stream. we find that E1's interrupt is only on the one cpu that cause this cpu usage is almost 100%,
but other cpus is much lower load, so the performance is not good. the cpu is 4-core.Quoted 6 lines
It looks to me like you're barking up the wrong tree. We have NAPI-enabled network drivers for this exact reason, and adding more interrupts to an already overloaded system doesn't strike me as going in the right direction. May I suggest that you look at integrating NAPI into your E1 driver?
Quoted 7 lines
great, NAPI maybe is a good option, I can try to use NAPI. thank you.
In other hand,
gic_set_affinitysets only one cpu to handle interrupt, that really makes me a little confused, why does GIC's driver not like theothers(MPIC, APIC etc)to support many cpus to handle interrupt?It seems that the GIC's driver constrain too much.
There is several drawbacks to this:
- Cache impacts and power efficiency, as already mentioned
- Not virtualizable (you cannot efficiently implement this in a hypervisor that emulates a GICv2 distributor)
- Doesn't scale (you cannot go beyond 8 CPUs)
I strongly suggest you give NAPI a go, and only then consider delivering interrupts to multiple CPUs, because multiple CPU delivery is not future proof.
Quoted 6 lines
I think it is more reasonable to let user decide what to do.
If I care about the power etc, then I only echo single cpu to
/proc/irq/xx/smp_affinity, but if I expect more than one cpu to handle one special interrupt, I can echo 'what I expect cpus' to/proc/irq/xx/smp_affinity.
If that's what you really want, a better patch may be something like this:
drivers/irqchip/irq-gic.c +13 -8
@@ -326,20 +326,25 @@ 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 cpu, shift = (gic_irq (d) %4 ) *8 ; -u32 val, mask, bit; -unsigned long flags; +u32 val, mask, bit =0 ; +unsigned long flags, aff =0 ; -if (!force) - cpu =cpumask_any_and (mask_val, cpu_online_mask); -else - cpu =cpumask_first (mask_val); +for_each_cpu (cpu, mask_val) { +if (force) { + aff =1 << cpu; +break ; + } + + aff |=cpu_online (cpu) << cpu; + } -if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids) +if (!aff)return -EINVAL ;gic_lock_irqsave (flags); mask =0xff << shift; - bit = gic_cpu_map[cpu] << shift; +for_each_set_bit (cpu, &aff, nr_cpu_ids) + bit |= gic_cpu_map[cpu] << shift; val =readl_relaxed (reg) & ~mask;writel_relaxed (val | bit, reg);gic_unlock_irqrestore (flags);
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Quoted 45 lines · Marc Zyngier
Quoted 30 lines · Cheng Chao
Quoted 22 lines · Marc Zyngier
Quoted 15 lines
Quoted 6 lines · Marc Zyngier
Quoted 2 lines · On Thu, 13 Oct 2016 18:57:14 +0800 Cheng Chao <[email protected]> wrote:
GIC can distribute an interrupt to more than one cpu, but now,
gic_set_affinitysets only one cpu to handle interrupt.Quoted 7 lines
What makes you think this is a good idea? What purpose does it serves? I can only see drawbacks to this: You're waking up more than one CPU, wasting power, adding jitter and clobbering the cache.
I assume you see a benefit to that approach, so can you please spell it out?
Quoted 6 lines
Ok, You are right, but the performance is another point that we should consider.
We use E1 device to transmit/receive video stream. we find that E1's interrupt is only on the one cpu that cause this cpu usage is almost 100%,
but other cpus is much lower load, so the performance is not good. the cpu is 4-core.Quoted 6 lines
It looks to me like you're barking up the wrong tree. We have NAPI-enabled network drivers for this exact reason, and adding more interrupts to an already overloaded system doesn't strike me as going in the right direction. May I suggest that you look at integrating NAPI into your E1 driver?
Quoted 7 lines
great, NAPI maybe is a good option, I can try to use NAPI. thank you.
In other hand,
gic_set_affinitysets only one cpu to handle interrupt, that really makes me a little confused, why does GIC's driver not like theothers(MPIC, APIC etc)to support many cpus to handle interrupt?It seems that the GIC's driver constrain too much.
There is several drawbacks to this:
- Cache impacts and power efficiency, as already mentioned
- Not virtualizable (you cannot efficiently implement this in a hypervisor that emulates a GICv2 distributor)
- Doesn't scale (you cannot go beyond 8 CPUs)
Quoted 4 lines
I strongly suggest you give NAPI a go, and only then consider delivering interrupts to multiple CPUs, because multiple CPU delivery is not future proof.
Thanks again, the E1 driver with NAPI is on the right track.
Quoted 9 lines
Quoted 6 lines
I think it is more reasonable to let user decide what to do.
If I care about the power etc, then I only echo single cpu to
/proc/irq/xx/smp_affinity, but if I expect more than one cpu to handle one special interrupt, I can echo 'what I expect cpus' to/proc/irq/xx/smp_affinity.If that's what you really want, a better patch may be something like this:
I hope the GIC'c driver is more flexible, and gic_set_affinity() doesn't constrain to set only one cpu. the GIC supports to distribute more than one cpu after all.
Quoted 29 lines
drivers/irqchip/irq-gic.c +11
-7@@ -326,20 +326,25 @@ 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 cpu, shift = (gic_irq (d) %4 ) *8 ; -u32 val, mask, bit; -unsigned long flags; +u32 val, mask, bit =0 ; +unsigned long flags, aff =0 ; -if (!force) - cpu =cpumask_any_and (mask_val, cpu_online_mask); -else - cpu =cpumask_first (mask_val); +for_each_cpu (cpu, mask_val) { +if (force) { + aff =1 << cpu; +break ; + } + + aff |=cpu_online (cpu) << cpu; + } -if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids) +if (!aff)
drivers/irqchip/irq-gic.c static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, · line 350
Quoted 10 lines
drivers/irqchip/irq-gic.c +2
-1return -EINVAL ;gic_lock_irqsave (flags); mask =0xff << shift; - bit = gic_cpu_map[cpu] << shift; +for_each_set_bit (cpu, &aff, nr_cpu_ids) + bit |= gic_cpu_map[cpu] << shift; val =readl_relaxed (reg) & ~mask;writel_relaxed (val | bit, reg);gic_unlock_irqrestore (flags);
this patch is more better than before.
a little check add.
drivers/irqchip/irq-gic.c +16 -8
@@ -326,20 +326,28 @@ 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 cpu, shift = (gic_irq (d) %4 ) *8 ; -u32 val, mask, bit; -unsigned long flags; +u32 val, mask, bit =0 ; +unsigned long flags, aff =0 ; -if (!force) - cpu =cpumask_any_and (mask_val, cpu_online_mask); -else - cpu =cpumask_first (mask_val); +for_each_cpu (cpu, mask_val) { +if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids) +break ; + +if (force) { + aff =1 << cpu; +break ; + } + + aff |=cpu_online (cpu) << cpu; + } -if (cpu >=NR_GIC_CPU_IF || cpu >= nr_cpu_ids) +if (!aff)return -EINVAL ;gic_lock_irqsave (flags); mask =0xff << shift; - bit = gic_cpu_map[cpu] << shift; +for_each_set_bit (cpu, &aff, nr_cpu_ids) + bit |= gic_cpu_map[cpu] << shift; val =readl_relaxed (reg) & ~mask;writel_relaxed (val | bit, reg);gic_unlock_irqrestore (flags);
Quoted 4 lines
Thanks,
M.
Thanks,
Cheng