[PATCH -next 1/3] genirq/affinity: replace managed with is_managed in irq_affinity_desc

[PATCH -next 1/3] genirq/affinity: replace managed with is_managed in irq_affinity_desc

[Xu Qiang]

This submission is based on the following two considerations:

1. The definition of is_managed field is misleading to assume
   that it only uses 1 bit of memory, which is not the case;
2. from the actual use of is_managed, it should be a Boolean type;

Fixes: c410abbbacb9 (“genirq/affinity: Add is_managed to struct irq_affinity_desc”)
Signed-off-by: Xu Qiang <xuqiang36@huawei.com>
---
 include/linux/interrupt.h | 4 ++--
 kernel/irq/affinity.c     | 2 +-
 kernel/irq/irqdesc.c      | 2 +-
 kernel/irq/manage.c       | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index a92bce40b04b..9fb36545c8b2 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -294,11 +294,11 @@ struct irq_affinity {
 /**
  * struct irq_affinity_desc - Interrupt affinity descriptor
  * @mask:	cpumask to hold the affinity assignment
- * @is_managed: 1 if the interrupt is managed internally
+ * @managed:	true if the interrupt is managed internally
  */
 struct irq_affinity_desc {
 	struct cpumask	mask;
-	unsigned int	is_managed : 1;
+	bool		managed;
 };
 
 #if defined(CONFIG_SMP)
diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
index d9a5c1d65a79..2a9fef1713df 100644
--- a/kernel/irq/affinity.c
+++ b/kernel/irq/affinity.c
@@ -483,7 +483,7 @@ irq_create_affinity_masks(unsigned int nvecs, struct irq_affinity *affd)
 
 	/* Mark the managed interrupts */
 	for (i = affd->pre_vectors; i < nvecs - affd->post_vectors; i++)
-		masks[i].is_managed = 1;
+		masks[i].managed = true;
 
 	return masks;
 }
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 5db0230aa6b5..8388b821e58b 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -486,7 +486,7 @@ static int alloc_descs(unsigned int start, unsigned int cnt, int node,
 		unsigned int flags = 0;
 
 		if (affinity) {
-			if (affinity->is_managed) {
+			if (affinity->managed) {
 				flags = IRQD_AFFINITY_MANAGED |
 					IRQD_MANAGED_SHUTDOWN;
 			}
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 40fe7806cc8c..c3423f552e0b 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -431,7 +431,7 @@ int irq_update_affinity_desc(unsigned int irq,
 	if (activated)
 		irq_domain_deactivate_irq(&desc->irq_data);
 
-	if (affinity->is_managed) {
+	if (affinity->managed) {
 		irqd_set(&desc->irq_data, IRQD_AFFINITY_MANAGED);
 		irqd_set(&desc->irq_data, IRQD_MANAGED_SHUTDOWN);
 	}
-- 
2.17.1

[PATCH -next 2/3] genirq/affinity: Define tmp_mask as a local variable in irq_do_set_affinity

[Xu Qiang]

When irq_do_set_affinity is called, tmp_mask saved last time
does not make any sense. it is reassigned before each use,
so it should be defined as a local variable.

Fixes: 33de0aa4bae9 (“genirq: Always limit the affinity to online CPUs”)
Signed-off-by: Xu Qiang <xuqiang36@huawei.com>
---
 kernel/irq/manage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index c3423f552e0b..ae1c7eebdfa6 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -218,7 +218,7 @@ int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
 	int ret;
 
 	static DEFINE_RAW_SPINLOCK(tmp_mask_lock);
-	static struct cpumask tmp_mask;
+	struct cpumask tmp_mask;
 
 	if (!chip || !chip->irq_set_affinity)
 		return -EINVAL;
-- 
2.17.1

[PATCH -next 3/3] genirq/affinity: Add __irq_do_set_affinity_lock function.

[Xu Qiang]

To make irq_do_set_affinity more concise and clear, force argument
is handled first, and then non-force scenarios are handled in
the __irq_do_set_affinity_lock function.

Signed-off-by: Xu Qiang <xuqiang36@huawei.com>
---
 kernel/irq/manage.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index ae1c7eebdfa6..1fa8ef781736 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -209,21 +209,14 @@ static void irq_validate_effective_affinity(struct irq_data *data)
 static inline void irq_validate_effective_affinity(struct irq_data *data) { }
 #endif
 
-int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
-			bool force)
+static int __irq_do_set_affinity_lock(struct irq_data *data, const struct cpumask *mask)
 {
-	struct irq_desc *desc = irq_data_to_desc(data);
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
 	const struct cpumask  *prog_mask;
 	int ret;
 
-	static DEFINE_RAW_SPINLOCK(tmp_mask_lock);
 	struct cpumask tmp_mask;
 
-	if (!chip || !chip->irq_set_affinity)
-		return -EINVAL;
-
-	raw_spin_lock(&tmp_mask_lock);
 	/*
 	 * If this is a managed interrupt and housekeeping is enabled on
 	 * it check whether the requested affinity mask intersects with
@@ -264,13 +257,31 @@ int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
 	 * case we do as we are told).
 	 */
 	cpumask_and(&tmp_mask, prog_mask, cpu_online_mask);
-	if (!force && !cpumask_empty(&tmp_mask))
-		ret = chip->irq_set_affinity(data, &tmp_mask, force);
-	else if (force)
-		ret = chip->irq_set_affinity(data, mask, force);
+	if (!cpumask_empty(&tmp_mask))
+		ret = chip->irq_set_affinity(data, &tmp_mask, false);
 	else
 		ret = -EINVAL;
 
+	return ret;
+}
+
+int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
+			bool force)
+{
+	struct irq_desc *desc = irq_data_to_desc(data);
+	struct irq_chip *chip = irq_data_get_irq_chip(data);
+	int ret;
+
+	static DEFINE_RAW_SPINLOCK(tmp_mask_lock);
+
+	if (!chip || !chip->irq_set_affinity)
+		return -EINVAL;
+
+	raw_spin_lock(&tmp_mask_lock);
+	if (force)
+		ret = chip->irq_set_affinity(data, mask, force);
+	else
+		ret = __irq_do_set_affinity_lock(data, mask);
 	raw_spin_unlock(&tmp_mask_lock);
 
 	switch (ret) {
-- 
2.17.1

Re: [PATCH -next 1/3] genirq/affinity: replace managed with is_managed in irq_affinity_desc

[Marc Zyngier]

On Sat, 27 Aug 2022 02:13:49 +0100,
Xu Qiang <xuqiang36@huawei.com> wrote:
> 
> This submission is based on the following two considerations:
> 
> 1. The definition of is_managed field is misleading to assume
>    that it only uses 1 bit of memory, which is not the case;

You realise that a bitfield is not about the memory used, but the
number of significant bits, right? The memory it uses is the
compiler's business.

> 2. from the actual use of is_managed, it should be a Boolean type;

Why? What is wrong with the existing bitfield? Why renaming it?

>
> Fixes: c410abbbacb9 (“genirq/affinity: Add is_managed to struct irq_affinity_desc”)

I don't see any fix here, only some seemingly pointless bike-shedding.
If you have identified an actual issue, please spell it out for me,
because I cannot see it.

Thanks,

	M.


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

Re: [PATCH -next 2/3] genirq/affinity: Define tmp_mask as a local variable in irq_do_set_affinity

[Marc Zyngier]

On Sat, 27 Aug 2022 02:13:50 +0100,
Xu Qiang <xuqiang36@huawei.com> wrote:
> 
> When irq_do_set_affinity is called, tmp_mask saved last time
> does not make any sense. it is reassigned before each use,
> so it should be defined as a local variable.
> 
> Fixes: 33de0aa4bae9 (“genirq: Always limit the affinity to online CPUs”)
> Signed-off-by: Xu Qiang <xuqiang36@huawei.com>
> ---
>  kernel/irq/manage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index c3423f552e0b..ae1c7eebdfa6 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -218,7 +218,7 @@ int irq_do_set_affinity(struct irq_data *data, const struct cpumask *mask,
>  	int ret;
>  
>  	static DEFINE_RAW_SPINLOCK(tmp_mask_lock);
> -	static struct cpumask tmp_mask;
> +	struct cpumask tmp_mask;
>  
>  	if (!chip || !chip->irq_set_affinity)
>  		return -EINVAL;

Oh Gawd... Don't you see *WHY* this is a static structure? Hint: what
is the effect of this on a machine with 4096 CPUs when the stack space
is tight? And what would be the point of having a spinlock to protect
a local variable?

	M.


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