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 <[email protected]> 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. > 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 [ ... ] > +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. > + 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) > +{ [ ... ] > + 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. > + } > + > +requeue_work: > + schedule_delayed_work(&sg_ctx.work, sg_ctx.delay); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
