Re: [PATCH 34/38] irqchip: mips-gic: Make pcpu_masks a per-cpu variable

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

On 18/08/17 18:02, Paul Burton wrote:
> Hi Marc,
> 
> On Friday, 18 August 2017 08:37:03 PDT Marc Zyngier wrote:
>> On 13/08/17 05:36, Paul Burton wrote:
>>> Define the pcpu_masks variable using the kernel's standard per-cpu
>>> variable support, rather than an open-coded array of structs containing
>>> bitmaps.
>>>
>>> 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 | 17 ++++++++---------
>>>  1 file changed, 8 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-mips-gic.c
>>> b/drivers/irqchip/irq-mips-gic.c index feff4bf97577..00153231376a 100644
>>> --- a/drivers/irqchip/irq-mips-gic.c
>>> +++ b/drivers/irqchip/irq-mips-gic.c
>>> @@ -13,6 +13,7 @@
>>>
>>>  #include <linux/irq.h>
>>>  #include <linux/irqchip.h>
>>>  #include <linux/of_address.h>
>>>
>>> +#include <linux/percpu.h>
>>>
>>>  #include <linux/sched.h>
>>>  #include <linux/smp.h>
>>>
>>> @@ -23,6 +24,7 @@
>>>
>>>  #include <dt-bindings/interrupt-controller/mips-gic.h>
>>>  
>>>  #define GIC_MAX_INTRS		256
>>>
>>> +#define GIC_MAX_LONGS		BITS_TO_LONGS(GIC_MAX_INTRS)
>>>
>>>  /* Add 2 to convert GIC CPU pin to core interrupt */
>>>  #define GIC_CPU_PIN_OFFSET	2
>>>
>>> @@ -40,11 +42,8 @@
>>>
>>>  void __iomem *mips_gic_base;
>>>
>>> -struct gic_pcpu_mask {
>>> -	DECLARE_BITMAP(pcpu_mask, GIC_MAX_INTRS);
>>> -};
>>> +DEFINE_PER_CPU_READ_MOSTLY(unsigned long[GIC_MAX_LONGS], pcpu_masks);
>>>
>>> -static struct gic_pcpu_mask pcpu_masks[NR_CPUS];
>>>
>>>  static DEFINE_SPINLOCK(gic_lock);
>>>  static struct irq_domain *gic_irq_domain;
>>>  static struct irq_domain *gic_ipi_domain;
>>>
>>> @@ -137,7 +136,7 @@ static void gic_handle_shared_int(bool chained)
>>>
>>>  	DECLARE_BITMAP(intrmask, GIC_MAX_INTRS);
>>>  	
>>>  	/* Get per-cpu bitmaps */
>>>
>>> -	pcpu_mask = pcpu_masks[smp_processor_id()].pcpu_mask;
>>> +	pcpu_mask = this_cpu_ptr(pcpu_masks);
>>>
>>>  	if (mips_cm_is64) {
>>>  	
>>>  		__ioread64_copy(pending, addr_gic_pend(),
>>>
>>> @@ -254,8 +253,8 @@ static int gic_set_affinity(struct irq_data *d, const
>>> struct cpumask *cpumask,> 
>>>  	/* Update the pcpu_masks */
>>>  	for (i = 0; i < min(gic_vpes, NR_CPUS); i++)
>>
>> Is there any case where gic_vpes is not equal to nr_cpus?
> 
> Yes - if the kernel is built with CONFIG_NR_CPUS set to something other than 
> the actual number of VPs (Virtual Processors, ie. hardware threads) in the 
> system. (VPE, or Virtual Processing Element, is a roughly equivalent term used 
> in older revisions of the MIPS architecture).
> 
> To be honest I suspect gic_vpes should go away, and for example in this case 
> we should juse use for_each_possible_cpu(). 

That's exactly where I was implicitly aiming to. Either NR_CPUS is set
to something bigger than the physical number of CPU/threads, and
nr_cpus_ids is then the right thing, or NR_CPUS is smaller than that
number, and nr_cpu_ids == NR_CPUS.

So for_each_possible_cpus should always do the right thing.

> The use of gic_vpes here was added 
> by commit 2a0787051182 ("irqchip/mips-gic: Use gic_vpes instead of NR_CPUS") 
> but it doesn't really do a good job of explaining why - I suspect Qais felt it 
> was an optimisation, but that's debatable.

Quite.

> This code will need adjusting to add multi-cluster support, which is my 
> ultimate goal here, anyway. So that'll be one of the next things for me to 
> tackle.
Sounds good to me.

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