Re: [PATCH v9 10/11] virt/steal_governor: Implement steal_governor policy loop

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

Hi Yury,

On 7/25/26 2:35 AM, Yury Norov wrote:
> On Fri, Jul 24, 2026 at 07:37:31PM +0530, Shrikanth Hegde wrote:

[...]
    
>> +/* Return collective steal time across system. */
>> +static u64 get_system_steal_time(void)
>> +{
>> +	int cpu;
>> +	u64 total_steal = 0;
>> +
>> +	for_each_possible_cpu(cpu)
>> +		total_steal += kcpustat_cpu(cpu).cpustat[CPUTIME_STEAL];
>> +
>> +	return total_steal;
>> +}
> 
> In v8 I pointed to the identical function in s390 code, and you agreed
> to unify them, but that didn't happen. Please do that in the next
> version.

I thought I will do this refactoring after the series gets merged upstream.
If you insist, I will do this in next version.

> 
>> +/* Return number of CPUs to consider steal ratio. */
>> +static unsigned int get_system_cpus(void)
>> +{
>> +	return num_possible_cpus();
>> +}
>> +
>> +/*
>> + *
> 
> Useless line

My bad. Will remove.

> 
>> + * Called when the steal governor detects high physical CPU contention.
>> + * It finds the last active core in the preferred mask and mark those
>> + * CPUs as non-preferred.
>> + *
>> + * Must ensure:
>> + * - at least one core is always kept as preferred
>> + * - preferred is always subset of active.
>> + */
>> +static void decrease_preferred_cpus(void)
>> +{
>> +	const struct cpumask *first_hk_core;
>> +	int target_cpu = nr_cpu_ids;
>> +	int cpu;
>> +
>> +	guard(cpus_read_lock)();
>> +	cpu = cpumask_first_and(housekeeping_cpumask(HK_TYPE_KERNEL_NOISE),
>> +				cpu_preferred_mask);
>> +	if (cpu >= nr_cpu_ids)
>> +		return;
>> +
>> +	/* Always leave first housekeeping core as preferred. */
>> +	first_hk_core = topology_sibling_cpumask(cpu);
>> +	cpu = cpumask_last(cpu_preferred_mask);
>> +	if (cpu >= nr_cpu_ids)
>> +		return;
>> +
>> +	/* Find the last CPU which doesn't belong to that first hk_core. */
>> +	if (!cpumask_test_cpu(cpu, first_hk_core)) {
>> +		target_cpu = cpu;
>> +	} else {
>> +		for_each_cpu_andnot(cpu, cpu_preferred_mask, first_hk_core)
>> +			target_cpu = cpu;
>> +	}
>> +
>> +	/* Only the first housekeeping core remains */
>> +	if (target_cpu >= nr_cpu_ids)
>> +		return;
>> +
>> +	for_each_cpu_and(cpu, topology_sibling_cpumask(target_cpu),
>> +			 cpu_preferred_mask)
>> +		set_cpu_preferred(cpu, false);
>> +}
>> +
>> +/*
>> + * Called when the steal governor detects no/low physical CPU contention.
>> + * It finds the first active core outside of preferred mask and mark
>> + * those CPUs as preferred.
>> + *
>> + * Must ensure preferred is subset of active.
>> + */
>> +static void increase_preferred_cpus(void)
>> +{
>> +	int first_cpu, cpu;
>> +
>> +	guard(cpus_read_lock)();
>> +	first_cpu = cpumask_first_andnot(cpu_active_mask, cpu_preferred_mask);
>> +
>> +	/* All CPUs are preferred. Nothing to increase further */
>> +	if (first_cpu >= nr_cpu_ids)
>> +		return;
>> +
>> +	for_each_cpu_and(cpu, topology_sibling_cpumask(first_cpu),
>> +			 cpu_active_mask)
>> +		set_cpu_preferred(cpu, true);
>> +}
>> +
>> +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)) {
>> +		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 compute_preferred_cpus_work(struct work_struct *work)
> 
> Bad name. You're not only computing here, but actually adjusting the
> preferred CPUs mask.
> 

adjust_preferred_cpus_work or steal_governor_loop ?

>> +{
>> +	u64 curr_steal, delta_steal, delta_ns, steal_ratio;
>> +	ktime_t now;
>> +
>> +	now = ktime_get();
>> +	delta_ns = ktime_to_ns(ktime_sub(now, sg_ctx.time));
>> +
>> +	if (unlikely(delta_ns < NSEC_PER_MSEC)) {
>> +		pr_err_ratelimited("work scheduled too soon delta_ns: %llu\n", delta_ns);
>> +		goto requeue_work;
>> +	}
>> +
>> +	curr_steal = get_system_steal_time();
>> +	delta_steal = curr_steal > sg_ctx.steal ? curr_steal - sg_ctx.steal : 0;
>> +	sg_ctx.steal = curr_steal;
>> +	sg_ctx.time = now;
>> +
>> +	/*
>> +	 * steal_ratio = (delta_steal * 100*100)/(delta_ns * num_cpus())
>> +	 * To avoid possible overflow, divide the denominator early.
>> +	 * Note minimum interval is 100ms.
>> +	 */
>> +	delta_ns = max_t(u64, div_u64(delta_ns * get_system_cpus(), 10000), 1);
>> +	steal_ratio = div64_u64(delta_steal, delta_ns);
> 
> So if:
> 
> Possible CPUs = 128
> Active CPUs = 8
> Steal on online CPUs = 50%
> Steal on offline CPUS = 0%
> 
> Then calculated ratio would be:
> 
>          (50% × 8 + 0% * 120) / 128 = 3.125%
> 
> Instead of decreasing the number of preferred CPUs, you'll do nothing
> under default thresholds, or even increase.
> 
> Have you tested your driver against such a configuration?

I thought about it, but given range of systems linux supports today, there
will always be configurations where defaults are not good enough.

That is why it is recommended to build it as module. load the driver
will custom high and low threshold.


I had it as active CPUs only earlier. but the concern is, what happens at hotplug.
Since online a new CPUs steal time can add big value, it can trigger high threshold
check, similarly for offline. Plus raciness w.r.t to active CPUs.
(same issue for online CPUs)

Hence I chose it as possible CPUs.

> 
> Also, I'm not quite sure how you'd handle a case when you have half
> of CPUS in your core offlined, but you manage preferred mask per-core,
> so you offline or online less CPUs than expected. Can you mention that
> scenario in the documentation?
> 

If it is with thresholds, it is same case as above. Other than that, i have
tested it doesn;t set for offline CPUs etc.

I am not sure if scaling the thresholds with active CPUs is a good idea or not.
It can easily cause more math headache for users.

>> +
>> +	if (steal_ratio > sg_ctx.high_threshold)
>> +		decrease_preferred_cpus();
>> +	else if (steal_ratio <= sg_ctx.low_threshold)
>> +		increase_preferred_cpus();
>> +	else
>> +		goto requeue_work;
>> +
>> +	if (!preferred_cpus_valid()) {
>> +		restore_preferred_to_active();
>> +		return;
>> +	}
>> +
>> +requeue_work:
>> +	schedule_delayed_work(&sg_ctx.work, sg_ctx.delay);
>> +}
>> +
>>   static int __init steal_governor_init(void)
>>   {
>>   	if (sg_ctx.low_threshold >= sg_ctx.high_threshold) {
>> @@ -117,6 +267,10 @@ static int __init steal_governor_init(void)
>>   	}
>>   
>>   	sg_ctx.delay = msecs_to_jiffies(sg_ctx.interval_ms);
>> +	INIT_DELAYED_WORK(&sg_ctx.work, compute_preferred_cpus_work);
>> +	sg_ctx.steal = get_system_steal_time();
>> +	sg_ctx.time = ktime_get();
>> +	schedule_delayed_work(&sg_ctx.work, sg_ctx.delay);
>>   	pr_info("enabled. interval: %ums, high_threshold: %u, low_threshold: %u\n",
>>   		sg_ctx.interval_ms, sg_ctx.high_threshold, sg_ctx.low_threshold);
>>   
>> @@ -125,6 +279,7 @@ static int __init steal_governor_init(void)
>>   
>>   static void __exit steal_governor_exit(void)
>>   {
>> +	disable_delayed_work_sync(&sg_ctx.work);
>>   	restore_preferred_to_active();
>>   	pr_info("disabled\n");
>>   }
>> -- 
>> 2.47.3