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

[Yury Norov <ynorov@nvidia.com>]

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.

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,
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).

Thanks,
Yury