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