[PATCH] genirq: Keep affinity_hint unchanged if it has a value

Yajun Deng posted 1 patch 11 months ago
include/linux/irqdesc.h |  2 +-
kernel/irq/irqdesc.c    |  9 +++++++++
kernel/irq/manage.c     |  9 ++-------
kernel/irq/proc.c       | 15 +--------------
4 files changed, 13 insertions(+), 22 deletions(-)
[PATCH] genirq: Keep affinity_hint unchanged if it has a value
Posted by Yajun Deng 11 months ago
affinity_hint is a hint to user space for preferred irq affinity, but
it could chang if the value it points to is changed. In other words,
the hint is invalid.

For example, if affinity_hint points to smp_affinity, smp_affinity
is changed by the user, and affinity_hint would chang. affinity_hint
couldn't as a hint to the user, it should keep the value if it has.

Allocate memory in alloc_masks(), and keep it unchanged if it has a
value in __irq_apply_affinity_hint().

Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
 include/linux/irqdesc.h |  2 +-
 kernel/irq/irqdesc.c    |  9 +++++++++
 kernel/irq/manage.c     |  9 ++-------
 kernel/irq/proc.c       | 15 +--------------
 4 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index fd091c35d572..f6363ba435c6 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -84,7 +84,7 @@ struct irq_desc {
 	struct cpumask		*percpu_enabled;
 	const struct cpumask	*percpu_affinity;
 #ifdef CONFIG_SMP
-	const struct cpumask	*affinity_hint;
+	struct cpumask		*affinity_hint;
 	struct irq_affinity_notify *affinity_notify;
 #ifdef CONFIG_GENERIC_PENDING_IRQ
 	cpumask_var_t		pending_mask;
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 287830739783..e4f87826b7c6 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -58,9 +58,16 @@ static int alloc_masks(struct irq_desc *desc, int node)
 				     GFP_KERNEL, node))
 		return -ENOMEM;
 
+	if (!zalloc_cpumask_var_node(&desc->affinity_hint,
+				     GFP_KERNEL, node)) {
+		free_cpumask_var(desc->irq_common_data.affinity);
+		return -ENOMEM;
+	}
+
 #ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 	if (!zalloc_cpumask_var_node(&desc->irq_common_data.effective_affinity,
 				     GFP_KERNEL, node)) {
+		free_cpumask_var(desc->affinity_hint);
 		free_cpumask_var(desc->irq_common_data.affinity);
 		return -ENOMEM;
 	}
@@ -71,6 +78,7 @@ static int alloc_masks(struct irq_desc *desc, int node)
 #ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 		free_cpumask_var(desc->irq_common_data.effective_affinity);
 #endif
+		free_cpumask_var(desc->affinity_hint);
 		free_cpumask_var(desc->irq_common_data.affinity);
 		return -ENOMEM;
 	}
@@ -98,6 +106,7 @@ static void free_masks(struct irq_desc *desc)
 #ifdef CONFIG_GENERIC_PENDING_IRQ
 	free_cpumask_var(desc->pending_mask);
 #endif
+	free_cpumask_var(desc->affinity_hint);
 	free_cpumask_var(desc->irq_common_data.affinity);
 #ifdef CONFIG_GENERIC_IRQ_EFFECTIVE_AFF_MASK
 	free_cpumask_var(desc->irq_common_data.effective_affinity);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f300bb6be3bd..49500af3effa 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -507,7 +507,8 @@ int __irq_apply_affinity_hint(unsigned int irq, const struct cpumask *m,
 
 	if (!desc)
 		return -EINVAL;
-	desc->affinity_hint = m;
+	if (m && cpumask_empty(desc->affinity_hint))
+		cpumask_copy(desc->affinity_hint, m);
 	irq_put_desc_unlock(desc, flags);
 	if (m && setaffinity)
 		__irq_set_affinity(irq, m, false);
@@ -1914,12 +1915,6 @@ static struct irqaction *__free_irq(struct irq_desc *desc, void *dev_id)
 		irq_shutdown(desc);
 	}
 
-#ifdef CONFIG_SMP
-	/* make sure affinity_hint is cleaned up */
-	if (WARN_ON_ONCE(desc->affinity_hint))
-		desc->affinity_hint = NULL;
-#endif
-
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 	/*
 	 * Drop bus_lock here so the changes which were done in the chip
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 8e29809de38d..b4d5932a1a1a 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -81,20 +81,7 @@ static int show_irq_affinity(int type, struct seq_file *m)
 static int irq_affinity_hint_proc_show(struct seq_file *m, void *v)
 {
 	struct irq_desc *desc = irq_to_desc((long)m->private);
-	unsigned long flags;
-	cpumask_var_t mask;
-
-	if (!zalloc_cpumask_var(&mask, GFP_KERNEL))
-		return -ENOMEM;
-
-	raw_spin_lock_irqsave(&desc->lock, flags);
-	if (desc->affinity_hint)
-		cpumask_copy(mask, desc->affinity_hint);
-	raw_spin_unlock_irqrestore(&desc->lock, flags);
-
-	seq_printf(m, "%*pb\n", cpumask_pr_args(mask));
-	free_cpumask_var(mask);
-
+	seq_printf(m, "%*pb\n", cpumask_pr_args(desc->affinity_hint));
 	return 0;
 }
 
-- 
2.25.1
Re: [PATCH] genirq: Keep affinity_hint unchanged if it has a value
Posted by kernel test robot 11 months ago
Hi Yajun,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/irq/core]
[also build test ERROR on linus/master v6.14-rc6 next-20250311]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yajun-Deng/genirq-Keep-affinity_hint-unchanged-if-it-has-a-value/20250311-093633
base:   tip/irq/core
patch link:    https://lore.kernel.org/r/20250311013352.2727490-1-yajun.deng%40linux.dev
patch subject: [PATCH] genirq: Keep affinity_hint unchanged if it has a value
config: s390-randconfig-002-20250312 (https://download.01.org/0day-ci/archive/20250312/202503120334.Dk9gw6LX-lkp@intel.com/config)
compiler: clang version 16.0.6 (https://github.com/llvm/llvm-project 7cbf1a2591520c2491aa35339f227775f4d3adf6)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250312/202503120334.Dk9gw6LX-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503120334.Dk9gw6LX-lkp@intel.com/

All errors (new ones prefixed by >>):

>> kernel/irq/irqdesc.c:61:31: error: incompatible pointer types passing 'struct cpumask **' to parameter of type 'cpumask_var_t *' (aka 'struct cpumask (*)[1]') [-Werror,-Wincompatible-pointer-types]
           if (!zalloc_cpumask_var_node(&desc->affinity_hint,
                                        ^~~~~~~~~~~~~~~~~~~~
   include/linux/cpumask.h:996:68: note: passing argument to parameter 'mask' here
   static __always_inline bool zalloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags,
                                                                      ^
   1 error generated.


vim +61 kernel/irq/irqdesc.c

    53	
    54	#ifdef CONFIG_SMP
    55	static int alloc_masks(struct irq_desc *desc, int node)
    56	{
    57		if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity,
    58					     GFP_KERNEL, node))
    59			return -ENOMEM;
    60	
  > 61		if (!zalloc_cpumask_var_node(&desc->affinity_hint,
    62					     GFP_KERNEL, node)) {
    63			free_cpumask_var(desc->irq_common_data.affinity);
    64			return -ENOMEM;
    65		}
    66	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH] genirq: Keep affinity_hint unchanged if it has a value
Posted by kernel test robot 11 months ago
Hi Yajun,

kernel test robot noticed the following build errors:

[auto build test ERROR on tip/irq/core]
[also build test ERROR on linus/master v6.14-rc6 next-20250311]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yajun-Deng/genirq-Keep-affinity_hint-unchanged-if-it-has-a-value/20250311-093633
base:   tip/irq/core
patch link:    https://lore.kernel.org/r/20250311013352.2727490-1-yajun.deng%40linux.dev
patch subject: [PATCH] genirq: Keep affinity_hint unchanged if it has a value
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20250312/202503120232.2RVdFOyX-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250312/202503120232.2RVdFOyX-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503120232.2RVdFOyX-lkp@intel.com/

All errors (new ones prefixed by >>):

   kernel/irq/irqdesc.c: In function 'alloc_masks':
>> kernel/irq/irqdesc.c:61:38: error: passing argument 1 of 'zalloc_cpumask_var_node' from incompatible pointer type [-Werror=incompatible-pointer-types]
      61 |         if (!zalloc_cpumask_var_node(&desc->affinity_hint,
         |                                      ^~~~~~~~~~~~~~~~~~~~
         |                                      |
         |                                      struct cpumask **
   In file included from include/linux/smp.h:13,
                    from include/linux/lockdep.h:14,
                    from include/linux/spinlock.h:63,
                    from include/linux/irq.h:14,
                    from kernel/irq/irqdesc.c:10:
   include/linux/cpumask.h:996:68: note: expected 'struct cpumask (*)[1]' but argument is of type 'struct cpumask **'
     996 | static __always_inline bool zalloc_cpumask_var_node(cpumask_var_t *mask, gfp_t flags,
         |                                                     ~~~~~~~~~~~~~~~^~~~
   cc1: some warnings being treated as errors


vim +/zalloc_cpumask_var_node +61 kernel/irq/irqdesc.c

    53	
    54	#ifdef CONFIG_SMP
    55	static int alloc_masks(struct irq_desc *desc, int node)
    56	{
    57		if (!zalloc_cpumask_var_node(&desc->irq_common_data.affinity,
    58					     GFP_KERNEL, node))
    59			return -ENOMEM;
    60	
  > 61		if (!zalloc_cpumask_var_node(&desc->affinity_hint,
    62					     GFP_KERNEL, node)) {
    63			free_cpumask_var(desc->irq_common_data.affinity);
    64			return -ENOMEM;
    65		}
    66	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH] genirq: Keep affinity_hint unchanged if it has a value
Posted by Thomas Gleixner 11 months ago
On Tue, Mar 11 2025 at 09:33, Yajun Deng wrote:
> affinity_hint is a hint to user space for preferred irq affinity, but
> it could chang if the value it points to is changed. In other words,
> the hint is invalid.

That's a problem of the driver which provides the hint.
>
> For example, if affinity_hint points to smp_affinity, smp_affinity
> is changed by the user, and affinity_hint would chang. affinity_hint
> couldn't as a hint to the user, it should keep the value if it has.

What is 'smp_affinity'? 

I really fail to understand the problem you are trying to solve.

Thanks,

        tglx
Re: [PATCH] genirq: Keep affinity_hint unchanged if it has a value
Posted by Yajun Deng 11 months ago
March 11, 2025 at 10:14 PM, "Thomas Gleixner" <tglx@linutronix.de> wrote:



> 
> On Tue, Mar 11 2025 at 09:33, Yajun Deng wrote:
> 
> > 
> > affinity_hint is a hint to user space for preferred irq affinity, but
> > 
> >  it could chang if the value it points to is changed. In other words,
> > 
> >  the hint is invalid.
> > 
> 
> That's a problem of the driver which provides the hint.
> 
> > 
> > For example, if affinity_hint points to smp_affinity, smp_affinity
> > 
> >  is changed by the user, and affinity_hint would chang. affinity_hint
> > 
> >  couldn't as a hint to the user, it should keep the value if it has.
> > 
> 
> What is 'smp_affinity'? 

It's 'desc->irq_common_data.affinity'.
> 
> I really fail to understand the problem you are trying to solve.

irq_update_affinity_hint(irq, desc->irq_common_data.affinity);

As in this code, this means affinity_hint points to smp_affinity.

If someone were to bind several irqs to the same core, affinity_hint would change.

it couldn't as a hint to the user.

This patch allocates memory for affinity_hint and keeps the first value, even if someone

modifies smp_affinity, affinity_hint wouldn't change, it can as a hint to the user.


> 
> Thanks,
> 
>  tglx
>
Re: [PATCH] genirq: Keep affinity_hint unchanged if it has a value
Posted by Thomas Gleixner 11 months ago
On Wed, Mar 12 2025 at 03:20, Yajun Deng wrote:
> March 11, 2025 at 10:14 PM, "Thomas Gleixner" <tglx@linutronix.de> wrote:
>> > For example, if affinity_hint points to smp_affinity, smp_affinity
>> >  is changed by the user, and affinity_hint would chang. affinity_hint
>> >  couldn't as a hint to the user, it should keep the value if it has.
>> 
>> What is 'smp_affinity'? 
>
> It's 'desc->irq_common_data.affinity'.

Which has no business to end up in the affinity hint.
 
>> I really fail to understand the problem you are trying to solve.
>
> irq_update_affinity_hint(irq, desc->irq_common_data.affinity);
>
> As in this code, this means affinity_hint points to smp_affinity.

This code does not exist and no driver should ever invoke
irq_update_affinity_hint() that way. The hint is a cpu mask managed by
the driver and not something the driver pulls out of a data structure
which it has no business to fiddle with.

Thanks

        tglx
Re: [PATCH] genirq: Keep affinity_hint unchanged if it has a value
Posted by Yajun Deng 11 months ago
March 12, 2025 at 5:10 PM, "Thomas Gleixner" <tglx@linutronix.de mailto:tglx@linutronix.de?to=%22Thomas%20Gleixner%22%20%3Ctglx%40linutronix.de%3E > wrote:


> 
> On Wed, Mar 12 2025 at 03:20, Yajun Deng wrote:
> 
> > 
> > March 11, 2025 at 10:14 PM, "Thomas Gleixner" <tglx@linutronix.de> wrote:
> > 
> > > 
> > > > For example, if affinity_hint points to smp_affinity, smp_affinity
> > >  > is changed by the user, and affinity_hint would chang. affinity_hint
> > >  > couldn't as a hint to the user, it should keep the value if it has.
> > >  
> > >  What is 'smp_affinity'?
> > > 
> >  It's 'desc->irq_common_data.affinity'.
> > 
> Which has no business to end up in the affinity hint.
>  
> 
> > 
> > > 
> > > I really fail to understand the problem you are trying to solve.
> > > 
> >  irq_update_affinity_hint(irq, desc->irq_common_data.affinity);
> > 
> >  As in this code, this means affinity_hint points to smp_affinity.
> > 
> This code does not exist and no driver should ever invoke
> irq_update_affinity_hint() that way. The hint is a cpu mask managed by
> the driver and not something the driver pulls out of a data structure
> which it has no business to fiddle with.
>

Okay, I got it. Thank you!
 
> Thanks
> 
>  tglx
>