Re: [PATCH v7 10/12] virt/steal_monitor: Provide functions for managing steal values

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

Hi Yury,

On 7/11/26 1:30 AM, Yury Norov wrote:
> On Fri, Jul 10, 2026 at 03:26:46AM +0530, Shrikanth Hegde wrote:
>> Provide functions which is going to be used in the periodic work
>> function to calculate and handle steal time values.
>>
>> get_system_steal_time()
>> - steal monitor takes global view of steal time instead of individual
>>    vCPU. For this collect overall steal values across all the vCPUs or
>>    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.
>>
>> 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 must stay 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.
>>
>> 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.
>>
>> get_num_cpus_steal_ratio()
>> - This method informs the steal_monitor core, how many CPUs it needs to
>>    consider 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>
>> ---
>> v6->v7:
>> - Combined patches which added helper functions.
>> - Use possible CPUs for steal value calculations.
>>
>>   drivers/virt/steal_monitor/Makefile   |   2 +-
>>   drivers/virt/steal_monitor/defaults.c | 100 ++++++++++++++++++++++++++
>>   drivers/virt/steal_monitor/sm_core.h  |   8 +++
> 
> What for do you split functionality into sm_core and default? There's
> no non-default implementation, right?
> 
> I'd just put everything in drivers/virt/steal_monitor.c. It would be
> ~300 LOCs file - quite bearable.

Ok. I will move it to sm_core.c

> 
>>   3 files changed, 109 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/virt/steal_monitor/defaults.c
>>
>> diff --git a/drivers/virt/steal_monitor/Makefile b/drivers/virt/steal_monitor/Makefile
>> index bd7d120a79b5..273a6dd59fea 100644
>> --- a/drivers/virt/steal_monitor/Makefile
>> +++ b/drivers/virt/steal_monitor/Makefile
>> @@ -3,4 +3,4 @@
>>   # Steal time monitor to alter preferred CPU state.
>>   obj-$(CONFIG_STEAL_MONITOR) += steal_monitor.o
>>   
>> -steal_monitor-y := sm_core.o
>> +steal_monitor-y := sm_core.o defaults.o
>> diff --git a/drivers/virt/steal_monitor/defaults.c b/drivers/virt/steal_monitor/defaults.c
>> new file mode 100644
>> index 000000000000..d4b016317554
>> --- /dev/null
>> +++ b/drivers/virt/steal_monitor/defaults.c
>> @@ -0,0 +1,100 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Base file contains the default implementations.
>> + *
>> + * Copyright (C) 2026 IBM
>> + * Author: Shrikanth Hegde <sshegde@linux.ibm.com>
>> + */
>> +#include "sm_core.h"
>> +
>> +/*
>> + * Returns steal time of the full system.
>> + * Compute collective steal time across all possible CPUs.
>> + */
>> +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;
>> +}
>> +
>> +/*
>> + * Returns number of CPUs to consider for steal ratio.
>> + * Return possible CPUs.
>> + */
>> +unsigned int get_num_cpus_steal_ratio(void)
>> +{
>> +	return num_possible_cpus();
>> +}
>> +
>> +/*
>> + * Take action to decrease preferred CPUs.
>> + *
>> + * 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
>> + * - preferred is always subset of active.
>> + */
>> +void decrease_preferred_cpus(struct steal_monitor *ctx)
>> +{
>> +	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);
> 
> Nit: you can return here if nothing found, and save on the 2nd
> traverse.

Ok.

I thought about initially, but since this almost never
happens i thought i will combine both.

> 
>> +	last_cpu = cpumask_last(cpu_preferred_mask);
>> +
>> +	if (first_hk_cpu >= nr_cpu_ids || 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;
>> +	}
>> +
>> +	/* 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);
> 
> I think it should return status: if the function can't disable CPUs
> now, it would be a good hint for the caller that it would be useless
> to call it again.
> 
> You may keep status in struct steal_monitor like:
> 
>          if (steal_ratio > sm_core_ctx.high_threshold)  {
>                  if (sm_core_ctx->status | CANT_DECREASE) {
>                          pr_something();
>                  else
>                          sm_core_ctx->status = decrease();
> 
> It would be a good hint to user that he has the driver misconfigured,
> and save the driver extra work. Same for increase().
> 

I thought about the extra work in function, but doesn't happen too often IMO.

Also, it is specially not a misconfiguration for increase.
So i have kept it stateless for the below reason.

- Under typical operation of this driver, user will enable it once.
- Once enabled, user will use their VM as usual.
- Majority of the time the steal time will be less.
- workload are bursty in nature.
- Occasionally many VM will have high utilization and there will be steal time.
   This lasts for sometime.
- After workload completes, steal time goes low again.
- Cycle could repeat after extended low steal time duration.

So when the steal time is low, though driver is enabled, doesn't mean it
is mis-configured. Just that there is contention and driver has nothing to
do. So, adding print there could easily consume the console.

Similarly, there could situations, where decrease cannot happen though there is
high steal time, Though they are corner cases.

For example,
- one small/few VMs have not enabled the driver. steal time could be high, but this
   VM has already down to one core. It can't decrease any further.
- Though all VMs have enabled the feature, but task running is not FAIR class. Though
   steal time shows high.

Hitting only one core or all cores isn't necessarily a misconfiguration.
It is a possible behavior during severe contention or complete idle system.

we need to continuously monitor steal time so that it can expand/contract the
based on current situation. If we stop calling the functions, natural expand/contract
will not happen. There is no interrupt which arrives due to high/low steal time where
we can kick start the driver again. Also it is a difficult ask for user to keep enabling
or disabling the driver.

Since this can be called at minimal once in 10ms, I guess we can incur the additional
overheads to keep the logic simple and stateless. What do you think?

PS: I will remove that additional SM_DIR as you suggested in other reply. That keeps
it all stateless.

>> +}
>> +
>> +/*
>> + * Take action to increase preferred CPUs.
>> + *
>> + * Increase the preferred CPUs by 1 core.
>> + * Add the first core in active & !preferred
>> + *
>> + * Must ensure preferred is subset of active.
>> + */
>> +void increase_preferred_cpus(struct steal_monitor *ctx)
>> +{
>> +	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);
>> +}
>> diff --git a/drivers/virt/steal_monitor/sm_core.h b/drivers/virt/steal_monitor/sm_core.h
>> index 8bbb606add99..ee68cd8b1944 100644
>> --- a/drivers/virt/steal_monitor/sm_core.h
>> +++ b/drivers/virt/steal_monitor/sm_core.h
>> @@ -11,6 +11,9 @@
>>   #include <linux/cpumask.h>
>>   #include <linux/workqueue.h>
>>   #include <linux/ktime.h>
>> +#include <linux/kernel_stat.h>
>> +#include <linux/topology.h>
>> +#include <linux/sched/isolation.h>
>>   
>>   struct steal_monitor {
>>   	struct delayed_work	work;
>> @@ -24,4 +27,9 @@ struct steal_monitor {
>>   
>>   extern struct steal_monitor sm_core_ctx;
>>   
>> +u64 get_system_steal_time(void);
>> +unsigned int get_num_cpus_steal_ratio(void);
>> +void increase_preferred_cpus(struct steal_monitor *ctx);
>> +void decrease_preferred_cpus(struct steal_monitor *ctx);
>> +
>>   #endif /* __VIRT_STEAL_CORE_H */
>> -- 
>> 2.47.3