Messages in this thread | | | Subject | Re: [PATCH] irqchip/gic: Enable gic_set_affinity set more than one cpu | From | Cheng Chao <> | Date | Fri, 14 Oct 2016 10:08:10 +0800 |
| |
Marc,
Thanks for your comments.
Cheng
on 10/13/2016 11:31 PM, Marc Zyngier wrote: > On Thu, 13 Oct 2016 18:57:14 +0800 > Cheng Chao <cs.os.kernel@gmail.com> wrote: > >> GIC can distribute an interrupt to more than one cpu, >> but now, gic_set_affinity sets 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? >
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.
>> >> Signed-off-by: Cheng Chao <cs.os.kernel@gmail.com> >> --- >> drivers/irqchip/irq-gic.c | 28 ++++++++++++++++++++++++---- >> 1 file changed, 24 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >> index 58e5b4e..198d33f 100644 >> --- a/drivers/irqchip/irq-gic.c >> +++ b/drivers/irqchip/irq-gic.c >> @@ -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); > > 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.
>> + } 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; > > 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.
>> + >> + 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. >
| |