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@xxxxxxxxxx>
>>> Cc: Jason Cooper <jason@xxxxxxxxxxxxxx>
>>> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
>>> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
>>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>> Cc: linux-mips@xxxxxxxxxxxxxx
>>> ---
>>>
>>> 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...
