Re: [PATCH v13 12/13] virt/steal_governor: Implement steal_governor policy loop

[sashiko-bot@kernel.org]

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 <sshegde@linux.ibm.com>

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/20260909135617.871006-1-sshegde@linux.ibm.com?part=12