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.ker...@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.ker...@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.
>
