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

[Yury Norov <ynorov@nvidia.com>]

On Mon, Jul 20, 2026 at 10:52:49PM +0530, Shrikanth Hegde wrote:
> schedule work at regular intervals to implement the steal_governor
> policy of monitoring the steal time and take action on the state of
> preferred CPUs. The interval is determined by interval_ms parameter.
> schedule_delayed_work is used since interval_ms
> is in the order of milliseconds. Work need not happen instantly.
> 
> Periodic policy loop essentially does:
> - Gets the total/delta steal values and cpus to use steal_ratio.
>   (get_system_steal_time, get_num_cpus_steal_ratio)
> - Calculate the steal_ratio as below.
> 
>        steal_ratio = (delta_steal * 100*100)/(delta_ns * num_cpus())
> 
>   It is calculated to consider the fractional values of steal time.
>   I.e 10 means 0.1% steal time. A few tricks such as divide by 10,000
>   are used to avoid possible overflow.
> - If steal value is higher than high threshold, call the method to reduce
>   the preferred CPUs. (decrease_preferred_cpus)
> - If steal value is lower or equal to low threshold, call the method to
>   increase the preferred CPUs. (increase_preferred_cpus)
> - If the steal value is in between, no action is taken.
> - Save the values for next delta calculations.
> - Ensure design checks are met.
>   1. At least one core/CPU must be there in preferred mask.
>   2. preferred CPUs is subset of active CPUs.
> If not met, then restore preferred CPUs to active and stop
> requeue of the work. Driver is effectively non-functional after that.
> 
> In Order to help the above loop, a few helper functions have been added.
> 
> 1. get_system_steal_time()
> - steal governor takes global view of steal time instead of individual
>   vCPU. Collect the steal values across the vCPUs of interest.
> - Sum up steal time values across possible CPUs. This helps to keep it
>   a monotonically increasing number and avoids spikes due to CPU
>   hotplug.
> 
> 2. decrease_preferred_cpus()
> - Called when there is high steal time. It needs to decide which CPUs to
>   mark as non-preferred and set that state.
> - Get first housekeeping CPU and its core mask. Mark it as
>   protected core. This helps to keep at least one core as preferred.
>   kernel ensures at least one housekeeping CPU stays active.
> - Find the last CPU outside of this protected core mask. (target CPU)
> - Based on that target CPU, get its sibling and mark them as
>   non-preferred.
> 
> 3. increase_preferred_cpus()
> - Called when there is low steal time. It needs to decide which CPUs to
>   mark as preferred and set that state.
> - Get the first active non-preferred CPUs. This likely is the last
>   set of CPUs being marked as non-preferred.
> - get the siblings of that CPU and mark them as preferred.
> 
> 4. get_num_cpus_steal_ratio()
> - informs how many CPUs needs to be considered for steal_ratio
>   calculations.
> - Return number of possible CPUs as get_system_steal_time computes
>   steal values across possible CPUs.
> 
> Notes:
> 1. Using core instead of individual CPUs performs better as SMT is
>    quite common and some hypervisor such as powerVM does core scheduling.
> 
> 2. This doesn't do any NUMA splicing to keep the code simpler and
>    minimal overhead. Current code expects CPUs spread uniformly
>    across NUMA nodes.
> 
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>  drivers/virt/steal_governor/core.c | 160 +++++++++++++++++++++++++++++
>  drivers/virt/steal_governor/core.h |   5 +
>  2 files changed, 165 insertions(+)
> 
> diff --git a/drivers/virt/steal_governor/core.c b/drivers/virt/steal_governor/core.c
> index 1cb766a8ce28..97f82d6df60f 100644
> --- a/drivers/virt/steal_governor/core.c
> +++ b/drivers/virt/steal_governor/core.c
> @@ -89,6 +89,158 @@ module_param_named(low_threshold, sg_core_ctx.low_threshold, uint, 0444);
>  MODULE_PARM_DESC(low_threshold,
>  		 "Low steal threshold. default: 200 i.e 2%. Must be < high_threshold");
>  
> +/*
> + * 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);

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. 

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

> +/*
> + * 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.

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

> + * 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/ ?

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

> +
> +	/* 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.

> + * 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;

> +	/* 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?

> +	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);

> +}
> +
>  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