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

[Shrikanth Hegde <sshegde@linux.ibm.com>]

On 9/9/26 7:46 PM, sashiko-bot@kernel.org wrote:
> 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.
> 

CPU Hotplug race is taken care. order of udpates are expected not to cause the
above issue.

> 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.

That's intentional.

> 
>> +	}
>> +
>> +requeue_work:
>> +	schedule_delayed_work(&sg_ctx.work, sg_ctx.delay);
>> +}
>