Re: [PATCH v6 20/23] virt/steal_monitor: Act on steal values at regular intervals

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

On 7/8/26 9:43 PM, Yury Norov wrote:
> On Wed, Jul 08, 2026 at 04:03:18PM +0530, Shrikanth Hegde wrote:
>>
>>
>> On 7/7/26 12:46 PM, Shrikanth Hegde wrote:
>>>
>>>
>>> On 7/7/26 2:03 AM, Yury Norov wrote:
>>>> On Wed, Jul 01, 2026 at 07:46:51PM +0530, Shrikanth Hegde wrote:
>>>>> This is the steal_monitor core functionality done in periodic work
>>>>>
>>>>> - Calculate the steal_ratio. It is multiplied by 100 to consider the
>>>>>     fractional values of steal time. I.e 10 means 0.1% steal time.
>>>>> - 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.
>>>>>
>>>>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>>>>> ---
>>>>> v5->v6:
>>>>> - Address u64 overflow concerns.
>>>>>
>>>>>    drivers/virt/steal_monitor/sm_core.c | 33 ++++++++++++++++++++++++++++
>>>>>    1 file changed, 33 insertions(+)
>>>>>
>>>>> diff --git a/drivers/virt/steal_monitor/sm_core.c
>>>>> b/drivers/virt/ steal_monitor/sm_core.c
>>>>> index b499faa61010..7b7435f79b85 100644
>>>>> --- a/drivers/virt/steal_monitor/sm_core.c
>>>>> +++ b/drivers/virt/steal_monitor/sm_core.c
>>>>> @@ -34,6 +34,37 @@ MODULE_PARM_DESC(low_threshold,
>>>>>    static void compute_preferred_cpus_work(struct work_struct *work)
>>>>>    {
>>>>> +    u64 curr_steal, delta_steal, delta_ns, steal_ratio;
>>>>> +    ktime_t now;
>>>>> +
>>>>> +    curr_steal = get_system_steal_time();
>>>>> +    now = ktime_get();
>>>>> +
>>>>> +    /* get the deltas */
>>>>> +    delta_steal = curr_steal > sm_core_ctx.prev_steal ?
>>>>> +              curr_steal - sm_core_ctx.prev_steal : 0;
>>>>> +    delta_ns = max_t(u64, ktime_to_ns(ktime_sub(now,
>>>>> sm_core_ctx.prev_time)), 1);
>>>>
>>>> The below return on '!delta_ns' makes this max(...) useless, right?
>>>> Regardless, if the time between 2 measures is less then 1ns, I
>>>> believe, the whole measure is not trustworthy
>>>>
>>>
>>> Seeing sashiko comment on divide by zero, i forgot I had max above there :(
>>>
>>>>> +
>>>>> +    /* Update for next calculation */
>>>>> +    sm_core_ctx.prev_steal = curr_steal;
>>>>> +    sm_core_ctx.prev_time = now;
>>>>
>>>> So below return should go prior to this update, because 'now' is actually
>>>> the same as 'prev_time', right?
>>>>
>>>> I don't understand why 'now' can be so close to prev_time, because
>>>> you've scheduled this callback on the regular interval. But if that's
>>>> possible, can you explain that and do like this at the very beginning
>>>> of the function:
>>>>
>>
>> I don't think it is possible.
>>
>>>>           now = ktime_get();
>>>>           if (unlikely(now < sm_core_ctx.prev_time +
>>>> sm_core_ctx.interval / 2)) {
>>>>                   pr_warn(...);
>>>>                   return;
>>>>           }
>>>
>>> Pretty much, just have to requeue the work.
>>>
>>>>
>>>> And if it's a never-happen condition, just use WARN_ON().
>>>>
>>
>> I have decided keep just max_t() to ensure no divide by zero happens.
>>
>> Issue with putting sm_core_ctx.interval_ms check is that, one may edit
>> it while the work is queued, but not yet called. It would print wrongly
>> in that case.
> 
> Who is that 'one'? If it's userspace, you can simply disallow changing
> the interval. If it's kernel, you're already compromised, and
> everything is not trustworthy.
> 
> I would simply disallow to change any parameters while the driver is
> loaded. If I need to change something - I unload it and load with new
> interval, or whatever.

It is user writing to /sys/module/steal_monitor/parameters/interval_ms
that i thought of. So user.

If we disable then those checks can be valid.

> 
>> Even if work function gets called prematurely, it should be fine since the
>> steal also increases accordingly. No issues in delta logic.
>>
>> So capping to 1 to ensure no divide by 0 happens due to scaling of
>> delta_ns is good enough.
>   
> If there's 1 nanosecond between two calls, your statistics is very
> inaccurate, pretty much a noise. You want to make decision based on
> noise. That's simply wrong.


yes, that makes sense too. I have kept interval_ms/8 as safe bet.
Return will not queue the timer and effectively disables it.

         ktime_t now;

         now = ktime_get();
         delta_ns = ktime_to_ns(ktime_sub(now, sm_core_ctx.prev_time));

         if (unlikely(delta_ns < sm_core_ctx.interval_ms * ((u64)NSEC_PER_MSEC >> 3))) {
                 pr_err("work scheduled too soon delta_time: %llu ns. Abort steal_monitor.\n",
                        delta_ns);

                 guard(cpus_read_lock)();
                 cpumask_copy(&__cpu_preferred_mask, cpu_active_mask);
                 return;
         }


>   
>>> Ya, it is a never-happen condition. I will use WARN_ON.
>>>