mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v4] lib/group_cpus: Snapshot cluster masks to keep grouping hotplug invariant
@ 2026-08-26 17:15 Vishal Badole
  2026-08-28  0:01 ` Andrew Morton
  2026-09-01  1:48 ` Guo, Wangyang
  0 siblings, 2 replies; 4+ messages in thread
From: Vishal Badole @ 2026-08-26 17:15 UTC (permalink / raw)
  To: tglx, akpm, linux-kernel
  Cc: bp, wangyang.guo, tianyou.li, tim.c.chen, dan.liang,
	Vishal Badole, stable

group_cpus_evenly() builds the managed-IRQ affinity spread used by
multi-queue devices such as NVMe. That spread is meant to be a property
of the static CPU topology: it walks cpu_present_mask and then
cpu_possible_mask so every hardware queue owns a fixed set of CPUs,
including CPUs that are offline at the time. A driver depends on that
partition staying stable across re-computation - the CPUs a queue is
given at probe must still describe the same queue after the device is
later reset and its affinity recomputed.

On an AMD system that stability breaks across an s2idle cycle. With CPUs
3-11 offlined and only CPUs 0-2 left online, the machine is suspended to
s2idle and resumed. The NVMe controller uses the simple-suspend quirk, so
resume fully re-initialises it and recomputes the affinity spread. The
system then hangs for roughly two minutes and stays sluggish afterwards,
the controller only making progress through its command-timeout poll:

  nvme nvme0: I/O tag 898 (3382) QID 9 timeout, completion polled
  nvme nvme0: I/O tag 398 (618e) QID 11 timeout, completion polled

QID 9 and QID 11 are the queues whose CPUs were offline when the spread
was recomputed. "completion polled" means the commands did finish in
hardware, but their interrupts were never delivered to a CPU that was
watching the queue, so nothing reaped them until the timeout fired.

It happens because commit 89802ca36c96 ("lib/group_cpus: make group CPU
cluster aware") derives the cluster groups from topology_cluster_cpumask(),
which lists only the cluster siblings that are online when it is called.
The resulting partition therefore depends on the transient online mask
rather than on the topology alone. Recomputed on resume while the non-boot
CPUs are still offline, it no longer matches the boot-time partition, and a
queue is left with an affinity that does not cover the CPU it is meant to
serve once that CPU comes back online. The dependence is on the online
mask, not on any AMD-specific behaviour, so the same stall is reproducible
on Intel platforms as well.

Make the cluster grouping depend on the complete cluster topology rather
than on whichever CPUs happen to be online. Snapshot the cluster masks
once while every present CPU is online and reuse that view for every
later spread. Every spread then groups from the same masks, so the
partition computed when the controller is reset matches the one computed
at probe and each queue's IRQ still covers the CPUs it serves. If the
snapshot was never taken, the cluster path is skipped and the plain
present/possible spread is used.

Fixes: 89802ca36c96 ("lib/group_cpus: make group CPU cluster aware")
Cc: stable@vger.kernel.org
Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
---
Changes in v4:
- Drop the index-based unwind; reuse the mask-based cleanup path.
- Link to v3: https://lore.kernel.org/all/20260821184923.2462575-1-Vishal.Badole@amd.com/

Changes in v3:
- Take the snapshot under cpus_read_trylock() to avoid a torn view.
- Link to v2: https://lore.kernel.org/all/20260814123402.3170161-1-Vishal.Badole@amd.com/

Changes in v2:
- Gate the snapshot on cpu_present_mask instead of cpu_possible_mask.
- Reword the changelog and the code comments. No other change.
- Link to v1: https://lore.kernel.org/all/20260807073612.3711269-1-Vishal.Badole@amd.com/
---
 lib/group_cpus.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 85 insertions(+), 2 deletions(-)

diff --git a/lib/group_cpus.c b/lib/group_cpus.c
index e6e18d7a49bb..3c2229feb9c3 100644
--- a/lib/group_cpus.c
+++ b/lib/group_cpus.c
@@ -6,6 +6,7 @@
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/cpu.h>
+#include <linux/mutex.h>
 #include <linux/sort.h>
 #include <linux/group_cpus.h>
 
@@ -286,6 +287,77 @@ static void assign_cpus_to_groups(unsigned int ncpus,
 	}
 }
 
+/*
+ * topology_cluster_cpumask() only lists the cluster siblings that are online,
+ * so group_cpus_evenly() would compute a different managed-IRQ partition when
+ * recomputed with CPUs offline (e.g. an NVMe reset across s2idle), steering a
+ * queue's IRQ away from the CPU it serves.
+ *
+ * Snapshot the cluster masks once, on the first spread seen with every
+ * present CPU online, and reuse it so the grouping stays stable. If no
+ * snapshot exists (partial boot via maxcpus=/nosmp, or allocation failure)
+ * the cluster path is skipped and the plain present/possible spread is
+ * used. Only the cluster path is stabilised; grp_spread_init_one()'s
+ * sibling mask is unchanged. The snapshot lives for the system lifetime
+ * and is not refreshed for CPUs hot-added after boot.
+ */
+static cpumask_var_t *cluster_snapshot;
+static bool cluster_snapshot_ready;
+static DEFINE_MUTEX(cluster_snapshot_lock);
+
+static void capture_cluster_snapshot(void)
+{
+	cpumask_var_t *snapshot;
+	unsigned int cpu;
+
+	/* Pairs with the smp_store_release() below. */
+	if (smp_load_acquire(&cluster_snapshot_ready))
+		return;
+
+	/* Only capture when all present CPUs are online. */
+	if (!data_race(cpumask_equal(cpu_present_mask, cpu_online_mask)))
+		return;
+
+	mutex_lock(&cluster_snapshot_lock);
+	if (cluster_snapshot_ready)
+		goto out;
+
+	snapshot = kcalloc(nr_cpu_ids, sizeof(*snapshot), GFP_KERNEL);
+	if (!snapshot)
+		goto out;
+
+	for_each_possible_cpu(cpu)
+		if (!zalloc_cpumask_var(&snapshot[cpu], GFP_KERNEL))
+			goto free_snapshot;
+
+	/* Trylock: a caller may hold a lock the hotplug writer needs. */
+	if (!cpus_read_trylock())
+		goto free_snapshot;
+
+	/* Recheck under the lock, which also pins the cluster masks. */
+	if (!data_race(cpumask_equal(cpu_present_mask, cpu_online_mask))) {
+		cpus_read_unlock();
+		goto free_snapshot;
+	}
+
+	for_each_possible_cpu(cpu)
+		cpumask_copy(snapshot[cpu], topology_cluster_cpumask(cpu));
+	cpus_read_unlock();
+
+	cluster_snapshot = snapshot;
+	/* Publish the filled snapshot before the ready flag. */
+	smp_store_release(&cluster_snapshot_ready, true);
+	goto out;
+
+free_snapshot:
+	/* Unallocated entries are NULL, which free_cpumask_var() ignores. */
+	for_each_possible_cpu(cpu)
+		free_cpumask_var(snapshot[cpu]);
+	kfree(snapshot);
+out:
+	mutex_unlock(&cluster_snapshot_lock);
+}
+
 static int alloc_cluster_groups(unsigned int ncpus,
 				unsigned int ngroups,
 				struct cpumask *node_cpumask,
@@ -299,6 +371,17 @@ static int alloc_cluster_groups(unsigned int ncpus,
 	const struct cpumask **clusters;
 	struct node_groups *cluster_groups;
 
+	/*
+	 * Capture on the first spread with every present CPU online (normally
+	 * the first device probe); later spreads reuse it. Sample the ready
+	 * flag once so both loops below use one consistent source.
+	 */
+	capture_cluster_snapshot();
+
+	/* Pairs with the smp_store_release() in capture_cluster_snapshot(). */
+	if (!smp_load_acquire(&cluster_snapshot_ready))
+		goto no_cluster;
+
 	cpumask_copy(msk, node_cpumask);
 
 	/* Probe how many clusters in this node. */
@@ -307,7 +390,7 @@ static int alloc_cluster_groups(unsigned int ncpus,
 		if (cpu >= nr_cpu_ids)
 			break;
 
-		cluster_mask = topology_cluster_cpumask(cpu);
+		cluster_mask = cluster_snapshot[cpu];
 		if (!cpumask_weight(cluster_mask))
 			goto no_cluster;
 		/* Clean out CPUs on the same cluster. */
@@ -331,7 +414,7 @@ static int alloc_cluster_groups(unsigned int ncpus,
 	cpumask_copy(msk, node_cpumask);
 	for (n = 0; n < ncluster; n++) {
 		cpu = cpumask_first(msk);
-		cluster_mask = topology_cluster_cpumask(cpu);
+		cluster_mask = cluster_snapshot[cpu];
 		nc = cpumask_weight_and(cluster_mask, node_cpumask);
 		clusters[n] = cluster_mask;
 		cluster_groups[n].id = n;
-- 
2.34.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] lib/group_cpus: Snapshot cluster masks to keep grouping hotplug invariant
  2026-08-26 17:15 [PATCH v4] lib/group_cpus: Snapshot cluster masks to keep grouping hotplug invariant Vishal Badole
@ 2026-08-28  0:01 ` Andrew Morton
  2026-09-01  1:48 ` Guo, Wangyang
  1 sibling, 0 replies; 4+ messages in thread
From: Andrew Morton @ 2026-08-28  0:01 UTC (permalink / raw)
  To: Vishal Badole
  Cc: tglx, linux-kernel, bp, wangyang.guo, tianyou.li, tim.c.chen,
	dan.liang, stable

On Wed, 26 Aug 2026 22:45:37 +0530 Vishal Badole <Vishal.Badole@amd.com> wrote:

> group_cpus_evenly() builds the managed-IRQ affinity spread used by
> multi-queue devices such as NVMe. That spread is meant to be a property
> of the static CPU topology: it walks cpu_present_mask and then
> cpu_possible_mask so every hardware queue owns a fixed set of CPUs,
> including CPUs that are offline at the time. A driver depends on that
> partition staying stable across re-computation - the CPUs a queue is
> given at probe must still describe the same queue after the device is
> later reset and its affinity recomputed.
> 
> On an AMD system that stability breaks across an s2idle cycle. With CPUs
> 3-11 offlined and only CPUs 0-2 left online, the machine is suspended to
> s2idle and resumed. The NVMe controller uses the simple-suspend quirk, so
> resume fully re-initialises it and recomputes the affinity spread. The
> system then hangs for roughly two minutes and stays sluggish afterwards,
> the controller only making progress through its command-timeout poll:
> 
>   nvme nvme0: I/O tag 898 (3382) QID 9 timeout, completion polled
>   nvme nvme0: I/O tag 398 (618e) QID 11 timeout, completion polled
> 
> ...
> 
> Make the cluster grouping depend on the complete cluster topology rather
> than on whichever CPUs happen to be online. Snapshot the cluster masks
> once while every present CPU is online and reuse that view for every
> later spread. Every spread then groups from the same masks, so the
> partition computed when the controller is reset matches the one computed
> at probe and each queue's IRQ still covers the CPUs it serves. If the
> snapshot was never taken, the cluster path is skipped and the plain
> present/possible spread is used.

Thanks, I'll queue this for testing in linux-next while awaiting
further review and testing input.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH v4] lib/group_cpus: Snapshot cluster masks to keep grouping hotplug invariant
  2026-08-26 17:15 [PATCH v4] lib/group_cpus: Snapshot cluster masks to keep grouping hotplug invariant Vishal Badole
  2026-08-28  0:01 ` Andrew Morton
@ 2026-09-01  1:48 ` Guo, Wangyang
  2026-09-07  6:12   ` Badole, Vishal
  1 sibling, 1 reply; 4+ messages in thread
From: Guo, Wangyang @ 2026-09-01  1:48 UTC (permalink / raw)
  To: Vishal Badole, tglx, akpm, linux-kernel
  Cc: bp, Li, Tianyou, tim.c.chen, Liang, Dan, stable

> Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
> ---
> Changes in v4:
> - Drop the index-based unwind; reuse the mask-based cleanup path.

Reproduced under QEMU via offline-cluster + reset_controller: I/O from the
online CPUs stalls (QID-timeout/completion-polled). This patch fixes it.

Reviewed-by: Wangyang Guo <wangyang.guo@intel.com>
Tested-by: Wangyang Guo <wangyang.guo@intel.com>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v4] lib/group_cpus: Snapshot cluster masks to keep grouping hotplug invariant
  2026-09-01  1:48 ` Guo, Wangyang
@ 2026-09-07  6:12   ` Badole, Vishal
  0 siblings, 0 replies; 4+ messages in thread
From: Badole, Vishal @ 2026-09-07  6:12 UTC (permalink / raw)
  To: Guo, Wangyang, tglx, akpm, linux-kernel
  Cc: bp, Li, Tianyou, tim.c.chen, Liang, Dan, stable

Hi Andrew,

On 9/1/2026 7:18 AM, Guo, Wangyang wrote:
>> Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
>> ---
>> Changes in v4:
>> - Drop the index-based unwind; reuse the mask-based cleanup path.
> 
> Reproduced under QEMU via offline-cluster + reset_controller: I/O from the
> online CPUs stalls (QID-timeout/completion-polled). This patch fixes it.
> 
> Reviewed-by: Wangyang Guo <wangyang.guo@intel.com>
> Tested-by: Wangyang Guo <wangyang.guo@intel.com>

This patch has been reviewed and tested by Intel, with corresponding 
Reviewed-by and Tested-by acknowledgments received. Could you please 
consider merging it into the appropriate branch or upcoming release?

Thanks,
Vishal

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-09-07  6:12 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-26 17:15 [PATCH v4] lib/group_cpus: Snapshot cluster masks to keep grouping hotplug invariant Vishal Badole
2026-08-28  0:01 ` Andrew Morton
2026-09-01  1:48 ` Guo, Wangyang
2026-09-07  6:12   ` Badole, Vishal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®