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

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

Hi Yury.

On 7/22/26 12:58 AM, Yury Norov wrote:
> On Mon, Jul 20, 2026 at 10:52:49PM +0530, Shrikanth Hegde wrote:

[...]
    
>> +/*
>> + * Returns steal time of the full system.
>> + * Compute collective steal time across all possible CPUs.
>> + */
>> +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;
>> +}
> 
> There's another implementation of the same logic in
> hd_calculate_steal_percentage())
> 
> It means it should live in include/linux/kernel_stat.h as:
>          
>          u64 kcpustat_steal_time(struct cpumask *cpus);
> 
> Or possibly even more generic:
> 
>          u64 kcpustat_field_total(enum cpu_usage_stat usage, struct cpumask *cpus);

That's good idea. Below are the callsites i think can be
refactored in addition to this patch.
  
arch/s390/kernel/hiperdispatch.c: hd_calculate_steal_percentage
fs/proc/uptime.c: uptime_proc_show

I can pick up this refactoring post the series. I think it is better
not to club them together. is that ok?

> 
> Similarly to the existing kcpustat_field().
> 
> The other possible candidates are: fs/proc/stat.c:: show_stat()
> arch_cpu_idle_time(), but I think it's out of the scope of your
> series.

Yes.

drivers/leds/trigger/ledtrig-activity.c: led_activity_function
fs/proc/stat.c: show_stat

These two sum up multiple CPUTIME_ fields. Doing them in one loop is
likely better than getting each of them in a separate loop. No?


> 
> What's the relation between the arch/s390/kernel/hiperdispatch.c and
> your steal governor? Is that a similar concept?
> 

It is similar but independent idea. Ilya explained it in brief below.
https://youtu.be/adxUKFPlOp0?t=1846

Tobais, Ilya, Correct me if i am wrong.

- It samples steal time and decides on CPU_CAPACITY of a
   CPU (between 1024 and 126) periodically.
- Triggers sched domain rebuild.


But Using CPU_CAPACITY to ensure no task running has drawbacks as we had
explored it very early.
- It fails under high concurrency. Typical real life workloads spawns
   threads/tasks based on online CPUs. Scheduler chooses low capacity
   CPUs instead of running them on busy CPUs. So under high concurrency or
   high utilization it simply doesn't work.
- It takes a long time to get the running task out of it.
- It needs a sched domain rebuild. That is costly
   (One can play tricks to avoid it)
- There are PR/SM overheads.


Since S390 has explicit knowledge of which CPUs to take out, it will likely
want a arch specific mechanism in steal_governor to take out Vertical Low CPUs
first. That is being deferred for now, once s390 has that, my take is
hiperdispatch can be removed altogether.

>> +/*
>> + * Returns number of CPUs to consider for steal ratio.
>> + * Return possible CPUs.
>> + */
> 
> Can you rephrase the comment? It has 2 'return' sections with
> different meaning. If the 2nd one is the implementation detail,
> I'd put it inside the function scope, or drop entirely.
> 

Ok. Let me drop second statement there.

>> +static unsigned int get_num_cpus_steal_ratio(void)
>> +{
>> +	return num_possible_cpus();
>> +}
>> +
>> +/*
>> + * Take action to decrease preferred CPUs.
>> + *
> 
> Drop this 'Take action' wording please.

Ok.

> 
>> + * Decrease the preferred CPUs by 1 core.
>> + * Take out the last core in the active & preferred.
>> + *
>> + * Must ensure
>> + * - least one housekeeping core is always kept as preferred
> 
> s/least/at least/ ?

Yep.

> 
>> + * - preferred is always subset of active.
>> + */
>> +static void decrease_preferred_cpus(void)
>> +{
>> +	int tmp_cpu, first_hk_cpu, last_cpu;
>> +	const struct cpumask *first_hk_core;
>> +	int target_cpu = nr_cpu_ids;
>> +
>> +	guard(cpus_read_lock)();
>> +	first_hk_cpu = cpumask_first_and(housekeeping_cpumask(HK_TYPE_KERNEL_NOISE),
>> +					 cpu_preferred_mask);
>> +	if (first_hk_cpu >= nr_cpu_ids)
>> +		return;
>> +
>> +	last_cpu = cpumask_last(cpu_preferred_mask);
>> +
>> +	if (last_cpu >= nr_cpu_ids)
>> +		return;
>> +
>> +	/* Always leave first housekeeping core as preferred. */
>> +	first_hk_core = topology_sibling_cpumask(first_hk_cpu);
>> +
>> +	/* Find the last CPU which doesn't belong to that first hk_core. */
>> +	if (!cpumask_test_cpu(last_cpu, first_hk_core)) {
>> +		target_cpu = last_cpu;
>> +	} else {
>> +		for_each_cpu_andnot(tmp_cpu, cpu_preferred_mask, first_hk_core)
>> +			target_cpu = tmp_cpu;
>> +	}
> 
> Too much local variables. You can drop those tmp_cpu, last_cpu and
> target_cpu, and just use a single variable 'cpu'. That would also
> simplify your logic:
> 
>          cpu = cpumask_last(cpu_preferred_mask);
>          core = topology_sibling_cpumask(first_hk_cpu);
> 
>          if (cpumask_test_cpu(cpu, core)) {
>                  for_each_cpu_andnot(cpu, cpu_preferred_mask, core)
>                          /* nop */ ;
>          }
> 
>          if (cpu >= nr_cpu_ids)
>                  return;
> 
> And so on.

Ok. Just two would suffice i think. cpu, and target_cpu.
With a bit shuffling first_hk_cpu can also go away.


I need at least two so that this loop works.

         for_each_cpu_and(tmp_cpu, topology_sibling_cpumask(target_cpu),
                          cpu_preferred_mask)
                 set_cpu_preferred(tmp_cpu, false);

> 
>> +
>> +	/* Only the first housekeeping core remains */
>> +	if (target_cpu >= nr_cpu_ids)
>> +		return;
>> +
>> +	for_each_cpu_and(tmp_cpu, topology_sibling_cpumask(target_cpu),
>> +			 cpu_preferred_mask)
>> +		set_cpu_preferred(tmp_cpu, false);
>> +}
>> +
>> +/*
>> + * Take action to increase preferred CPUs.
>> + *
> 
> Again, drop this 'take action' thing.

Sure.

> 
>> + * Increase the preferred CPUs by 1 core.
>> + * Add the first core in active & !preferred
>> + *
>> + * Must ensure preferred is subset of active.
>> + */
>> +static void increase_preferred_cpus(void)
>> +{
>> +	int first_cpu, tmp_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(tmp_cpu, topology_sibling_cpumask(first_cpu),
>> +			 cpu_active_mask)
>> +		set_cpu_preferred(tmp_cpu, true);
>> +}
>> +
>> +static void compute_preferred_cpus_work(struct work_struct *work)
>> +{
>> +	u64 curr_steal, delta_steal, delta_ns, steal_ratio;
>> +	ktime_t now;
>> +
>> +	now = ktime_get();
>> +	delta_ns = ktime_to_ns(ktime_sub(now, sg_core_ctx.time));
>> +
>> +	if (unlikely(delta_ns < NSEC_PER_MSEC)) {
>> +		pr_err_ratelimited("steal_governor: work scheduled too soon delta_ns: %llu\n",
>> +				   delta_ns);
>> +		goto requeue_work;
>> +	}
>> +
>> +	curr_steal = get_system_steal_time();
>> +	delta_steal = curr_steal > sg_core_ctx.steal ?
>> +		      curr_steal - sg_core_ctx.steal : 0;
>> +
>> +	/* Update for next calculation */
>> +	sg_core_ctx.steal = curr_steal;
>> +	sg_core_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_num_cpus_steal_ratio(),
>> +				      100 * 100), 1);
>> +	steal_ratio = div64_u64(delta_steal, delta_ns);
>> +
>> +	if (steal_ratio > sg_core_ctx.high_threshold)
>> +		decrease_preferred_cpus();
>> +	if (steal_ratio <= sg_core_ctx.low_threshold)
>> +		increase_preferred_cpus();
> 
> If you neither increase, nor decrease, you don't need to check the mask
> because you know you don't modify it. Also, I'd wrap the below integrity
> checks into a helper function.
> 
> 	if (steal_ratio > sg_core_ctx.high_threshold)
> 		decrease_preferred_cpus();
> 	else if (steal_ratio <= sg_core_ctx.low_threshold)
> 		increase_preferred_cpus();
>          else
>                  goto requeue_work;
> 
>          if (check_integrity())
>                  return;
> 

Ack. Good catch. Will do.

>> +	/* maintain design constructs always */
>> +	if (cpumask_empty(cpu_preferred_mask)) {
>> +		pr_err("empty preferred mask. stop steal governor\n");
>> +		restore_preferred_to_active();
>> +		return;
>> +	}
>> +
>> +	if (!cpumask_subset(cpu_preferred_mask, cpu_active_mask)) {
>> +		pr_err("preferred: %*pbl is not subset of active: %*pbl, stop steal governor\n",
>> +		       cpumask_pr_args(cpu_preferred_mask),
>> +		       cpumask_pr_args(cpu_active_mask));
>> +		restore_preferred_to_active();
>> +		return;
>> +	}
>> +
>> +requeue_work:
>> +	/* Trigger for next sampling */
> 
> The lablel above is pretty explaining to me. The comment just
> duplicates it. Maybe drop the comment?

Ok.

> 
>> +	schedule_delayed_work(&sg_core_ctx.work,
>> +			      msecs_to_jiffies(sg_core_ctx.interval_ms));
> 
> If you need jiffies, why don't you have them in the structure, instead of
> milliseconds?
> 
> 	schedule_delayed_work(&sg_core_ctx.work, sg_core_ctx.delay);
> 

I would prefer milliseconds as jiffies is very difficult for users to understand.
It depends on HZ value and one has to query from configs.

HZ can very from 100 to 1000 today. Again I will have to play tricks to schedule
the governor at fixed intervals.

So i think it is better to use milliseconds here.
Correct me if i am not making sense.

>> +}
>> +
>>   static int __init steal_governor_init(void)
>>   {
>>   	if (sg_core_ctx.low_threshold >= sg_core_ctx.high_threshold) {
>> @@ -100,11 +252,19 @@ static int __init steal_governor_init(void)
>>   	pr_info("steal_governor is enabled. interval: %ums, high_threshold: %u, low_threshold: %u\n",
>>   		sg_core_ctx.interval_ms, sg_core_ctx.high_threshold, sg_core_ctx.low_threshold);
>>   
>> +	INIT_DELAYED_WORK(&sg_core_ctx.work, compute_preferred_cpus_work);
>> +	sg_core_ctx.steal = get_system_steal_time();
>> +	sg_core_ctx.time = ktime_get();
>> +
>> +	schedule_delayed_work(&sg_core_ctx.work,
>> +			      msecs_to_jiffies(sg_core_ctx.interval_ms));
>> +
>>   	return 0;
>>   }
>>   
>>   static void __exit steal_governor_exit(void)
>>   {
>> +	disable_delayed_work_sync(&sg_core_ctx.work);
>>   	restore_preferred_to_active();
>>   	pr_info("steal_governor is disabled\n");
>>   }
>> diff --git a/drivers/virt/steal_governor/core.h b/drivers/virt/steal_governor/core.h
>> index e27305284ac0..59329c1d7109 100644
>> --- a/drivers/virt/steal_governor/core.h
>> +++ b/drivers/virt/steal_governor/core.h
>> @@ -12,6 +12,11 @@
>>   #include <linux/workqueue.h>
>>   #include <linux/ktime.h>
>>   #include <linux/kconfig.h>
>> +#include <linux/kernel_stat.h>
>> +#include <linux/topology.h>
>> +#include <linux/sched/isolation.h>
>> +#include <linux/cleanup.h>
>> +#include <linux/math64.h>
>>   
>>   struct steal_governor {
>>   	struct delayed_work	work;
>> -- 
>> 2.47.3