[RFC PATCH 0/5] irq: sysfs interface improvements for SMP affinity control

[RFC PATCH 0/5] irq: sysfs interface improvements for SMP affinity control

[Radu Rendec]

This patch set implements new sysfs interfaces that facilitate SMP
affinity control of chained interrupts. It follows the guidelines in
https://lore.kernel.org/all/87fslr7ygl.wl-maz@kernel.org/ with slight
deviations, which are explained below.

The assumption is that irqbalance must be aware of the chained interrupt
topology regardless of how it is exposed to userspace, for the following
reasons:
- Interrupt counters are not updated for the parent interrupt. Counters
  must be read separately for each of the chained interrupts and summed
  up to assess the CPU usage impact of the group as a whole.
- The affinity setting is shared by all multiplexed interrupts (and the
  parent interrupt) and cannot be changed individually.

Since irqbalance must be aware of the topology anyway, it is easier to
move parts of the problem there and reduce the complexity of the kernel
changes.
- Instead of creating a new affinity interface for chained interrupts
  that has different semantics from the existing procfs interface (and
  changes the affinity of the parent interrupt in the case of muxed
  interrupts), it is easier to let irqbalance set the affinity of the
  parent interrupt by itself (since it already knows who the parent is).
- Tracking groups of interrupts in the kernel creates additional
  synchronization challenges that are otherwise unnecessary. The kernel
  already has a (struct irq_desc).parent_irq field that can be (re)used
  for this purpose (see below).

Brief description of the patches in this set:
- Patch 1/5 makes the (struct irq_desc).parent_irq field available
  unconditionally. So far, it has been used for IRQ-resend and depended
  on CONFIG_HARDIRQS_SW_RESEND. But it can be (re)used to track chained
  interrupt parents for the general use case, without any changes to the
  existing IRQ chip drivers.
- Patch 2/5 is trivial and just exposes (struct irq_desc).parent_irq in
  debugfs.
- Patch 3/5 exposes the chained interrupt topology in sysfs in two ways:
  the muxed_irqs directory (as described in the original email thread)
  and the parent_irq symlink. From a userspace perspective, they are
  redundant. However, in the first case the synchronization is likely
  incomplete/broken and not so easy to fix.
- Patch 4/5 moves the SMP affinity write handlers from procfs code to
  generic code, with the intention to reuse them for a new sysfs
  interface.
- Patch 5/5 creates a sysfs interface for the affinity, with identical
  semantics to the existing procfs interface. The sole purpose is to
  allow userspace (irqbalance) to control the affinity of the parent
  interrupt, which is typically *not* visible in procfs.

The only required change to existing chained IRQ chip drivers in order
to support the new affinity control is to call irq_set_parent() in their
.map domain op. If they use the newer hierarchical API, they should call
irq_set_parent() in their .alloc domain op instead. This doesn't affect
the existing procfs based affinity interface in any way.

A few IRQ chip drivers already call irq_set_parent() in their .map
domain op to implement IRQ-resend. No change is required to those
drivers to support the new affinity control.

Last but not least, it turns out that hierarchical domains are entirely
out of the scope of these changes (unless chained interrupts are used
along the path). In the case of hierarchical domains, each interrupt in
the outermost domain has a *single* corresponding Linux virq (that is
mapped to each domain in the hierarchy). That makes it perfectly safe to
implement the .irq_set_affinity chip op as irq_chip_set_affinity_parent
and delegate affinity control to the parent chip/domain. This will *not*
suddenly change the affinity of a different interrupt behind anyone's
back simply because there cannot be another interrupt that shares the
same affinity setting.

Note: I still need to update the Documentation/ directory for the new
      sysfs interface, and I will address that in a future version.
      At this point, I just want to get feedback about the current
      approach.

Radu Rendec (5):
  irq: Always enable parent interrupt tracking
  irq: Show the parent chained interrupt in debugfs
  irq: Expose chained interrupt parents in sysfs
  irq: Move SMP affinity write handler out of proc.c
  irq: Add smp_affinity/list attributes to sysfs

 include/linux/irq.h     |   9 +-
 include/linux/irqdesc.h |   1 +
 kernel/irq/debugfs.c    |   1 +
 kernel/irq/internals.h  |  10 ++
 kernel/irq/irqdesc.c    | 206 +++++++++++++++++++++++++++++++++++++---
 kernel/irq/irqdomain.c  |  15 +++
 kernel/irq/manage.c     |  20 +++-
 kernel/irq/proc.c       |  72 +-------------
 8 files changed, 244 insertions(+), 90 deletions(-)

-- 
2.40.1

[RFC PATCH 1/5] irq: Always enable parent interrupt tracking

[Radu Rendec]

The kernel already has some support for tracking the parent interrupt in
the case of chained interrupts, but it is currently used only by the
IRQ-resend code.

This patch enables the parent interrupt tracking code unconditionally.
The IRQ-resend code still depends on CONFIG_HARDIRQS_SW_RESEND.

The intention is to (re)use the existing parent interrupt tracking
support for different purposes, more specifically to expose chained
interrupt topology to userspace. That, in turn, makes it possible to
control the SMP affinity of chained interrupts in a way that does not
break the existing interface and the promises it makes.

Signed-off-by: Radu Rendec <rrendec@redhat.com>
---
 include/linux/irq.h | 7 -------
 kernel/irq/manage.c | 2 --
 2 files changed, 9 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index b1b28affb32a7..7710f157e12de 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -641,14 +641,7 @@ static inline void irq_force_complete_move(struct irq_desc *desc) { }
 
 extern int no_irq_affinity;
 
-#ifdef CONFIG_HARDIRQS_SW_RESEND
 int irq_set_parent(int irq, int parent_irq);
-#else
-static inline int irq_set_parent(int irq, int parent_irq)
-{
-	return 0;
-}
-#endif
 
 /*
  * Built-in IRQ handlers for various IRQ types,
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index eb862b5f91c42..49683e55261eb 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1004,7 +1004,6 @@ int __irq_set_trigger(struct irq_desc *desc, unsigned long flags)
 	return ret;
 }
 
-#ifdef CONFIG_HARDIRQS_SW_RESEND
 int irq_set_parent(int irq, int parent_irq)
 {
 	unsigned long flags;
@@ -1019,7 +1018,6 @@ int irq_set_parent(int irq, int parent_irq)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(irq_set_parent);
-#endif
 
 /*
  * Default primary interrupt handler for threaded interrupts. Is
-- 
2.40.1

[RFC PATCH 2/5] irq: Show the parent chained interrupt in debugfs

[Radu Rendec]

This is a trivial change to expose the parent chained interrupt. The
intention is to make it easier to debug chained interrupts, particularly
in the context of setting the SMP affinity.

Signed-off-by: Radu Rendec <rrendec@redhat.com>
---
 kernel/irq/debugfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/debugfs.c b/kernel/irq/debugfs.c
index bbcaac64038ef..3ada976df8612 100644
--- a/kernel/irq/debugfs.c
+++ b/kernel/irq/debugfs.c
@@ -177,6 +177,7 @@ static int irq_debug_show(struct seq_file *m, void *p)
 			    ARRAY_SIZE(irqdesc_istates));
 	seq_printf(m, "ddepth:   %u\n", desc->depth);
 	seq_printf(m, "wdepth:   %u\n", desc->wake_depth);
+	seq_printf(m, "parent:   %d\n", desc->parent_irq);
 	seq_printf(m, "dstate:   0x%08x\n", irqd_get(data));
 	irq_debug_show_bits(m, 0, irqd_get(data), irqdata_states,
 			    ARRAY_SIZE(irqdata_states));
-- 
2.40.1

[RFC PATCH 3/5] irq: Expose chained interrupt parents in sysfs

[Radu Rendec]

This patch extends irq_set_parent() by adding the ability to show the
direct parent of a chained interrupt as a symlink in sysfs. In addition,
the root level interrupt (in a multi-level chained interrupt topology)
will also show all child interrupts. Example:

$ ls -l /sys/kernel/irq/38/muxed_irqs/
total 0
lrwxrwxrwx 1 root root 0 May 24 16:00 41 -> ../../41
lrwxrwxrwx 1 root root 0 May 24 16:00 42 -> ../../42
$ ls -l /sys/kernel/irq/41/parent
lrwxrwxrwx 1 root root 0 May 24 16:00 /sys/kernel/irq/41/parent -> ../38
$ ls -l /sys/kernel/irq/42/parent
lrwxrwxrwx 1 root root 0 May 24 16:18 /sys/kernel/irq/42/parent -> ../38

Chained IRQ chip drivers are expected to call irq_set_parent() in their
.map domain op handler. A few already do (for the purpose of enabling
IRQ-resend support).

An IRQ chip driver may also implement the newer hierarchical domain API
and still use chained interrupts. In that case, irq_set_parent() would
be called in the .alloc domain op handler.

Since most legacy drivers do not implement the .unmap domain op and most
hierarchical drivers use irq_domain_free_irqs_common() as their .free
domain op, the irqdomain core is modified to remove an existing parent
mapping automatically when the interrupt is disassociated with the
domain.

The whole purpose of exposing interrupt topology in sysfs is to be able
to control the SMP affinity of chained interrupts. Since chained IRQ
chips can be stacked and the affinity can be typically controlled only
at the root level, irq_set_parent() will always populate the muxed_irqs
directory of the root interrupt, not the one of the direct parent.
However, the "parent" symlink will always point to the direct parent.

Notes (RFC/draft patch):
* The muxed_irqs directory may not be necessary to control the SMP
  affinity. See the cover letter for details.
* Synchronization around muxed_irqs is likely broken. But since we may
  decide to get rid of muxed_irqs altogether, the implementation here is
  provided as a proof of concept and ground for discussion.

Signed-off-by: Radu Rendec <rrendec@redhat.com>
---
 include/linux/irqdesc.h |  1 +
 kernel/irq/internals.h  |  8 ++++
 kernel/irq/irqdesc.c    | 84 +++++++++++++++++++++++++++++++++++------
 kernel/irq/irqdomain.c  | 15 ++++++++
 kernel/irq/manage.c     | 16 ++++++--
 5 files changed, 110 insertions(+), 14 deletions(-)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 844a8e30e6de5..b57450745857f 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -97,6 +97,7 @@ struct irq_desc {
 #ifdef CONFIG_SPARSE_IRQ
 	struct rcu_head		rcu;
 	struct kobject		kobj;
+	struct kobject		*muxed_irqs;
 #endif
 	struct mutex		request_mutex;
 	int			parent_irq;
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 5fdc0b5575797..c75cd836155c9 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -101,6 +101,14 @@ static inline void irq_mark_irq(unsigned int irq) { }
 extern void irq_mark_irq(unsigned int irq);
 #endif
 
+#if defined(CONFIG_SPARSE_IRQ) && defined(CONFIG_SYSFS)
+extern void irq_sysfs_update_parent(struct irq_desc *desc,
+				    struct irq_desc *parent);
+#else
+static inline void irq_sysfs_update_parent(struct irq_desc *desc,
+					   struct irq_desc *parent) { }
+#endif
+
 extern int __irq_get_irqchip_state(struct irq_data *data,
 				   enum irqchip_irq_state which,
 				   bool *state);
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index d45c163dbb749..ec52b8b41002e 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -285,17 +285,27 @@ static const struct kobj_type irq_kobj_type = {
 
 static void irq_sysfs_add(int irq, struct irq_desc *desc)
 {
-	if (irq_kobj_base) {
-		/*
-		 * Continue even in case of failure as this is nothing
-		 * crucial and failures in the late irq_sysfs_init()
-		 * cannot be rolled back.
-		 */
-		if (kobject_add(&desc->kobj, irq_kobj_base, "%d", irq))
-			pr_warn("Failed to add kobject for irq %d\n", irq);
-		else
-			desc->istate |= IRQS_SYSFS;
+	if (!irq_kobj_base)
+		return;
+
+	/*
+	 * Continue even in case of failure as this is nothing
+	 * crucial and failures in the late irq_sysfs_init()
+	 * cannot be rolled back.
+	 */
+	if (kobject_add(&desc->kobj, irq_kobj_base, "%d", irq)) {
+		pr_warn("Failed to add kobject for irq %d\n", irq);
+		return;
+	}
+
+	desc->muxed_irqs = kobject_create_and_add("muxed_irqs", &desc->kobj);
+	if (!desc->muxed_irqs) {
+		pr_warn("Failed to add mux kobject for irq %d\n", irq);
+		kobject_del(&desc->kobj);
+		return;
 	}
+
+	desc->istate |= IRQS_SYSFS;
 }
 
 static void irq_sysfs_del(struct irq_desc *desc)
@@ -306,8 +316,10 @@ static void irq_sysfs_del(struct irq_desc *desc)
 	 * sysfs is not initialized yet, and the case of a failed
 	 * kobject_add() invocation.
 	 */
-	if (desc->istate & IRQS_SYSFS)
+	if (desc->istate & IRQS_SYSFS) {
+		kobject_del(desc->muxed_irqs);
 		kobject_del(&desc->kobj);
+	}
 }
 
 static int __init irq_sysfs_init(void)
@@ -333,6 +345,55 @@ static int __init irq_sysfs_init(void)
 }
 postcore_initcall(irq_sysfs_init);
 
+static inline struct irq_desc *irq_find_root_desc(struct irq_desc *desc)
+{
+	while (desc && desc->parent_irq)
+		desc = irq_to_desc(desc->parent_irq);
+
+	return desc;
+}
+
+/*
+ * Update sysfs entries to show that parent_desc is the new parent of desc
+ *
+ * At this point, desc->parent_irq is the old parent, and parent_desc is the
+ * new parent. If no parent has been set yet, desc->parent is 0. If the parent
+ * is being removed, parent_desc is NULL.
+ *
+ * Called with desc->request_mutex locked.
+ */
+void irq_sysfs_update_parent(struct irq_desc *desc, struct irq_desc *parent_desc)
+{
+	unsigned int irq = desc->irq_data.irq;
+	struct irq_desc *root_desc;
+	int err;
+
+	/* First, remove all references to the old parent (if any). */
+	if (desc->parent_irq) {
+		sysfs_remove_link(&desc->kobj, "parent");
+		root_desc = irq_find_root_desc(irq_to_desc(desc->parent_irq));
+		if (!WARN_ON(!root_desc))
+			sysfs_remove_link(root_desc->muxed_irqs, desc->kobj.name);
+	}
+
+	/* Next, create references to the new parent (if any). */
+	if (!parent_desc)
+		return;
+
+	err = sysfs_create_link(&desc->kobj, &parent_desc->kobj, "parent");
+	if (err)
+		pr_warn("Failed to link irq %u parent: %d\n", irq, err);
+
+	root_desc = irq_find_root_desc(parent_desc);
+	if (WARN_ON(!root_desc))
+		return;
+
+	err = sysfs_create_link(root_desc->muxed_irqs, &desc->kobj,
+				desc->kobj.name);
+	if (err)
+		pr_warn("Failed to link irq %u root: %d\n", irq, err);
+}
+
 #else /* !CONFIG_SYSFS */
 
 static const struct kobj_type irq_kobj_type = {
@@ -438,6 +499,7 @@ static void delayed_free_desc(struct rcu_head *rhp)
 {
 	struct irq_desc *desc = container_of(rhp, struct irq_desc, rcu);
 
+	kobject_put(desc->muxed_irqs);
 	kobject_put(&desc->kobj);
 }
 
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index f34760a1e2226..fad1559587c3e 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -553,6 +553,14 @@ static void irq_domain_disassociate(struct irq_domain *domain, unsigned int irq)
 
 	mutex_lock(&domain->root->mutex);
 
+	/*
+	 * For domains that support it, irq_set_parent() is called in the map()
+	 * op. The reverse operation of removing the parent mapping should be
+	 * done in the unmap() op which is called below. Since most domains do
+	 * not implement unmap(), remove the parent mapping here.
+	 */
+	irq_set_parent(irq, 0);
+
 	irq_set_status_flags(irq, IRQ_NOREQUEST);
 
 	/* remove chip and handler */
@@ -1748,6 +1756,13 @@ void irq_domain_free_irqs(unsigned int virq, unsigned int nr_irqs)
 	domain = data->domain;
 
 	mutex_lock(&domain->root->mutex);
+	/*
+	 * Typically irq_set_parent() would be called in the alloc() op. The
+	 * reverse operation of removing the parent mapping should be done in
+	 * the free() op which is called indirectly below. Since most domains
+	 * use irq_domain_free_irqs_common(), remove the parent mapping here.
+	 */
+	irq_set_parent(virq, 0);
 	for (i = 0; i < nr_irqs; i++)
 		irq_domain_remove_irq(virq + i);
 	irq_domain_free_irqs_hierarchy(domain, virq, nr_irqs);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 49683e55261eb..eec9b94747439 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1007,14 +1007,24 @@ int __irq_set_trigger(struct irq_desc *desc, unsigned long flags)
 int irq_set_parent(int irq, int parent_irq)
 {
 	unsigned long flags;
-	struct irq_desc *desc = irq_get_desc_lock(irq, &flags, 0);
+	struct irq_desc *desc = irq_to_desc(irq);
+	struct irq_desc *parent_desc = parent_irq ?
+				       irq_to_desc(parent_irq) : NULL;
 
-	if (!desc)
+	if (!desc || (parent_irq && !parent_desc))
 		return -EINVAL;
 
+	mutex_lock(&desc->request_mutex);
+
+	if (desc->parent_irq != parent_irq)
+		irq_sysfs_update_parent(desc, parent_desc);
+
+	raw_spin_lock_irqsave(&desc->lock, flags);
 	desc->parent_irq = parent_irq;
+	raw_spin_unlock_irqrestore(&desc->lock, flags);
+
+	mutex_unlock(&desc->request_mutex);
 
-	irq_put_desc_unlock(desc, flags);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(irq_set_parent);
-- 
2.40.1

[RFC PATCH 4/5] irq: Move SMP affinity write handler out of proc.c

[Radu Rendec]

This patch prepares the ground for setting the SMP affinity from sysfs.
The bulk of the code is identical for procfs and sysfs, except for the
cpumask parsing functions, where procfs requires the _user variants.

Summary of changes:
- irq_select_affinity_usr() and write_irq_affinity() are moved from
  from proc.c to irqdesc.c
- write_irq_affinity() is slightly modified to allow using the other
  variant of cpumask parsing functions
- the definition of no_irq_affinity is moved from proc.c to manage.c
  and available only when CONFIG_SMP is enabled
- the declaration of no_irq_affinity is available only when CONFIG_SMP
  is enabled

Note that all existing use cases of no_irq_affinity were already
confined within CONFIG_SMP preprocessor conditionals.

Signed-off-by: Radu Rendec <rrendec@redhat.com>
---
 include/linux/irq.h    |  2 ++
 kernel/irq/internals.h |  2 ++
 kernel/irq/irqdesc.c   | 67 +++++++++++++++++++++++++++++++++++++++
 kernel/irq/manage.c    |  2 ++
 kernel/irq/proc.c      | 72 +++---------------------------------------
 5 files changed, 78 insertions(+), 67 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 7710f157e12de..0393fc02cfd46 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -639,7 +639,9 @@ static inline void irq_move_masked_irq(struct irq_data *data) { }
 static inline void irq_force_complete_move(struct irq_desc *desc) { }
 #endif
 
+#ifdef CONFIG_SMP
 extern int no_irq_affinity;
+#endif
 
 int irq_set_parent(int irq, int parent_irq);
 
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index c75cd836155c9..381a0b4c1d381 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -147,6 +147,8 @@ extern int irq_do_set_affinity(struct irq_data *data,
 
 #ifdef CONFIG_SMP
 extern int irq_setup_affinity(struct irq_desc *desc);
+extern ssize_t write_irq_affinity(unsigned int irq, const char __user *buffer,
+				  size_t count, bool is_list, bool is_user);
 #else
 static inline int irq_setup_affinity(struct irq_desc *desc) { return 0; }
 #endif
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index ec52b8b41002e..a46a76c29b8d1 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -133,6 +133,73 @@ EXPORT_SYMBOL_GPL(nr_irqs);
 static DEFINE_MUTEX(sparse_irq_lock);
 static DECLARE_BITMAP(allocated_irqs, IRQ_BITMAP_BITS);
 
+#ifdef CONFIG_SMP
+
+#ifndef CONFIG_AUTO_IRQ_AFFINITY
+static inline int irq_select_affinity_usr(unsigned int irq)
+{
+	/*
+	 * If the interrupt is started up already then this fails. The
+	 * interrupt is assigned to an online CPU already. There is no
+	 * point to move it around randomly. Tell user space that the
+	 * selected mask is bogus.
+	 *
+	 * If not then any change to the affinity is pointless because the
+	 * startup code invokes irq_setup_affinity() which will select
+	 * a online CPU anyway.
+	 */
+	return -EINVAL;
+}
+#else
+/* ALPHA magic affinity auto selector. Keep it for historical reasons. */
+static inline int irq_select_affinity_usr(unsigned int irq)
+{
+	return irq_select_affinity(irq);
+}
+#endif
+
+ssize_t write_irq_affinity(unsigned int irq, const char __user *buffer,
+			   size_t count, bool is_list, bool is_user)
+{
+	cpumask_var_t mask;
+	int err;
+
+	if (!irq_can_set_affinity_usr(irq) || no_irq_affinity)
+		return -EIO;
+
+	if (!zalloc_cpumask_var(&mask, GFP_KERNEL))
+		return -ENOMEM;
+
+	if (is_user)
+		err = is_list ? cpumask_parselist_user(buffer, count, mask) :
+				cpumask_parse_user(buffer, count, mask);
+	else
+		err = is_list ? cpulist_parse(buffer, mask) :
+				cpumask_parse(buffer, mask);
+	if (err)
+		goto free_cpumask;
+
+	/*
+	 * Do not allow disabling IRQs completely - it's a too easy
+	 * way to make the system unusable accidentally :-) At least
+	 * one online CPU still has to be targeted.
+	 */
+	if (!cpumask_intersects(mask, cpu_online_mask)) {
+		/*
+		 * Special case for empty set - allow the architecture code
+		 * to set default SMP affinity.
+		 */
+		err = irq_select_affinity_usr(irq) ? -EINVAL : count;
+	} else {
+		err = irq_set_affinity(irq, mask) ?: count;
+	}
+
+free_cpumask:
+	free_cpumask_var(mask);
+	return err;
+}
+#endif
+
 #ifdef CONFIG_SPARSE_IRQ
 
 static void irq_kobj_release(struct kobject *kobj);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index eec9b94747439..91cee7270d221 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -143,6 +143,8 @@ EXPORT_SYMBOL(synchronize_irq);
 #ifdef CONFIG_SMP
 cpumask_var_t irq_default_affinity;
 
+int no_irq_affinity;
+
 static bool __irq_can_set_affinity(struct irq_desc *desc)
 {
 	if (!desc || !irqd_can_balance(&desc->irq_data) ||
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 623b8136e9af3..76f0dda1f26b8 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -100,7 +100,6 @@ static int irq_affinity_hint_proc_show(struct seq_file *m, void *v)
 	return 0;
 }
 
-int no_irq_affinity;
 static int irq_affinity_proc_show(struct seq_file *m, void *v)
 {
 	return show_irq_affinity(AFFINITY, m);
@@ -111,81 +110,20 @@ static int irq_affinity_list_proc_show(struct seq_file *m, void *v)
 	return show_irq_affinity(AFFINITY_LIST, m);
 }
 
-#ifndef CONFIG_AUTO_IRQ_AFFINITY
-static inline int irq_select_affinity_usr(unsigned int irq)
-{
-	/*
-	 * If the interrupt is started up already then this fails. The
-	 * interrupt is assigned to an online CPU already. There is no
-	 * point to move it around randomly. Tell user space that the
-	 * selected mask is bogus.
-	 *
-	 * If not then any change to the affinity is pointless because the
-	 * startup code invokes irq_setup_affinity() which will select
-	 * a online CPU anyway.
-	 */
-	return -EINVAL;
-}
-#else
-/* ALPHA magic affinity auto selector. Keep it for historical reasons. */
-static inline int irq_select_affinity_usr(unsigned int irq)
-{
-	return irq_select_affinity(irq);
-}
-#endif
-
-static ssize_t write_irq_affinity(int type, struct file *file,
+static ssize_t irq_affinity_proc_write(struct file *file,
 		const char __user *buffer, size_t count, loff_t *pos)
 {
 	unsigned int irq = (int)(long)pde_data(file_inode(file));
-	cpumask_var_t new_value;
-	int err;
-
-	if (!irq_can_set_affinity_usr(irq) || no_irq_affinity)
-		return -EIO;
-
-	if (!zalloc_cpumask_var(&new_value, GFP_KERNEL))
-		return -ENOMEM;
-
-	if (type)
-		err = cpumask_parselist_user(buffer, count, new_value);
-	else
-		err = cpumask_parse_user(buffer, count, new_value);
-	if (err)
-		goto free_cpumask;
 
-	/*
-	 * Do not allow disabling IRQs completely - it's a too easy
-	 * way to make the system unusable accidentally :-) At least
-	 * one online CPU still has to be targeted.
-	 */
-	if (!cpumask_intersects(new_value, cpu_online_mask)) {
-		/*
-		 * Special case for empty set - allow the architecture code
-		 * to set default SMP affinity.
-		 */
-		err = irq_select_affinity_usr(irq) ? -EINVAL : count;
-	} else {
-		err = irq_set_affinity(irq, new_value);
-		if (!err)
-			err = count;
-	}
-
-free_cpumask:
-	free_cpumask_var(new_value);
-	return err;
-}
-
-static ssize_t irq_affinity_proc_write(struct file *file,
-		const char __user *buffer, size_t count, loff_t *pos)
-{
-	return write_irq_affinity(0, file, buffer, count, pos);
+	return write_irq_affinity(irq, buffer, count, false, true);
 }
 
 static ssize_t irq_affinity_list_proc_write(struct file *file,
 		const char __user *buffer, size_t count, loff_t *pos)
 {
-	return write_irq_affinity(1, file, buffer, count, pos);
+	unsigned int irq = (int)(long)pde_data(file_inode(file));
+
+	return write_irq_affinity(irq, buffer, count, true, true);
 }
 
 static int irq_affinity_proc_open(struct inode *inode, struct file *file)
-- 
2.40.1

[RFC PATCH 5/5] irq: Add smp_affinity/list attributes to sysfs

[Radu Rendec]

This patch adds the smp_affinity and smp_affinity_list attributes to the
sysfs interrupt interface. The implementation is identical to procfs,
and the attributes are visible only when CONFIG_SMP is enabled.

The intention is to allow SMP affinity to be controlled for chained
interrupt parents, which are typically not visible in procfs because
they are not requested through request_irq().

Signed-off-by: Radu Rendec <rrendec@redhat.com>
---
 kernel/irq/irqdesc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index a46a76c29b8d1..5b014df9fd730 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -210,6 +210,9 @@ static struct kobject *irq_kobj_base;
 #define IRQ_ATTR_RO(_name) \
 static struct kobj_attribute _name##_attr = __ATTR_RO(_name)
 
+#define IRQ_ATTR_RW(_name) \
+static struct kobj_attribute _name##_attr = __ATTR_RW(_name)
+
 static ssize_t per_cpu_count_show(struct kobject *kobj,
 				  struct kobj_attribute *attr, char *buf)
 {
@@ -332,6 +335,54 @@ static ssize_t actions_show(struct kobject *kobj,
 }
 IRQ_ATTR_RO(actions);
 
+#ifdef CONFIG_SMP
+static ssize_t smp_affinity_show(struct kobject *kobj,
+				 struct kobj_attribute *attr, char *buf)
+{
+	struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj);
+	const struct cpumask *mask = desc->irq_common_data.affinity;
+
+#ifdef CONFIG_GENERIC_PENDING_IRQ
+	if (irqd_is_setaffinity_pending(&desc->irq_data))
+		mask = desc->pending_mask;
+#endif
+
+	return scnprintf(buf, PAGE_SIZE, "%*pb\n", cpumask_pr_args(mask));
+}
+static ssize_t smp_affinity_store(struct kobject *kobj,
+				  struct kobj_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj);
+
+	return write_irq_affinity(desc->irq_data.irq, buf, count, false, false);
+}
+IRQ_ATTR_RW(smp_affinity);
+
+static ssize_t smp_affinity_list_show(struct kobject *kobj,
+				      struct kobj_attribute *attr, char *buf)
+{
+	struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj);
+	const struct cpumask *mask = desc->irq_common_data.affinity;
+
+#ifdef CONFIG_GENERIC_PENDING_IRQ
+	if (irqd_is_setaffinity_pending(&desc->irq_data))
+		mask = desc->pending_mask;
+#endif
+
+	return scnprintf(buf, PAGE_SIZE, "%*pbl\n", cpumask_pr_args(mask));
+}
+static ssize_t smp_affinity_list_store(struct kobject *kobj,
+				       struct kobj_attribute *attr,
+				       const char *buf, size_t count)
+{
+	struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj);
+
+	return write_irq_affinity(desc->irq_data.irq, buf, count, true, false);
+}
+IRQ_ATTR_RW(smp_affinity_list);
+#endif
+
 static struct attribute *irq_attrs[] = {
 	&per_cpu_count_attr.attr,
 	&chip_name_attr.attr,
@@ -340,6 +391,10 @@ static struct attribute *irq_attrs[] = {
 	&wakeup_attr.attr,
 	&name_attr.attr,
 	&actions_attr.attr,
+#ifdef CONFIG_SMP
+	&smp_affinity_attr.attr,
+	&smp_affinity_list_attr.attr,
+#endif
 	NULL
 };
 ATTRIBUTE_GROUPS(irq);
-- 
2.40.1

Re: [RFC PATCH 0/5] irq: sysfs interface improvements for SMP affinity control

[Thomas Gleixner]

On Tue, May 30 2023 at 17:45, Radu Rendec wrote:
> Note: I still need to update the Documentation/ directory for the new
>       sysfs interface, and I will address that in a future version.
>       At this point, I just want to get feedback about the current
>       approach.

From a conceptual POV I understand why this is required, which makes me
hate this chained mechanism even more.

Aside of having no visibility (counters, affinity, etc) the worst thing
about these chained hidden interrupts is:

  There is no control of runaway interrupts as they circumvent the core,
  which has caused hard to diagnose system hangups in the past. See
   
    ba714a9c1dea ("pinctrl/amd: Use regular interrupt instead of chained")

  for demonstration.

The argument I heard for this chained interrupt muck is that it's so
much more performant than using regular interrupt handlers via
request_irq(). It's obviously less overhead, but whether it matters and
most importantly whether it justifies the downsides is a different
question.

There is also the argument about double accounting. Right now the
chained handler is not accounted and only the childs are.

Though that is inconsistent with other demultiplex handlers which _must_
use regular interrupt handlers (threaded) to demultiplex interrupt chips
which sit behind SPI/I2C...

The sum of child interrupts is also not necessarily the number of parent
interrupts, unless there is always exactly only one child handler to
invoke.

Quite some of those demux handlers are also not RT compatible.

AFAICT, there is no real technical reason at least not for the vast
majority of usage sites why the demultiplex handler cannot run inside a
regular interrupt handler context.

So I personally would prefer to get rid of this chained oddball and just
have consistent mechanisms for dealing with interrupts, which would just
avoid exposing the affinity files in two different places.

Providing information about child/parent relationship is an orthogonal
issue.

If there is some good reason (aside of the chained muck) to have sysfs
based affinity management, then I'm not objecting as long as the
functionality is the same, i.e. effective affinity needs be exposed too.

Thanks,

        tglx

Re: [RFC PATCH 1/5] irq: Always enable parent interrupt tracking

[Thomas Gleixner]

Radu!

On Tue, May 30 2023 at 17:45, Radu Rendec wrote:

Please read and follow:

    https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

Especially the section about change logs.
   
Thanks,

        tglx

Re: [RFC PATCH 0/5] irq: sysfs interface improvements for SMP affinity control

[Radu Rendec]

On Wed, 2023-05-31 at 15:09 +0200, Thomas Gleixner wrote:
> From a conceptual POV I understand why this is required, which makes me
> hate this chained mechanism even more.
> 
> [cut]
> 
> AFAICT, there is no real technical reason at least not for the vast
> majority of usage sites why the demultiplex handler cannot run inside a
> regular interrupt handler context.

Thanks for taking the time to explain everything in such detail.
Much appreciated!

> So I personally would prefer to get rid of this chained oddball and just
> have consistent mechanisms for dealing with interrupts, which would just
> avoid exposing the affinity files in two different places.

Does this mean that if I came across a chained driver that lacked
affinity support, then changing it to use regular interrupts via
request_irq() would be a viable solution to enable affinity support
and you would consider accepting such a patch?

Taking a step back, in the case of hierarchical domains where *no*
multiplexing is involved, do you consider setting .irq_set_affinity()
to irq_chip_set_affinity_parent() a good way to enable affinity support
(assuming, of course, the driver lacks such support originally)?

At the end of the day, what I'm trying to do is find a way to enable
affinity support for a few specific drivers where it's lacking. My
initial impression, after reading Marc's message[1], was that the fix
lay at the system level, at least for multiplexing drivers. Hence my
naive attempt at a system-level fix. It is now becoming clear that the
fix will have to be evaluated/addressed at the driver level, on a case
by case basis.

As a side note, one thing I particularly like about your approach is
that it doesn't require any changes to irqbalance.

> Providing information about child/parent relationship is an orthogonal
> issue.

Agreed. Do you see any value in doing that? And, if yes, is reusing
(struct irq_desc).parent_irq and irq_set_parent() a good way of doing
it? FWIW, a multiplexing driver could do that regardless of how it
registers a handler for the parent interrupt (chained/regular).

> If there is some good reason (aside of the chained muck) to have sysfs
> based affinity management, then I'm not objecting as long as the
> functionality is the same, i.e. effective affinity needs be exposed too.

I can't think of any other reason. AFAICT, chained interrupts are the
only interrupts that are active but not visible in procfs. For any
other purpose, the procfs interface is good as it is.

Thanks,
Radu

[1] https://lore.kernel.org/all/87fslr7ygl.wl-maz@kernel.org/

Re: [RFC PATCH 1/5] irq: Always enable parent interrupt tracking

[Radu Rendec]

On Wed, 2023-05-31 at 15:10 +0200, Thomas Gleixner wrote:
> Please read and follow:
> 
>     https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
> 
> Especially the section about change logs.

Duly noted. Thanks for pointing me to the relevant documentation!

Best regards,
Radu

Re: [RFC PATCH 0/5] irq: sysfs interface improvements for SMP affinity control

[Radu Rendec]

On Wed, 2023-05-31 at 15:09 +0200, Thomas Gleixner wrote:
> On Tue, May 30 2023 at 17:45, Radu Rendec wrote:
> > Note: I still need to update the Documentation/ directory for the new
> >       sysfs interface, and I will address that in a future version.
> >       At this point, I just want to get feedback about the current
> >       approach.
> 
> From a conceptual POV I understand why this is required, which makes me
> hate this chained mechanism even more.
> 
> Aside of having no visibility (counters, affinity, etc) the worst thing
> about these chained hidden interrupts is:
> 
>   There is no control of runaway interrupts as they circumvent the core,
>   which has caused hard to diagnose system hangups in the past. See
>    
>     ba714a9c1dea ("pinctrl/amd: Use regular interrupt instead of chained")
> 
>   for demonstration.
> 
> The argument I heard for this chained interrupt muck is that it's so
> much more performant than using regular interrupt handlers via
> request_irq(). It's obviously less overhead, but whether it matters and
> most importantly whether it justifies the downsides is a different
> question.
> 
> There is also the argument about double accounting. Right now the
> chained handler is not accounted and only the childs are.
> 
> Though that is inconsistent with other demultiplex handlers which _must_
> use regular interrupt handlers (threaded) to demultiplex interrupt chips
> which sit behind SPI/I2C...
> 
> The sum of child interrupts is also not necessarily the number of parent
> interrupts, unless there is always exactly only one child handler to
> invoke.
> 
> Quite some of those demux handlers are also not RT compatible.
> 
> AFAICT, there is no real technical reason at least not for the vast
> majority of usage sites why the demultiplex handler cannot run inside a
> regular interrupt handler context.

I was about to send an RFC patch that converts a multiplexing IRQ chip
driver from chained to regular interrupts, when I realized I had come
full circle. Marc rejected a similar patch in the past [1] and the main
argument was that exposing the parent interrupt in procfs is backwards
incompatible. Quote:

 The problem of changing affinities for chained (or multiplexing)
 interrupts is, to make it short, that it breaks the existing userspace
 ABI that a change in affinity affects only the interrupt userspace
 acts upon, and no other. Which is why we don't expose any affinity
 setting for such an interrupt, as by definition changing its affinity
 affects all the interrupts that are muxed onto it.

So, there are two contradictory arguments here:
 * Chained interrupts are "muck", mainly because they circumvent the
   interrupt core and prevent the interrupt storm detector from kicking
   in when hardware misbehaves (and for other reasons as well).
 * Exposing the parent interrupt affinity control in procfs breaks the
   userspace ABI because all child interrupts inherently share the same
   affinity settings. This implies regular interrupts cannot be used.

Meanwhile, both interrupt storms and the lack of affinity control
support for multiplexing drivers are real problems, and my team and I
came across both. FWIW, the interrupt storm is the one we found more
recently, and it's the reason why I wanted to send that RFC patch I
mentioned before.

Is Marc's argument still relevant? Perhaps with both arguments in the
same email it becomes clearer that there needs to be some alignment at
the maintainer level. I would be more than happy to send patches and
help fix both issues. But I think that's not possible until you come up
with a strategy that you *both* agree to.

[1] https://lore.kernel.org/all/874k0bf7f7.wl-maz@kernel.org/

Thanks,
Radu

> So I personally would prefer to get rid of this chained oddball and just
> have consistent mechanisms for dealing with interrupts, which would just
> avoid exposing the affinity files in two different places.
> 
> Providing information about child/parent relationship is an orthogonal
> issue.
> 
> If there is some good reason (aside of the chained muck) to have sysfs
> based affinity management, then I'm not objecting as long as the
> functionality is the same, i.e. effective affinity needs be exposed too.
> 
> Thanks,
> 
>         tglx