Re: [PATCH v7 09/12] virt/steal_monitor: Add control knobs for handling steal values

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

On 7/10/26 3:26 AM, Shrikanth Hegde wrote:
> These are the knobs to control the steal_monitor.
> 
> interval_ms:
> How often steal monitor checks for steal time.
> (Default: 1000 i.e 1 second)
> This controls how fast steal monitor driver reacts to changes to
> the contention of physical CPUs. Since it does a fair amount of
> work, setting too low will have overheads. Setting it too high
> might render the feature ineffective.
> Can be set between 10 to 100000. i.e. 10ms to 100seconds.
> 
> low_threshold:
> lower threshold value in percentage * 100.
> (Default: 200, i.e 2% steal is considered as low threshold)
> This determines what values should be considered as nil/no steal values.
> When steal monitor see steal time is below or equal to this value, it
> will increase the preferred CPUs by 1 core. Having value as zero
> might cause oscillations
> 
> high_threshold:
> higher threshold value in percentage * 100
> (Default: 500, i.e 5% steal is considered as high threshold)
> This determines what values should be considered as high steal values.
> When steal monitor sees steal time is higher than this value, it will
> reduce the preferred CPUs by 1 core.
> 
> module_param_cb methods are used to do the validation checks.
> This helps to ensure one configures sane values.
> Parameters values can't be changed at runtime. One has to unload
> the module and change it.
> 
> Also available at: Documentation/driver-api/steal-monitor.rst
> 
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
> v6->v7:
> - Add module_param_cb to do parameter checks.
> - Make parameters read only after module load.
> 
> 
>   drivers/virt/steal_monitor/sm_core.c | 92 +++++++++++++++++++++++++++-
>   1 file changed, 91 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virt/steal_monitor/sm_core.c b/drivers/virt/steal_monitor/sm_core.c
> index 180db424846c..4a03c14337be 100644
> --- a/drivers/virt/steal_monitor/sm_core.c
> +++ b/drivers/virt/steal_monitor/sm_core.c
> @@ -14,7 +14,97 @@
>   
>   #include "sm_core.h"
>   
> -struct steal_monitor sm_core_ctx;
> +struct steal_monitor sm_core_ctx = {
> +	.interval_ms = 1000,	/* 1 second */
> +	.high_threshold = 500,	/* 5% */
> +	.low_threshold = 200,	/* 2% */
> +};
> +
> +static int param_set_interval_ms(const char *val, const struct kernel_param *kp)
> +{
> +	unsigned int interval;
> +	int ret;
> +
> +	ret = kstrtouint(val, 0, &interval);
> +	if (ret)
> +		return ret;
> +
> +	if (interval < 10 || interval > 100000) {
> +		pr_err("steal_monitor: interval_ms must be between 10 and 100000\n");
> +		return -EINVAL;
> +	}
> +
> +	return param_set_uint(val, kp);
> +}
> +
> +static const struct kernel_param_ops interval_ms_ops = {
> +	.set = param_set_interval_ms,
> +	.get = param_get_uint,
> +};
> +
> +module_param_cb(interval_ms, &interval_ms_ops, &sm_core_ctx.interval_ms, 0444);
> +MODULE_PARM_DESC(interval_ms,
> +		 "Sampling frequency in milliseconds. default: 1000");
> +
> +static int param_set_high_threshold(const char *val, const struct kernel_param *kp)
> +{
> +	unsigned int threshold;
> +	int ret;
> +
> +	ret = kstrtouint(val, 0, &threshold);
> +	if (ret)
> +		return ret;
> +
> +	if (threshold <= sm_core_ctx.low_threshold) {
> +		pr_err("steal_monitor: high_threshold (%u) must be more than low_threshold (%u)\n",
> +		       threshold, sm_core_ctx.low_threshold);
> +		return -EINVAL;
> +	}
> +
> +	if (threshold >= 100 * 100) {
> +		pr_err("steal_monitor: high_threshold (%u) can't be more than 99.99%%\n",
> +		       threshold);
> +		return -EINVAL;
> +	}
> +
> +	return param_set_uint(val, kp);
> +}
> +
> +static const struct kernel_param_ops high_threshold_ops = {
> +	.set = param_set_high_threshold,
> +	.get = param_get_uint,
> +};
> +
> +module_param_cb(high_threshold, &high_threshold_ops, &sm_core_ctx.high_threshold, 0444);
> +MODULE_PARM_DESC(high_threshold,
> +		 "High steal threshold. default: 500 i.e 5%. Must be > low_threshold");
> +
> +static int param_set_low_threshold(const char *val, const struct kernel_param *kp)
> +{
> +	unsigned int threshold;
> +	int ret;
> +
> +	ret = kstrtouint(val, 0, &threshold);
> +	if (ret)
> +		return ret;
> +
> +	if (threshold >= sm_core_ctx.high_threshold) {
> +		pr_err("steal_monitor: low_threshold (%u) must be less than high_threshold (%u)\n",
> +		       threshold, sm_core_ctx.high_threshold);
> +		return -EINVAL;
> +	}
> +
> +	return param_set_uint(val, kp);
> +}
> +
> +static const struct kernel_param_ops low_threshold_ops = {
> +	.set = param_set_low_threshold,
> +	.get = param_get_uint,
> +};
> +
> +module_param_cb(low_threshold, &low_threshold_ops, &sm_core_ctx.low_threshold, 0444);
> +MODULE_PARM_DESC(low_threshold,
> +		 "Low steal threshold. default: 200 i.e 2%. Must be < high_threshold");
>  

As reported correctly by sashiko, below fails, but is a valid parameter set.
modprobe steal_monitor low_threshold=700 high_threshold=1500

Since low_threshold and high_thresholds are interdependent, I need to
defer this to steal_monitor_init. That's probably what yury mentioned
earlier, just that i didn't understand.

I think interval_ms, high_thresholds check can still be as module_param_cb
as they are independent.

I will fix it in v8.

Effectively diff:
---

diff --git a/drivers/virt/steal_monitor/sm_core.c b/drivers/virt/steal_monitor/sm_core.c
index 09a5c3a299c3..38007791a2dd 100644
--- a/drivers/virt/steal_monitor/sm_core.c
+++ b/drivers/virt/steal_monitor/sm_core.c
@@ -61,12 +61,6 @@ static int param_set_high_threshold(const char *val, const struct kernel_param *
         if (ret)
                 return ret;
  
-       if (threshold <= sm_core_ctx.low_threshold) {
-               pr_err("steal_monitor: high_threshold (%u) must be more than low_threshold (%u)\n",
-                      threshold, sm_core_ctx.low_threshold);
-               return -EINVAL;
-       }
-
         if (threshold >= 100 * 100) {
                 pr_err("steal_monitor: high_threshold (%u) can't be more than 99.99%%\n",
                        threshold);
@@ -85,30 +79,7 @@ module_param_cb(high_threshold, &high_threshold_ops, &sm_core_ctx.high_threshold
  MODULE_PARM_DESC(high_threshold,
                  "High steal threshold. default: 500 i.e 5%. Must be > low_threshold");
  
-static int param_set_low_threshold(const char *val, const struct kernel_param *kp)
-{
-       unsigned int threshold;
-       int ret;
-
-       ret = kstrtouint(val, 0, &threshold);
-       if (ret)
-               return ret;
-
-       if (threshold >= sm_core_ctx.high_threshold) {
-               pr_err("steal_monitor: low_threshold (%u) must be less than high_threshold (%u)\n",
-                      threshold, sm_core_ctx.high_threshold);
-               return -EINVAL;
-       }
-
-       return param_set_uint(val, kp);
-}
-
-static const struct kernel_param_ops low_threshold_ops = {
-       .set = param_set_low_threshold,
-       .get = param_get_uint,
-};
-
-module_param_cb(low_threshold, &low_threshold_ops, &sm_core_ctx.low_threshold, 0444);
+module_param_named(low_threshold, sm_core_ctx.low_threshold, uint, 0444);
  MODULE_PARM_DESC(low_threshold,
                  "Low steal threshold. default: 200 i.e 2%. Must be < high_threshold");
  
@@ -172,6 +143,11 @@ static void compute_preferred_cpus_work(struct work_struct *work)
  
  static int __init steal_monitor_init(void)
  {
+       if (sm_core_ctx.low_threshold >= sm_core_ctx.high_threshold) {
+               pr_err("steal_monitor: low_threshold (%u) must be less than high_threshold (%u)\n",
+                      sm_core_ctx.low_threshold, sm_core_ctx.high_threshold);
+               return -EINVAL;
+       }
         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);
  


  
>   static int __init steal_monitor_init(void)
>   {