Re: [PATCH v7 11/12] virt/steal_monitor: Act on steal time periodically and decide on preferred CPUs

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

On 7/11/26 2:07 AM, Yury Norov wrote:
> On Fri, Jul 10, 2026 at 03:26:47AM +0530, Shrikanth Hegde wrote:
>> schedule work at regular intervals. Interval is determined by
>> interval_ms parameter. schedule_delayed_work is used since interval_ms
>> is usually in order of milliseconds. Work need not happen instantly.
>>
>> Periodic work function essentially does:
>> - 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.
>> - If steal value is lower or equal to low threshold, call the method to
>>    increase the preferred CPUs.
>> - If the steal value is in between, no action is taken.
>> - Save the values for next delta calculations.
>> - Save the current direction of steal values to avoid oscillations.
>>    So two consecutive values of high values or low values are taken for
>>    decrease/increase of preferred CPUs.
>> - 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.
>>
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> ---
>> v6->v7:
>> - Merge two patches which did periodic work function.
>> - Misc checks for early firing, requeue work, math safety.
>>
>>   drivers/virt/steal_monitor/sm_core.c | 76 +++++++++++++++++++++++++++-
>>   drivers/virt/steal_monitor/sm_core.h |  1 +
>>   2 files changed, 76 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/virt/steal_monitor/sm_core.c b/drivers/virt/steal_monitor/sm_core.c
>> index 4a03c14337be..09a5c3a299c3 100644
>> --- a/drivers/virt/steal_monitor/sm_core.c
>> +++ b/drivers/virt/steal_monitor/sm_core.c
>> @@ -20,6 +20,12 @@ struct steal_monitor sm_core_ctx = {
>>   	.low_threshold = 200,	/* 2% */
>>   };
>>   
>> +enum sm_direction {
>> +	SM_DIR_INCREASE = -1,
>> +	SM_DIR_NONE	=  0,
>> +	SM_DIR_DECREASE	=  1,
>> +};
>> +
>>   static int param_set_interval_ms(const char *val, const struct kernel_param *kp)
>>   {
>>   	unsigned int interval;
>> @@ -106,14 +112,82 @@ module_param_cb(low_threshold, &low_threshold_ops, &sm_core_ctx.low_threshold, 0
>>   MODULE_PARM_DESC(low_threshold,
>>   		 "Low steal threshold. default: 200 i.e 2%. Must be < high_threshold");
>>   
>> +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, sm_core_ctx.prev_time));
>> +
>> +	if (unlikely(delta_ns < NSEC_PER_MSEC)) {
>> +		pr_err_ratelimited("steal_monitor: work scheduled too soon delta_ns: %llu\n",
>> +				   delta_ns);
>> +		goto requeue_work;
>> +	}
>> +
>> +	curr_steal = get_system_steal_time();
>> +	delta_steal = curr_steal > sm_core_ctx.prev_steal ?
>> +		      curr_steal - sm_core_ctx.prev_steal : 0;
>> +
>> +	/* Update for next calculation */
>> +	sm_core_ctx.prev_steal = curr_steal;
>> +	sm_core_ctx.prev_time = now;
>> +
>> +	/*
>> +	 * steal_ratio = (delta_steal * 100*100)/(delta_ns * num_cpus())
>> +	 * To avoid possible overflow, divide the denominator early.
>> +	 * Note minimum interval is 10ms.
>> +	 */
>> +	delta_ns = div_u64(delta_ns * get_num_cpus_steal_ratio(), 100 * 100);
>> +	steal_ratio = div64_u64(delta_steal, delta_ns);
>> +
>> +	if (sm_core_ctx.prev_direction == SM_DIR_DECREASE &&
>> +	    steal_ratio > sm_core_ctx.high_threshold)
>> +		decrease_preferred_cpus(&sm_core_ctx);
>> +	if (sm_core_ctx.prev_direction == SM_DIR_INCREASE &&
>> +	    steal_ratio <= sm_core_ctx.low_threshold)
>> +		increase_preferred_cpus(&sm_core_ctx);
> 
> I already said, I don't like this SM_DIR approach. If you want to
> avoid oscillations, just increase the gap. If it doesn't work, then we
> need to understand why.
> 

Ok. It was to avoid first simple, as it may have corner cases.
But yes, dropping it for now to keep things simple.

>> +
>> +	/*
>> +	 * mark the direction. Increasing the gap between hi and lo_threshold
>> +	 * helps to avoid ping-pongs.
>> +	 */
>> +	if (steal_ratio > sm_core_ctx.high_threshold)
>> +		sm_core_ctx.prev_direction = SM_DIR_DECREASE;
>> +	else if (steal_ratio <= sm_core_ctx.low_threshold)
>> +		sm_core_ctx.prev_direction = SM_DIR_INCREASE;
>> +	else
>> +		sm_core_ctx.prev_direction = SM_DIR_NONE;
>> +
>> +requeue_work:
>> +	/* maintain design constructs always */
>> +	WARN_ON_ONCE(cpumask_empty(cpu_preferred_mask));
>> +	WARN_ON_ONCE(!cpumask_subset(cpu_preferred_mask, cpu_active_mask));
> 
> cpu_read_lock here? And again, you should do something to restore
> integrity. WARN_ON is not enough. The simplest and safest thing you
> can do is to unload the driver. You definitely shouldn't schedule a
> new work against the broken cpu_preferred_mask.

How about not requeue the work if it broken. Add a pr_err and return.
That makes driver pretty much nop until rmmod.

         /* maintain design constructs always */
         if (cpumask_empty(cpu_preferred_mask)) {
                 pr_err("empty cpu_preferred_mask, stop steal_monitor work");
                 return;
         }

         if (!cpumask_subset(cpu_preferred_mask, cpu_active_mask)) {
                 pr_err("preferred: %*pbl is not a subset of active: %*pbl, stop steal_monitor work\n",
                        pr_cpuamsk_args(cpu_preferred_mask), pr_cpuamsk_args(cpu_active_mask));
		return;
	}

(Ignore whitespace mangling)

> 
>> +
>> +	/* Trigger for next sampling */
>> +	schedule_delayed_work(&sm_core_ctx.work,
>> +			      msecs_to_jiffies(sm_core_ctx.interval_ms));
>> +}
>> +
>>   static int __init steal_monitor_init(void)
>>   {
>> -	pr_info("steal_monitor is enabled\n");
>> +	pr_info("steal_monitor is enabled. interval: %ums, high_threshold: %u, low_threshold: %u\n",
>> +		sm_core_ctx.interval_ms, sm_core_ctx.high_threshold, sm_core_ctx.low_threshold);
>> +
>> +	INIT_DELAYED_WORK(&sm_core_ctx.work, compute_preferred_cpus_work);
>> +	sm_core_ctx.prev_steal = get_system_steal_time();
>> +	sm_core_ctx.prev_time = ktime_get();
>> +
>> +	schedule_delayed_work(&sm_core_ctx.work,
>> +			      msecs_to_jiffies(sm_core_ctx.interval_ms));
>> +
>>   	return 0;
>>   }
>>   
>>   static void __exit steal_monitor_exit(void)
>>   {
>> +	cancel_delayed_work_sync(&sm_core_ctx.work);
> 
> cancel_delayed_work_sync() is not enough for a self-requeueing work.
> compute_preferred_cpus_work() always requeues itself. Module unload
> can return with delayed work armed against module text/data. Use
> disable_delayed_work_sync() or a stop flag checked before requeueing.

ok. I will make it disable_delayed_work_sync.
Thanks for catching that.