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

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

On 7/9/26 1:29 AM, Yury Norov wrote:
> On Wed, Jul 08, 2026 at 10:25:09PM +0530, Shrikanth Hegde wrote:
>>
>>
>> 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;
>>          }
> 
> Don't overthink it. Simply don't allow user to change the parameters
> while the driver is running. It may only cause troubles to those
> compiling the driver into the Image. Those compiling it as a module
> will only have to reload the driver. This is how most of the kernel
> works.

Ok. I have made it that way.

I will just put a pr_err for delta_ns less than 1ms. If anyone hits it,
then we can bring in the logic to abort etc.

PS: I wasn;t able to hit it even with 1ms as interval and check for delta_ns as (1ms/8)

> 
> So, when the driver is loaded, make sanity checks for all user parameters.
> Interval, for example must be between 10ms and 10s. If user wants 20s,

I have it as 10ms and 100s in current v7.

> pr_err() something and fail to load. After the driver is loaded, don't
> allow to change the params exactly because it may bring certain type of
> errors in the logic, and you don't want to mitigate them.
> 
> If there will be a real need to adjust parameters on the fly - it will
> be another interesting topic (mostly interesting why user wants this).
> 

Makes sense. If the need arise, we will figure something out.
I have kept it simple for 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;
         }