Re: [PATCH 35/38] irqchip: mips-gic: Use pcpu_masks to avoid reading GIC_SH_MASK*

[Marc Zyngier <marc.zyngier@arm.com>]

On 18/08/17 18:11, Paul Burton wrote:
> Hi Marc,
> 
> On Friday, 18 August 2017 08:44:15 PDT Marc Zyngier wrote:
>> On 13/08/17 05:36, Paul Burton wrote:
>>> This patch avoids the need to read the GIC_SH_MASK* registers when
>>> decoding shared interrupts by setting & clearing the interrupt's bit in
>>> the appropriate CPU's pcpu_masks entry when masking or unmasking the
>>> interrupt.
>>>
>>> This effectively means that whilst an interrupt is masked we clear its
>>> bit in all pcpu_masks, which causes gic_handle_shared_int() to ignore it
>>> on all CPUs without needing to check GIC_SH_MASK*.
>>>
>>> In essence, we add a little overhead to masking or unmasking interrupts
>>> but in return reduce the overhead of the far more common task of
>>> decoding interrupts.
>>>
>>> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>>> Cc: Jason Cooper <jason@lakedaemon.net>
>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>> Cc: Ralf Baechle <ralf@linux-mips.org>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: linux-mips@linux-mips.org
>>> ---
>>>
>>>  drivers/irqchip/irq-mips-gic.c | 49
>>>  ++++++++++++++++++++++++------------------ 1 file changed, 28
>>>  insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-mips-gic.c
>>> b/drivers/irqchip/irq-mips-gic.c index 00153231376a..7a42f0b3822f 100644
>>> --- a/drivers/irqchip/irq-mips-gic.c
>>> +++ b/drivers/irqchip/irq-mips-gic.c
>>> @@ -55,6 +55,19 @@ static struct irq_chip gic_level_irq_controller,
>>> gic_edge_irq_controller;> 
>>>  DECLARE_BITMAP(ipi_resrv, GIC_MAX_INTRS);
>>>  DECLARE_BITMAP(ipi_available, GIC_MAX_INTRS);
>>>
>>> +static void gic_setup_pcpu_mask(unsigned int intr, unsigned int cpu)
>>> +{
>>> +	unsigned int i;
>>> +
>>> +	/* Clear the interrupt's bit in all pcpu_masks */
>>> +	for_each_possible_cpu(i)
>>> +		clear_bit(intr, per_cpu_ptr(pcpu_masks, i));
>>
>> This iterates from 0 to nr_cpu_ids-1...
>>
>>> +
>>> +	/* Set the interrupt's bit in the appropriate CPU's mask */
>>> +	if (cpu < NR_CPUS)
>>
>> and here you're using NR_CPUS. I'm a bit worried that you're not quite
>> using the same thing (nr_cpu_ids <= NR_CPUS).
> 
> I think that would be fine - if nr_cpu_ids is less than NR_CPUS then all we 
> risk is leaving bits set in the mask for CPUs that aren't in cpu_possible_mask 
> (which is where nr_cpu_ids comes from). Since those CPUs don't exist & thus 
> can't take interrupts they'll never check the bits in their mask.
> 
> The NR_CPUS check here is basically just to allow the interrupt to be set in 
> none of the masks when called by gic_mask_irq() - NR_CPUS is just some 
> constant value that we know will never be the ID of a CPU that an interrupt's 
> affinity is set to.
> 
> Perhaps it'd be clearer to instead split gic_setup_pcpu_mask() into 2 
> functions - one to just clear all the masks, and one to set the interrupt's 
> bit in the appropriate one? Possibly just inline that second one. That way we 
> wouldn't be using NR_CPUS at all here which might be clearer. How does that 
> sound?

That'd probably be best, even if that's a couple extra lines here and
there. This NR_CPUS is just confusing.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...