Re: [PATCH] irqchip/gic: Enable gic_set_affinity set more than one cpu

[Cheng Chao <cs.os.kernel@gmail.com>]

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