Re: [PATCH v13 13/13] virt/steal_governor: Enable the driver

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

On 9/9/26 7:47 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] preferred_cpus_valid() reads cpu_preferred_mask and cpu_active_mask without holding cpus_read_lock(), creating a data race with CPU hotplug that causes the governor to permanently shut down.
> --
> 
> commit b0d791ea69ac4f874448d3baf935e4825cba622f
> Author: Shrikanth Hegde <sshegde@linux.ibm.com>
> 
> virt/steal_governor: Enable the driver
> 
> This commit provides a configuration option to enable the steal_governor driver
> for paravirtualized environments. It enforces PARAVIRT and SMP dependencies and
> selects CONFIG_PREFERRED_CPU for core scheduler mechanisms.
> 
>> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
>> index 52eb7e4ba71f6..eeb84e578ddf9 100644
>> --- a/drivers/virt/Kconfig
>> +++ b/drivers/virt/Kconfig
>> @@ -41,6 +41,23 @@ config FSL_HV_MANAGER
>>             4) A kernel interface for receiving callbacks when a managed
>>   	     partition shuts down.
>>   
>> +config STEAL_GOVERNOR
>> +	tristate "Dynamic vCPU management based on steal time"
>> +	depends on PARAVIRT && SMP
>> +	select PREFERRED_CPU
> 
> [Severity: High]
> While this patch enables the driver, can the background loop in
> steal_governor_loop() permanently terminate during a concurrent CPU hotplug
> operation?
> 
> If a CPU is offlined, sched_cpu_deactivate() clears the target CPU from
> cpu_preferred_mask and then cpu_active_mask without memory barriers. If this
> happens concurrently, preferred_cpus_valid() in drivers/virt/steal_governor.c
> might observe a torn state because it evaluates the masks without holding
> cpus_read_lock():
> 

clear_bit is atomic operation. It cannot happen concurrently IMHO.

> drivers/virt/steal_governor.c:preferred_cpus_valid()
> 	if (!cpumask_subset(cpu_preferred_mask, cpu_active_mask)) {
> 		pr_err("preferred: %*pbl is not subset of active: ...\n",
> 		       ...);
> 		return false;
> 	}
> 
> If the check fails due to this race, the governor loop permanently shuts down
> since it returns instead of requeuing the work:

Even if it happens, it is safe to shut down, rather than continue in weird state.

> 
> drivers/virt/steal_governor.c:steal_governor_loop()
> 	if (!preferred_cpus_valid()) {
> 		restore_preferred_to_active();
> 		return;
> 	}
> 
> Would holding cpus_read_lock() during these mask checks prevent this race and
> keep the loop running correctly?
> 
> [ ... ]
>