Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Lockless access to `cpu_active_mask` in `preferred_cpus_valid()` can lead to a spurious failure of `cpumask_subset()`, causing the driver to permanently shut down.
--
commit 77aa795bc87999089d7a60bf5b5985b2c580cbec
Author: Shrikanth Hegde [off-list ref]
virt/steal_governor: Implement steal_governor policy loop
This patch implements the core policy loop for the steal_governor driver using
a delayed workqueue. It periodically calculates the steal time ratio and
dynamically adjusts the preferred CPU mask based on high or low contention
thresholds.
quoted hunk ↗ jump to hunk
diff --git a/drivers/virt/steal_governor.c b/drivers/virt/steal_governor.c
index 27f53ea16498b..6e31f9923dea4 100644
--- a/drivers/virt/steal_governor.c
+++ b/drivers/virt/steal_governor.c
[ ... ]
quoted hunk ↗ jump to hunk
+static bool preferred_cpus_valid(void)
+{
+ if (cpumask_empty(cpu_preferred_mask)) {
+ pr_err("empty preferred mask. stopping\n");
+ return false;
+ }
+
+ if (!cpumask_subset(cpu_preferred_mask, cpu_active_mask)) {
[Severity: High]
Can this lockless check spuriously fail and cause the driver to incorrectly
shut down?
Because cpus_read_lock() is not held here, there appears to be a race
condition with CPU hotplug operations (such as sched_cpu_deactivate()). When a
CPU is taken offline, it is removed from the relevant masks sequentially.
If cpumask_subset() reads cpu_preferred_mask while the dying CPU's bit is
still set, and then reads cpu_active_mask after the CPU's bit has been cleared,
the subset evaluation will spuriously return false.
quoted hunk ↗ jump to hunk
+ pr_err("preferred: %*pbl is not subset of active: %*pbl, stopping\n",
+ cpumask_pr_args(cpu_preferred_mask),
+ cpumask_pr_args(cpu_active_mask));
+ return false;
+ }
+
+ return true;
+}
+
+static void steal_governor_loop(struct work_struct *work)
+{
[ ... ]
quoted hunk ↗ jump to hunk
+ if (steal_ratio > sg_ctx.high_threshold)
+ decrease_preferred_cpus();
+ else if (steal_ratio <= sg_ctx.low_threshold)
+ increase_preferred_cpus();
+ /*
+ * else: steal ratio is within bounds. Still do design checks so that
+ * module restores to active if CPU hotplug breaks those assumptions.
+ */
+ if (!preferred_cpus_valid()) {
+ restore_preferred_to_active();
+ return;
[Severity: High]
If preferred_cpus_valid() spuriously fails due to the lockless race described
above, does this early return permanently disable the driver?
By returning here without requeueing the work via schedule_delayed_work(),
the periodic loop is halted completely, leaving the driver non-functional
until the module is manually reloaded.
quoted hunk ↗ jump to hunk
+ }
+
+requeue_work:
+ schedule_delayed_work(&sg_ctx.work, sg_ctx.delay);
+}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260909135617.871006-1-sshegde@linux.ibm.com?part=12