Re: [PATCH v6 16/23] virt/steal_monitor: Compute work at regular intervals

Hi Yury,

On 7/3/26 11:44 PM, Yury Norov wrote:

On Wed, Jul 01, 2026 at 07:46:47PM +0530, Shrikanth Hegde wrote:

quoted

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 <redacted>
---
  drivers/virt/steal_monitor/sm_core.c | 26 +++++++++++++++++++++++++-
  drivers/virt/steal_monitor/sm_core.h |  3 +++
  2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/virt/steal_monitor/sm_core.c b/drivers/virt/steal_monitor/sm_core.c
index 1ba638224abb..b499faa61010 100644
--- a/drivers/virt/steal_monitor/sm_core.c
+++ b/drivers/virt/steal_monitor/sm_core.c

@@ -32,9 +32,32 @@ module_param_named(low_threshold, sm_core_ctx.low_threshold, uint, 0644);
  MODULE_PARM_DESC(low_threshold,
  		 "Low steal threshold (default: 200 i.e 2%)");
  
+static void compute_preferred_cpus_work(struct work_struct *work)
+{
+	/* At least one core is kept as preferred */
+	WARN_ON(cpumask_empty(cpu_preferred_mask));

This is very true, at least one CPU must be preferred. But throwing
warnings is useless. What do you want me to do if I see this warning?
The only possible solution I see is unloading the driver and not using
this balancer at all. Don't think it's what you want me to do.

The design construct of preferred ⊆ active should be valid during entire
steal monitor lifetime. Not just init of module.

My thought process here is, if one sees warning, they could report this issue
and we can fix any such violations.

Your logic should make it impossible to have the preferred cpumask
empty.

quoted

+
+	/* Warn if interval_ms is set to 0, that might cause lockup. */
+	if (unlikely(sm_core_ctx.interval_ms == 0)) {
+		WARN_ON(1);
+		sm_core_ctx.interval_ms = 1000; /* Fallback to default */
+	}

WARN_ON() means panic under some configurations. You shouldn't do
that. The proper way of handling it is failing in steal_monitor_init().
This function is int, not a void, for a reason.

This i have removed in v7. I have moved to module_named_cb for validating the parameters.

quoted

+	/* 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);
+
+	schedule_delayed_work(&sm_core_ctx.work,
+			      msecs_to_jiffies(sm_core_ctx.interval_ms));
+
  	return 0;
  }
  

@@ -42,6 +65,7 @@ static void __exit steal_monitor_exit(void)
  {
  	pr_info("steal_monitor is disabled\n");
  
+	cancel_delayed_work_sync(&sm_core_ctx.work);
  	guard(cpus_read_lock)();
  	cpumask_copy(&__cpu_preferred_mask, cpu_active_mask);
  }

diff --git a/drivers/virt/steal_monitor/sm_core.h b/drivers/virt/steal_monitor/sm_core.h
index e5c3ea0a63c9..ea06e83c228c 100644
--- a/drivers/virt/steal_monitor/sm_core.h
+++ b/drivers/virt/steal_monitor/sm_core.h

@@ -8,6 +8,9 @@
  #include <linux/kernel.h>
  #include <linux/init.h>
  #include <linux/cpuhplock.h>
+#include <linux/cpumask.h>
+#include <linux/workqueue.h>
+#include <linux/sched/isolation.h>
  
  struct steal_monitor {
  	struct delayed_work	work;

-- 
2.47.3