Re: [PATCH 6/7] virt: steal_governor: restart sampling immediately after CPU hotplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] virt: steal_governor: deprecated system_wq use and workqueue
  mismatch
--

commit 71c885eda2b0a72a307fd87b15575e6e5a4178ed
Author: Yury Norov <ynorov@nvidia.com>

virt: steal_governor: restart sampling immediately after CPU hotplug

This commit registers a CPU hotplug callback to invalidate the current
steal-time sample and immediately requeue the governor work when a CPU
goes online or offline.

> diff --git a/drivers/virt/steal_governor.c b/drivers/virt/steal_governor.c
> index c34d91267ec1d..f71dbf27e4a9d 100644
> --- a/drivers/virt/steal_governor.c
> +++ b/drivers/virt/steal_governor.c
[ ... ]
> @@ -217,6 +220,13 @@ static bool preferred_cpus_valid(void)
>  	return true;
>  }
>  
> +static int hotplug_callback(unsigned int cpu)
> +{
> +	sg_ctx.sample_invalidated = true;
> +	mod_delayed_work(system_wq, &sg_ctx.work, 0);
> +	return 0;
> +}
> +
>  static void steal_governor_loop(struct work_struct *work)
>  {
>  	u64 curr_steal, delta_steal, delta_ns, steal_ratio;
> @@ -233,6 +243,11 @@ static void steal_governor_loop(struct work_struct *work)
>  	sg_ctx.steal = curr_steal;
>  	sg_ctx.time = now;
>  
> +	if (sg_ctx.sample_invalidated) {
> +		sg_ctx.sample_invalidated = false;
> +		goto requeue_work;
> +	}
> +
>  	/*
>  	 * steal_ratio = (delta_steal * 100*100)/(delta_ns * num_cpus())
>  	 * To avoid possible overflow, divide the denominator early.
> @@ -254,11 +269,14 @@ static void steal_governor_loop(struct work_struct *work)
>  		return;
>  	}
>  
> -	schedule_delayed_work(&sg_ctx.work, sg_ctx.delay);
> +requeue_work:
> +	mod_delayed_work(system_wq, &sg_ctx.work, sg_ctx.delay);
>  }

[Severity: Medium]
Should both mod_delayed_work() calls use system_percpu_wq instead of
system_wq?

system_wq is allocated with __WQ_DEPRECATED, so queueing sg_ctx.work onto
it triggers a pr_warn_once() deprecation warning in __queue_work().

In addition, steal_governor_init() queues sg_ctx.work onto
system_percpu_wq via schedule_delayed_work(), which is a separate
workqueue_struct from system_wq.  Because __queue_work() only enforces
cross-CPU non-reentrancy when worker->current_pwq->wq == wq, a hotplug
event during the first execution of steal_governor_loop() can queue
sg_ctx.work on a second CPU while it is still running on the first:

CPU0 (system_percpu_wq)           CPU1 (hotplug)
steal_governor_loop()
  guard(cpus_read_lock)()
    (blocks on cpus_write_lock)
                                  hotplug_callback()
                                    sg_ctx.sample_invalidated = true
                                    mod_delayed_work(system_wq, ..., 0)
                                      (wq != current_pwq->wq, queues on CPU1)
                                  cpus_write_unlock()
  sg_ctx.sample_invalidated = false
  ...
                                  steal_governor_loop()
                                    guard(cpus_read_lock)()
                                    (sample_invalidated is false, delta_ns ~ 0)
                                    increase_preferred_cpus()  <-- bogus sample

When the second worker runs right after the first finishes,
sample_invalidated has already been cleared and delta_ns is near zero,
yielding steal_ratio == 0 and calling increase_preferred_cpus()
prematurely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113184257.621707-2-ynorov@nvidia.com?part=6