[PATCH 0/6] Remove on-stack cpumask var for irq subsystem

[PATCH 0/6] Remove on-stack cpumask var for irq subsystem

[Dawei Li]

Hi,

Generally it's preferable to avoid placing cpumasks on the stack, as
for large values of NR_CPUS these can consume significant amounts of
stack space and make stack overflows more likely.

One may argue that alloc_cpumask_var() and its friends are the formal
way for these cases. But for struct irqchip::irq_set_affinity(), it's
called under atomic context(raw spinlock held), and dynamic memory
allocation in atomic context is less-favorable.

So a new helper is introduced to address all these issues above. It's
free of any context issue and intermediate cpumask variable allocation
issue(no matter it's on stack or heap).

The case with gic-v3-its is special from others since it's not related
to intersections between 3 cpumask.

And yes, the helper naming(maybe the whole idea) is terrible, and I am
all ears for any comments from you :).

Dawei Li (6):
  cpumask: introduce cpumask_first_and_and()
  irqchip/irq-bcm6345-l1: Avoid explicit cpumask allocation on stack
  irqchip/gic-v3-its: Avoid explicit cpumask allocation on stack
  irqchip/loongson-eiointc: Avoid explicit cpumask allocation on stack
  irqchip/riscv-aplic-direct: Avoid explicit cpumask allocation on stack
  irqchip/sifive-plic: Avoid explicit cpumask allocation on stack

 drivers/irqchip/irq-bcm6345-l1.c         |  7 ++----
 drivers/irqchip/irq-gic-v3-its.c         |  9 +++++---
 drivers/irqchip/irq-loongson-eiointc.c   |  9 +++-----
 drivers/irqchip/irq-riscv-aplic-direct.c |  8 +++----
 drivers/irqchip/irq-sifive-plic.c        |  8 +++----
 include/linux/cpumask.h                  | 17 ++++++++++++++
 include/linux/find.h                     | 29 ++++++++++++++++++++++++
 lib/find_bit.c                           | 14 ++++++++++++
 8 files changed, 77 insertions(+), 24 deletions(-)

Thanks,

    Dawei

-- 
2.27.0

[PATCH 1/6] cpumask: introduce cpumask_first_and_and()

[Dawei Li]

For some cases, it's required to make intersection between 3 cpumasks
and return possible cpu bit. Current implementation for these cases are
allocating a temporary cpumask var(sometimes on stack) storing intermediate
calculation result.

Introduce cpumask_first_and_and() to get rid of this intermediate orinted
approach. Instead, cpumask_first_and_and() works in-place with all inputs
and produce desired output directly.

Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
 include/linux/cpumask.h | 17 +++++++++++++++++
 include/linux/find.h    | 29 +++++++++++++++++++++++++++++
 lib/find_bit.c          | 14 ++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 1c29947db848..c46f9e9e1d66 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -187,6 +187,23 @@ unsigned int cpumask_first_and(const struct cpumask *srcp1, const struct cpumask
 	return find_first_and_bit(cpumask_bits(srcp1), cpumask_bits(srcp2), small_cpumask_bits);
 }
 
+/**
+ * cpumask_first_and_and - return the first cpu from *srcp1 & *srcp2 & *srcp3
+ * @srcp1: the first input
+ * @srcp2: the second input
+ * @srcp3: the third input
+ *
+ * Return: >= nr_cpu_ids if no cpus set in all.
+ */
+static inline
+unsigned int cpumask_first_and_and(const struct cpumask *srcp1,
+				   const struct cpumask *srcp2,
+				   const struct cpumask *srcp3)
+{
+	return find_first_and_and_bit(cpumask_bits(srcp1), cpumask_bits(srcp2),
+				      cpumask_bits(srcp3), small_cpumask_bits);
+}
+
 /**
  * cpumask_last - get the last CPU in a cpumask
  * @srcp:	- the cpumask pointer
diff --git a/include/linux/find.h b/include/linux/find.h
index c69598e383c1..047081c6b9f7 100644
--- a/include/linux/find.h
+++ b/include/linux/find.h
@@ -29,6 +29,8 @@ unsigned long __find_nth_and_andnot_bit(const unsigned long *addr1, const unsign
 					unsigned long n);
 extern unsigned long _find_first_and_bit(const unsigned long *addr1,
 					 const unsigned long *addr2, unsigned long size);
+unsigned long _find_first_and_and_bit(const unsigned long *addr1, const unsigned long *addr2,
+				      const unsigned long *addr3, unsigned long size);
 extern unsigned long _find_first_zero_bit(const unsigned long *addr, unsigned long size);
 extern unsigned long _find_last_bit(const unsigned long *addr, unsigned long size);
 
@@ -345,6 +347,33 @@ unsigned long find_first_and_bit(const unsigned long *addr1,
 }
 #endif
 
+#ifndef find_first_and_and_bit
+/**
+ * find_first_and_and_bit - find the first set bit in 3 memory regions
+ * @addr1: The first address to base the search on
+ * @addr2: The second address to base the search on
+ * @addr3: The third address to base the search on
+ * @size: The bitmap size in bits
+ *
+ * Returns the bit number for the first set bit
+ * If no bits are set, returns @size.
+ */
+static inline
+unsigned long find_first_and_and_bit(const unsigned long *addr1,
+				     const unsigned long *addr2,
+				     const unsigned long *addr3,
+				     unsigned long size)
+{
+	if (small_const_nbits(size)) {
+		unsigned long val = *addr1 & *addr2 & *addr3 & GENMASK(size - 1, 0);
+
+		return val ? __ffs(val) : size;
+	}
+
+	return _find_first_and_and_bit(addr1, addr2, addr3, size);
+}
+#endif
+
 #ifndef find_first_zero_bit
 /**
  * find_first_zero_bit - find the first cleared bit in a memory region
diff --git a/lib/find_bit.c b/lib/find_bit.c
index 32f99e9a670e..fdc5c4117e8b 100644
--- a/lib/find_bit.c
+++ b/lib/find_bit.c
@@ -116,6 +116,20 @@ unsigned long _find_first_and_bit(const unsigned long *addr1,
 EXPORT_SYMBOL(_find_first_and_bit);
 #endif
 
+#ifndef find_first_and_and_bit
+/*
+ * Find the first set bit in three memory regions.
+ */
+unsigned long _find_first_and_and_bit(const unsigned long *addr1,
+				      const unsigned long *addr2,
+				      const unsigned long *addr3,
+				      unsigned long size)
+{
+	return FIND_FIRST_BIT(addr1[idx] & addr2[idx] & addr3[idx], /* nop */, size);
+}
+EXPORT_SYMBOL(_find_first_and_and_bit);
+#endif
+
 #ifndef find_first_zero_bit
 /*
  * Find the first cleared bit in a memory region.
-- 
2.27.0

Re: [PATCH 1/6] cpumask: introduce cpumask_first_and_and()

[Yury Norov]

On Fri, Apr 12, 2024 at 3:59 AM Dawei Li <dawei.li@shingroup.cn> wrote:
>
> For some cases, it's required to make intersection between 3 cpumasks
> and return possible cpu bit. Current implementation for these cases are

/s/possible cpu bit/set cpu/

And sometimes you just need an intersection cpumask, and don't need to return
any set cpu. This patch introduces new API, so description should be as
common as possible.

We've already have some 3-bitmap functions. Can you look at commit messages
there and align your wording?

I'll be OK if you just skip this part and go straight to "Introduce
... to get rid of"

> allocating a temporary cpumask var(sometimes on stack) storing intermediate
> calculation result.
>
> Introduce cpumask_first_and_and() to get rid of this intermediate orinted

what the 'orinted' is?

> approach. Instead, cpumask_first_and_and() works in-place with all inputs
> and produce desired output directly.

/s/produce/procuces/

>
> Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> ---
>  include/linux/cpumask.h | 17 +++++++++++++++++
>  include/linux/find.h    | 29 +++++++++++++++++++++++++++++
>  lib/find_bit.c          | 14 ++++++++++++++
>  3 files changed, 60 insertions(+)
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 1c29947db848..c46f9e9e1d66 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -187,6 +187,23 @@ unsigned int cpumask_first_and(const struct cpumask *srcp1, const struct cpumask
>         return find_first_and_bit(cpumask_bits(srcp1), cpumask_bits(srcp2), small_cpumask_bits);
>  }
>
> +/**
> + * cpumask_first_and_and - return the first cpu from *srcp1 & *srcp2 & *srcp3
> + * @srcp1: the first input
> + * @srcp2: the second input
> + * @srcp3: the third input
> + *
> + * Return: >= nr_cpu_ids if no cpus set in all.
> + */
> +static inline
> +unsigned int cpumask_first_and_and(const struct cpumask *srcp1,
> +                                  const struct cpumask *srcp2,
> +                                  const struct cpumask *srcp3)
> +{
> +       return find_first_and_and_bit(cpumask_bits(srcp1), cpumask_bits(srcp2),
> +                                     cpumask_bits(srcp3), small_cpumask_bits);
> +}
> +
>  /**
>   * cpumask_last - get the last CPU in a cpumask
>   * @srcp:      - the cpumask pointer
> diff --git a/include/linux/find.h b/include/linux/find.h
> index c69598e383c1..047081c6b9f7 100644
> --- a/include/linux/find.h
> +++ b/include/linux/find.h
> @@ -29,6 +29,8 @@ unsigned long __find_nth_and_andnot_bit(const unsigned long *addr1, const unsign
>                                         unsigned long n);
>  extern unsigned long _find_first_and_bit(const unsigned long *addr1,
>                                          const unsigned long *addr2, unsigned long size);
> +unsigned long _find_first_and_and_bit(const unsigned long *addr1, const unsigned long *addr2,
> +                                     const unsigned long *addr3, unsigned long size);
>  extern unsigned long _find_first_zero_bit(const unsigned long *addr, unsigned long size);
>  extern unsigned long _find_last_bit(const unsigned long *addr, unsigned long size);
>
> @@ -345,6 +347,33 @@ unsigned long find_first_and_bit(const unsigned long *addr1,
>  }
>  #endif
>
> +#ifndef find_first_and_and_bit

Don't need to protect new API unless you've got an actual arch implementation.

> +/**
> + * find_first_and_and_bit - find the first set bit in 3 memory regions
> + * @addr1: The first address to base the search on
> + * @addr2: The second address to base the search on
> + * @addr3: The third address to base the search on
> + * @size: The bitmap size in bits
> + *
> + * Returns the bit number for the first set bit
> + * If no bits are set, returns @size.
> + */
> +static inline
> +unsigned long find_first_and_and_bit(const unsigned long *addr1,
> +                                    const unsigned long *addr2,
> +                                    const unsigned long *addr3,
> +                                    unsigned long size)
> +{
> +       if (small_const_nbits(size)) {
> +               unsigned long val = *addr1 & *addr2 & *addr3 & GENMASK(size - 1, 0);
> +
> +               return val ? __ffs(val) : size;
> +       }
> +
> +       return _find_first_and_and_bit(addr1, addr2, addr3, size);
> +}
> +#endif
> +
>  #ifndef find_first_zero_bit
>  /**
>   * find_first_zero_bit - find the first cleared bit in a memory region
> diff --git a/lib/find_bit.c b/lib/find_bit.c
> index 32f99e9a670e..fdc5c4117e8b 100644
> --- a/lib/find_bit.c
> +++ b/lib/find_bit.c
> @@ -116,6 +116,20 @@ unsigned long _find_first_and_bit(const unsigned long *addr1,
>  EXPORT_SYMBOL(_find_first_and_bit);
>  #endif
>
> +#ifndef find_first_and_and_bit
> +/*
> + * Find the first set bit in three memory regions.
> + */
> +unsigned long _find_first_and_and_bit(const unsigned long *addr1,
> +                                     const unsigned long *addr2,
> +                                     const unsigned long *addr3,
> +                                     unsigned long size)
> +{
> +       return FIND_FIRST_BIT(addr1[idx] & addr2[idx] & addr3[idx], /* nop */, size);
> +}
> +EXPORT_SYMBOL(_find_first_and_and_bit);
> +#endif
> +
>  #ifndef find_first_zero_bit
>  /*
>   * Find the first cleared bit in a memory region.
> --
> 2.27.0
>

Re: [PATCH 1/6] cpumask: introduce cpumask_first_and_and()

[Dawei Li]

Hi Yury,

Thanks for the review.

On Fri, Apr 12, 2024 at 07:40:30AM -0700, Yury Norov wrote:
> On Fri, Apr 12, 2024 at 3:59 AM Dawei Li <dawei.li@shingroup.cn> wrote:
> >
> > For some cases, it's required to make intersection between 3 cpumasks
> > and return possible cpu bit. Current implementation for these cases are
> 
> /s/possible cpu bit/set cpu/
> 
> And sometimes you just need an intersection cpumask, and don't need to return
> any set cpu. This patch introduces new API, so description should be as
> common as possible.
> 
> We've already have some 3-bitmap functions. Can you look at commit messages
> there and align your wording?
> 
> I'll be OK if you just skip this part and go straight to "Introduce
> ... to get rid of"

Ack.

> 
> > allocating a temporary cpumask var(sometimes on stack) storing intermediate
> > calculation result.
> >
> > Introduce cpumask_first_and_and() to get rid of this intermediate orinted
> 
> what the 'orinted' is?

Good catch, typo.

> 
> > approach. Instead, cpumask_first_and_and() works in-place with all inputs
> > and produce desired output directly.
> 
> /s/produce/procuces/

/s/produce/produces/ :)

> 
> >
> > Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> > ---
> >  include/linux/cpumask.h | 17 +++++++++++++++++
> >  include/linux/find.h    | 29 +++++++++++++++++++++++++++++
> >  lib/find_bit.c          | 14 ++++++++++++++
> >  3 files changed, 60 insertions(+)
> >
> > diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> > index 1c29947db848..c46f9e9e1d66 100644
> > --- a/include/linux/cpumask.h
> > +++ b/include/linux/cpumask.h
> > @@ -187,6 +187,23 @@ unsigned int cpumask_first_and(const struct cpumask *srcp1, const struct cpumask
> >         return find_first_and_bit(cpumask_bits(srcp1), cpumask_bits(srcp2), small_cpumask_bits);
> >  }
> >
> > +/**
> > + * cpumask_first_and_and - return the first cpu from *srcp1 & *srcp2 & *srcp3
> > + * @srcp1: the first input
> > + * @srcp2: the second input
> > + * @srcp3: the third input
> > + *
> > + * Return: >= nr_cpu_ids if no cpus set in all.
> > + */
> > +static inline
> > +unsigned int cpumask_first_and_and(const struct cpumask *srcp1,
> > +                                  const struct cpumask *srcp2,
> > +                                  const struct cpumask *srcp3)
> > +{
> > +       return find_first_and_and_bit(cpumask_bits(srcp1), cpumask_bits(srcp2),
> > +                                     cpumask_bits(srcp3), small_cpumask_bits);
> > +}
> > +
> >  /**
> >   * cpumask_last - get the last CPU in a cpumask
> >   * @srcp:      - the cpumask pointer
> > diff --git a/include/linux/find.h b/include/linux/find.h
> > index c69598e383c1..047081c6b9f7 100644
> > --- a/include/linux/find.h
> > +++ b/include/linux/find.h
> > @@ -29,6 +29,8 @@ unsigned long __find_nth_and_andnot_bit(const unsigned long *addr1, const unsign
> >                                         unsigned long n);
> >  extern unsigned long _find_first_and_bit(const unsigned long *addr1,
> >                                          const unsigned long *addr2, unsigned long size);
> > +unsigned long _find_first_and_and_bit(const unsigned long *addr1, const unsigned long *addr2,
> > +                                     const unsigned long *addr3, unsigned long size);
> >  extern unsigned long _find_first_zero_bit(const unsigned long *addr, unsigned long size);
> >  extern unsigned long _find_last_bit(const unsigned long *addr, unsigned long size);
> >
> > @@ -345,6 +347,33 @@ unsigned long find_first_and_bit(const unsigned long *addr1,
> >  }
> >  #endif
> >
> > +#ifndef find_first_and_and_bit
> 
> Don't need to protect new API unless you've got an actual arch implementation.

Correct. I will remove it in respin.


Thanks,

    Dawei

> 
> > +/**
> > + * find_first_and_and_bit - find the first set bit in 3 memory regions
> > + * @addr1: The first address to base the search on
> > + * @addr2: The second address to base the search on
> > + * @addr3: The third address to base the search on
> > + * @size: The bitmap size in bits
> > + *
> > + * Returns the bit number for the first set bit
> > + * If no bits are set, returns @size.
> > + */
> > +static inline
> > +unsigned long find_first_and_and_bit(const unsigned long *addr1,
> > +                                    const unsigned long *addr2,
> > +                                    const unsigned long *addr3,
> > +                                    unsigned long size)
> > +{
> > +       if (small_const_nbits(size)) {
> > +               unsigned long val = *addr1 & *addr2 & *addr3 & GENMASK(size - 1, 0);
> > +
> > +               return val ? __ffs(val) : size;
> > +       }
> > +
> > +       return _find_first_and_and_bit(addr1, addr2, addr3, size);
> > +}
> > +#endif
> > +
> >  #ifndef find_first_zero_bit
> >  /**
> >   * find_first_zero_bit - find the first cleared bit in a memory region
> > diff --git a/lib/find_bit.c b/lib/find_bit.c
> > index 32f99e9a670e..fdc5c4117e8b 100644
> > --- a/lib/find_bit.c
> > +++ b/lib/find_bit.c
> > @@ -116,6 +116,20 @@ unsigned long _find_first_and_bit(const unsigned long *addr1,
> >  EXPORT_SYMBOL(_find_first_and_bit);
> >  #endif
> >
> > +#ifndef find_first_and_and_bit
> > +/*
> > + * Find the first set bit in three memory regions.
> > + */
> > +unsigned long _find_first_and_and_bit(const unsigned long *addr1,
> > +                                     const unsigned long *addr2,
> > +                                     const unsigned long *addr3,
> > +                                     unsigned long size)
> > +{
> > +       return FIND_FIRST_BIT(addr1[idx] & addr2[idx] & addr3[idx], /* nop */, size);
> > +}
> > +EXPORT_SYMBOL(_find_first_and_and_bit);
> > +#endif
> > +
> >  #ifndef find_first_zero_bit
> >  /*
> >   * Find the first cleared bit in a memory region.
> > --
> > 2.27.0
> >
> 

[PATCH 2/6] irqchip/irq-bcm6345-l1: Avoid explicit cpumask allocation on stack

[Dawei Li]

In general it's preferable to avoid placing cpumasks on the stack, as
for large values of NR_CPUS these can consume significant amounts of
stack space and make stack overflows more likely.

Use cpumask_first_and_and() to avoid the need for a temporary cpumask on
the stack.

Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
 drivers/irqchip/irq-bcm6345-l1.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-bcm6345-l1.c b/drivers/irqchip/irq-bcm6345-l1.c
index eb02d203c963..6df37957b39c 100644
--- a/drivers/irqchip/irq-bcm6345-l1.c
+++ b/drivers/irqchip/irq-bcm6345-l1.c
@@ -192,14 +192,11 @@ static int bcm6345_l1_set_affinity(struct irq_data *d,
 	u32 mask = BIT(d->hwirq % IRQS_PER_WORD);
 	unsigned int old_cpu = cpu_for_irq(intc, d);
 	unsigned int new_cpu;
-	struct cpumask valid;
 	unsigned long flags;
 	bool enabled;
 
-	if (!cpumask_and(&valid, &intc->cpumask, dest))
-		return -EINVAL;
-
-	new_cpu = cpumask_any_and(&valid, cpu_online_mask);
+	new_cpu = cpumask_first_and_and(&intc->cpumask, dest,
+					cpu_online_mask);
 	if (new_cpu >= nr_cpu_ids)
 		return -EINVAL;
 
-- 
2.27.0

[PATCH 3/6] irqchip/gic-v3-its: Avoid explicit cpumask allocation on stack

[Dawei Li]

In general it's preferable to avoid placing cpumasks on the stack, as
for large values of NR_CPUS these can consume significant amounts of
stack space and make stack overflows more likely.

Remove cpumask var on stack and use proper cpumask API to address it.

Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
 drivers/irqchip/irq-gic-v3-its.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index fca888b36680..a821396c4261 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3826,7 +3826,7 @@ static int its_vpe_set_affinity(struct irq_data *d,
 				bool force)
 {
 	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
-	struct cpumask common, *table_mask;
+	struct cpumask *table_mask;
 	unsigned long flags;
 	int from, cpu;
 
@@ -3850,8 +3850,11 @@ static int its_vpe_set_affinity(struct irq_data *d,
 	 * If we are offered another CPU in the same GICv4.1 ITS
 	 * affinity, pick this one. Otherwise, any CPU will do.
 	 */
-	if (table_mask && cpumask_and(&common, mask_val, table_mask))
-		cpu = cpumask_test_cpu(from, &common) ? from : cpumask_first(&common);
+	if (table_mask && cpumask_intersects(mask_val, table_mask)) {
+		cpu = cpumask_test_cpu(from, mask_val) &&
+		      cpumask_test_cpu(from, table_mask) ?
+		      from : cpumask_first_and(mask_val, table_mask);
+	}
 	else
 		cpu = cpumask_first(mask_val);
 
-- 
2.27.0

Re: [PATCH 3/6] irqchip/gic-v3-its: Avoid explicit cpumask allocation on stack

[Marc Zyngier]

On Fri, 12 Apr 2024 11:58:36 +0100,
Dawei Li <dawei.li@shingroup.cn> wrote:
> 
> In general it's preferable to avoid placing cpumasks on the stack, as
> for large values of NR_CPUS these can consume significant amounts of
> stack space and make stack overflows more likely.
>
> Remove cpumask var on stack and use proper cpumask API to address it.

Define proper. Or better, define what is "improper" about the current
usage.

>
> Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index fca888b36680..a821396c4261 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3826,7 +3826,7 @@ static int its_vpe_set_affinity(struct irq_data *d,
>  				bool force)
>  {
>  	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> -	struct cpumask common, *table_mask;
> +	struct cpumask *table_mask;
>  	unsigned long flags;
>  	int from, cpu;
>  
> @@ -3850,8 +3850,11 @@ static int its_vpe_set_affinity(struct irq_data *d,
>  	 * If we are offered another CPU in the same GICv4.1 ITS
>  	 * affinity, pick this one. Otherwise, any CPU will do.
>  	 */
> -	if (table_mask && cpumask_and(&common, mask_val, table_mask))
> -		cpu = cpumask_test_cpu(from, &common) ? from : cpumask_first(&common);
> +	if (table_mask && cpumask_intersects(mask_val, table_mask)) {
> +		cpu = cpumask_test_cpu(from, mask_val) &&
> +		      cpumask_test_cpu(from, table_mask) ?
> +		      from : cpumask_first_and(mask_val, table_mask);

So we may end-up computing the AND of the two bitmaps twice (once for
cpumask_intersects(), once for cpumask_first_and()), instead of only
doing it once.

I don't expect that to be horrible, but I also note that you don't
even talk about the trade-offs you are choosing to make.

> +	}
>  	else
>  		cpu = cpumask_first(mask_val);

Please fix the coding style (if () { ... } else { ... }).

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 3/6] irqchip/gic-v3-its: Avoid explicit cpumask allocation on stack

[Dawei Li]

Hi Marc,

Thanks for the review.

On Fri, Apr 12, 2024 at 02:53:32PM +0100, Marc Zyngier wrote:
> On Fri, 12 Apr 2024 11:58:36 +0100,
> Dawei Li <dawei.li@shingroup.cn> wrote:
> > 
> > In general it's preferable to avoid placing cpumasks on the stack, as
> > for large values of NR_CPUS these can consume significant amounts of
> > stack space and make stack overflows more likely.
> >
> > Remove cpumask var on stack and use proper cpumask API to address it.
> 
> Define proper. Or better, define what is "improper" about the current
> usage.

Sorry for the confusion.

I didn't mean current implementation is 'improper', actually both
implementations share equivalent API usages. I will remove this
misleading expression from commit message.

> 
> >
> > Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> > ---
> >  drivers/irqchip/irq-gic-v3-its.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > index fca888b36680..a821396c4261 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -3826,7 +3826,7 @@ static int its_vpe_set_affinity(struct irq_data *d,
> >  				bool force)
> >  {
> >  	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> > -	struct cpumask common, *table_mask;
> > +	struct cpumask *table_mask;
> >  	unsigned long flags;
> >  	int from, cpu;
> >  
> > @@ -3850,8 +3850,11 @@ static int its_vpe_set_affinity(struct irq_data *d,
> >  	 * If we are offered another CPU in the same GICv4.1 ITS
> >  	 * affinity, pick this one. Otherwise, any CPU will do.
> >  	 */
> > -	if (table_mask && cpumask_and(&common, mask_val, table_mask))
> > -		cpu = cpumask_test_cpu(from, &common) ? from : cpumask_first(&common);
> > +	if (table_mask && cpumask_intersects(mask_val, table_mask)) {
> > +		cpu = cpumask_test_cpu(from, mask_val) &&
> > +		      cpumask_test_cpu(from, table_mask) ?
> > +		      from : cpumask_first_and(mask_val, table_mask);
> 
> So we may end-up computing the AND of the two bitmaps twice (once for
> cpumask_intersects(), once for cpumask_first_and()), instead of only
> doing it once.

Actually maybe it's possible to merge these 2 bitmap ops into one:

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index fca888b36680..7a267777bd0b 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3826,7 +3826,8 @@ static int its_vpe_set_affinity(struct irq_data *d,
                                bool force)
 {
        struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
-       struct cpumask common, *table_mask;
+       struct cpumask *table_mask;
+       unsigned int common;
        unsigned long flags;
        int from, cpu;

@@ -3850,10 +3851,13 @@ static int its_vpe_set_affinity(struct irq_data *d,
         * If we are offered another CPU in the same GICv4.1 ITS
         * affinity, pick this one. Otherwise, any CPU will do.
         */
-       if (table_mask && cpumask_and(&common, mask_val, table_mask))
-               cpu = cpumask_test_cpu(from, &common) ? from : cpumask_first(&common);
-       else
+       if (table_mask && (common = cpumask_first_and(mask_val, table_mask)) < nr_cpu_ids) {
+               cpu = cpumask_test_cpu(from, mask_val) &&
+                     cpumask_test_cpu(from, table_mask) ?
+                     from : common;
+       } else {
                cpu = cpumask_first(mask_val);
+       }

> 
> I don't expect that to be horrible, but I also note that you don't
> even talk about the trade-offs you are choosing to make.

With change above, I assume that the tradeoff is minor and can be ignored?

And I aplogize if I am missing something.

> 
> > +	}
> >  	else
> >  		cpu = cpumask_first(mask_val);
> 
> Please fix the coding style (if () { ... } else { ... }).

Ack.


Thanks,

    Dawei

> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
> 

Re: [PATCH 3/6] irqchip/gic-v3-its: Avoid explicit cpumask allocation on stack

[Marc Zyngier]

On Sat, 13 Apr 2024 11:29:20 +0100,
Dawei Li <dawei.li@shingroup.cn> wrote:
> 
> Hi Marc,
> 
> Thanks for the review.
> 
> On Fri, Apr 12, 2024 at 02:53:32PM +0100, Marc Zyngier wrote:
> > On Fri, 12 Apr 2024 11:58:36 +0100,
> > Dawei Li <dawei.li@shingroup.cn> wrote:
> > > 
> > > In general it's preferable to avoid placing cpumasks on the stack, as
> > > for large values of NR_CPUS these can consume significant amounts of
> > > stack space and make stack overflows more likely.
> > >
> > > Remove cpumask var on stack and use proper cpumask API to address it.
> > 
> > Define proper. Or better, define what is "improper" about the current
> > usage.
> 
> Sorry for the confusion.
> 
> I didn't mean current implementation is 'improper', actually both
> implementations share equivalent API usages. I will remove this
> misleading expression from commit message.
> 
> > 
> > >
> > > Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> > > ---
> > >  drivers/irqchip/irq-gic-v3-its.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > > index fca888b36680..a821396c4261 100644
> > > --- a/drivers/irqchip/irq-gic-v3-its.c
> > > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > > @@ -3826,7 +3826,7 @@ static int its_vpe_set_affinity(struct irq_data *d,
> > >  				bool force)
> > >  {
> > >  	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> > > -	struct cpumask common, *table_mask;
> > > +	struct cpumask *table_mask;
> > >  	unsigned long flags;
> > >  	int from, cpu;
> > >  
> > > @@ -3850,8 +3850,11 @@ static int its_vpe_set_affinity(struct irq_data *d,
> > >  	 * If we are offered another CPU in the same GICv4.1 ITS
> > >  	 * affinity, pick this one. Otherwise, any CPU will do.
> > >  	 */
> > > -	if (table_mask && cpumask_and(&common, mask_val, table_mask))
> > > -		cpu = cpumask_test_cpu(from, &common) ? from : cpumask_first(&common);
> > > +	if (table_mask && cpumask_intersects(mask_val, table_mask)) {
> > > +		cpu = cpumask_test_cpu(from, mask_val) &&
> > > +		      cpumask_test_cpu(from, table_mask) ?
> > > +		      from : cpumask_first_and(mask_val, table_mask);
> > 
> > So we may end-up computing the AND of the two bitmaps twice (once for
> > cpumask_intersects(), once for cpumask_first_and()), instead of only
> > doing it once.
> 
> Actually maybe it's possible to merge these 2 bitmap ops into one:
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index fca888b36680..7a267777bd0b 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -3826,7 +3826,8 @@ static int its_vpe_set_affinity(struct irq_data *d,
>                                 bool force)
>  {
>         struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
> -       struct cpumask common, *table_mask;
> +       struct cpumask *table_mask;
> +       unsigned int common;
>         unsigned long flags;
>         int from, cpu;
> 
> @@ -3850,10 +3851,13 @@ static int its_vpe_set_affinity(struct irq_data *d,
>          * If we are offered another CPU in the same GICv4.1 ITS
>          * affinity, pick this one. Otherwise, any CPU will do.
>          */
> -       if (table_mask && cpumask_and(&common, mask_val, table_mask))
> -               cpu = cpumask_test_cpu(from, &common) ? from : cpumask_first(&common);
> -       else
> +       if (table_mask && (common = cpumask_first_and(mask_val, table_mask)) < nr_cpu_ids) {
> +               cpu = cpumask_test_cpu(from, mask_val) &&
> +                     cpumask_test_cpu(from, table_mask) ?
> +                     from : common;
> +       } else {
>                 cpu = cpumask_first(mask_val);
> +       }
> 
> > 
> > I don't expect that to be horrible, but I also note that you don't
> > even talk about the trade-offs you are choosing to make.
> 
> With change above, I assume that the tradeoff is minor and can be ignored?

Yup, this works. My preference would be something which I find
slightly more readable though (avoiding assignment in the
conditional):

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index fca888b36680..299dafc7c0ea 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3826,9 +3826,9 @@ static int its_vpe_set_affinity(struct irq_data *d,
 				bool force)
 {
 	struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
-	struct cpumask common, *table_mask;
+	struct cpumask *table_mask;
 	unsigned long flags;
-	int from, cpu;
+	int from, cpu = nr_cpu_ids;
 
 	/*
 	 * Changing affinity is mega expensive, so let's be as lazy as
@@ -3850,10 +3850,15 @@ static int its_vpe_set_affinity(struct irq_data *d,
 	 * If we are offered another CPU in the same GICv4.1 ITS
 	 * affinity, pick this one. Otherwise, any CPU will do.
 	 */
-	if (table_mask && cpumask_and(&common, mask_val, table_mask))
-		cpu = cpumask_test_cpu(from, &common) ? from : cpumask_first(&common);
-	else
+	if (table_mask)
+		cpu = cpumask_any_and(mask_val, table_mask);
+	if (cpu < nr_cpu_ids) {
+		 if (cpumask_test_cpu(from, mask_val) &&
+		     cpumask_test_cpu(from, table_mask))
+			 cpu = from;
+	} else {
 		cpu = cpumask_first(mask_val);
+	}
 
 	if (from == cpu)
 		goto out;

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH 4/6] irqchip/loongson-eiointc: Avoid explicit cpumask allocation on stack

[Dawei Li]

In general it's preferable to avoid placing cpumasks on the stack, as
for large values of NR_CPUS these can consume significant amounts of
stack space and make stack overflows more likely.

Use cpumask_first_and_and() to avoid the need for a temporary cpumask on
the stack.

Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
 drivers/irqchip/irq-loongson-eiointc.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-loongson-eiointc.c b/drivers/irqchip/irq-loongson-eiointc.c
index b64cbe3052e8..c9f30e96b5b5 100644
--- a/drivers/irqchip/irq-loongson-eiointc.c
+++ b/drivers/irqchip/irq-loongson-eiointc.c
@@ -92,19 +92,16 @@ static int eiointc_set_irq_affinity(struct irq_data *d, const struct cpumask *af
 	unsigned int cpu;
 	unsigned long flags;
 	uint32_t vector, regaddr;
-	struct cpumask intersect_affinity;
 	struct eiointc_priv *priv = d->domain->host_data;
 
 	raw_spin_lock_irqsave(&affinity_lock, flags);
 
-	cpumask_and(&intersect_affinity, affinity, cpu_online_mask);
-	cpumask_and(&intersect_affinity, &intersect_affinity, &priv->cpuspan_map);
-
-	if (cpumask_empty(&intersect_affinity)) {
+	cpu = cpumask_first_and_and(&priv->cpuspan_map, affinity,
+				    cpu_online_mask);
+	if (cpu >= nr_cpu_ids) {
 		raw_spin_unlock_irqrestore(&affinity_lock, flags);
 		return -EINVAL;
 	}
-	cpu = cpumask_first(&intersect_affinity);
 
 	vector = d->hwirq;
 	regaddr = EIOINTC_REG_ENABLE + ((vector >> 5) << 2);
-- 
2.27.0

[PATCH 5/6] irqchip/riscv-aplic-direct: Avoid explicit cpumask allocation on stack

[Dawei Li]

In general it's preferable to avoid placing cpumasks on the stack, as
for large values of NR_CPUS these can consume significant amounts of
stack space and make stack overflows more likely.

Use cpumask_first_and_and() to avoid the need for a temporary cpumask on
the stack.

Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
 drivers/irqchip/irq-riscv-aplic-direct.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-riscv-aplic-direct.c b/drivers/irqchip/irq-riscv-aplic-direct.c
index 06bace9b7497..5cb7d74d0927 100644
--- a/drivers/irqchip/irq-riscv-aplic-direct.c
+++ b/drivers/irqchip/irq-riscv-aplic-direct.c
@@ -54,15 +54,13 @@ static int aplic_direct_set_affinity(struct irq_data *d, const struct cpumask *m
 	struct aplic_direct *direct = container_of(priv, struct aplic_direct, priv);
 	struct aplic_idc *idc;
 	unsigned int cpu, val;
-	struct cpumask amask;
 	void __iomem *target;
 
-	cpumask_and(&amask, &direct->lmask, mask_val);
-
 	if (force)
-		cpu = cpumask_first(&amask);
+		cpu = cpumask_first_and(&direct->lmask, mask_val);
 	else
-		cpu = cpumask_any_and(&amask, cpu_online_mask);
+		cpu = cpumask_first_and_and(&direct->lmask, mask_val,
+					    cpu_online_mask);
 
 	if (cpu >= nr_cpu_ids)
 		return -EINVAL;
-- 
2.27.0

[PATCH 6/6] irqchip/sifive-plic: Avoid explicit cpumask allocation on stack

[Dawei Li]

In general it's preferable to avoid placing cpumasks on the stack, as
for large values of NR_CPUS these can consume significant amounts of
stack space and make stack overflows more likely.

Use cpumask_first_and_and() to avoid the need for a temporary cpumask on
the stack.

Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
 drivers/irqchip/irq-sifive-plic.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index f3d4cb9e34f7..bf5d2fc6396e 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -164,15 +164,13 @@ static int plic_set_affinity(struct irq_data *d,
 			     const struct cpumask *mask_val, bool force)
 {
 	unsigned int cpu;
-	struct cpumask amask;
 	struct plic_priv *priv = irq_data_get_irq_chip_data(d);
 
-	cpumask_and(&amask, &priv->lmask, mask_val);
-
 	if (force)
-		cpu = cpumask_first(&amask);
+		cpu = cpumask_first_and(&priv->lmask, mask_val);
 	else
-		cpu = cpumask_any_and(&amask, cpu_online_mask);
+		cpu = cpumask_first_and_and(&priv->lmask, mask_val,
+					    cpu_online_mask);
 
 	if (cpu >= nr_cpu_ids)
 		return -EINVAL;
-- 
2.27.0

Re: [PATCH 6/6] irqchip/sifive-plic: Avoid explicit cpumask allocation on stack

[Yury Norov]

On Fri, Apr 12, 2024 at 06:58:39PM +0800, Dawei Li wrote:
> In general it's preferable to avoid placing cpumasks on the stack, as
> for large values of NR_CPUS these can consume significant amounts of
> stack space and make stack overflows more likely.
> 
> Use cpumask_first_and_and() to avoid the need for a temporary cpumask on
> the stack.
> 
> Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> ---
>  drivers/irqchip/irq-sifive-plic.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index f3d4cb9e34f7..bf5d2fc6396e 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -164,15 +164,13 @@ static int plic_set_affinity(struct irq_data *d,
>  			     const struct cpumask *mask_val, bool force)
>  {
>  	unsigned int cpu;
> -	struct cpumask amask;
>  	struct plic_priv *priv = irq_data_get_irq_chip_data(d);
>  
> -	cpumask_and(&amask, &priv->lmask, mask_val);
> -
>  	if (force)
> -		cpu = cpumask_first(&amask);
> +		cpu = cpumask_first_and(&priv->lmask, mask_val);
>  	else
> -		cpu = cpumask_any_and(&amask, cpu_online_mask);
> +		cpu = cpumask_first_and_and(&priv->lmask, mask_val,
> +					    cpu_online_mask);

Don't need to split the line here. The new max length is 100 chars,
here's 85 when unsplit, and it hurts readability for nothing.

>  
>  	if (cpu >= nr_cpu_ids)
>  		return -EINVAL;
> -- 
> 2.27.0