Re: [PATCH v6 12/23] virt: Introduce steal monitor driver

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

Hi Yury.

On 7/3/26 11:50 PM, Yury Norov wrote:
> On Wed, Jul 01, 2026 at 07:46:43PM +0530, Shrikanth Hegde wrote:
>> Introduce a new driver in virt named steal_monitor. This driver
>> will compute the steal time and drive the policy decisions of preferred
>> CPU state.
>>
>> More on it can be found in the Documentation/driver-api/steal-monitor.rst
>> Introduce the skeleton code first.
>>
>> There is no new kconfig. It depends on CONFIG_PREFERRED_CPU.
>> - If CONFIG_PREFERRED_CPU=y, it gets compiled as a module. It is not
>>    loaded by default.
>> - If CONFIG_PREFERRED_CPU=n, module isn't compiled.
>>
>> File layout of the driver is designed with having arch specific
>> files in the future.
>>
>> - sm_core.c - contains main driver code. This includes the periodic
>>    work function and take action on steal time.
>> - defaults.c - contains the default implementation defined with __weak
>>    symbols.
>> - sm_core.h - header file which includes data structure.
>>
>> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> 
> You've split the driver code into 12 patches. It makes it impossible
> to review the driver as a whole. Please make it less granular. 2 or 3
> patches for the new driver is more than enough, I think.
> 

Ok. I assumed splitting into more makes it easy to review.

I will plan to make it 4-5 patches.

1. Introduce the driver (include doc, skeleton code, datastructure)
    0011-virt-steal_monitor-Add-documentation.patch
    0012-virt-Introduce-steal-monitor-driver.patch
    0013-virt-steal_monitor-Restore-to-active-on-module-di.patch
    0014-virt-steal_monitor-Define-steal_monitor-structure.patch

2. Add debug knobs as module parameters. (as it is slightly longer patch with module_named_cb methods)
    0015-virt-steal_monitor-Add-control-knobs-for-handling.patch

3. Add default callback functions (i.e implemented as __weak)
    0017-virt-steal_monitor-Provide-default-method-to-get-.patch
    0018-virt-steal_monitor-Provide-default-method-to-inc-.patch
    0019-virt-steal_monitor-Provide-default-method-to-get-.patch

4. Handle periodic work and act on steal time.
    0016-virt-steal_monitor-Compute-work-at-regular-interv.patch
    0020-virt-steal_monitor-Act-on-steal-values-at-regular.patch
    0021-virt-steal_monitor-Add-direction-control.patch
    0022-virt-steal_monitor-Add-design-checks.patch

Does that look better?

>> ---
>>   drivers/virt/Makefile                |  1 +
>>   drivers/virt/steal_monitor/Makefile  | 14 ++++++++++++
>>   drivers/virt/steal_monitor/sm_core.c | 33 ++++++++++++++++++++++++++++
>>   drivers/virt/steal_monitor/sm_core.h | 11 ++++++++++
>>   4 files changed, 59 insertions(+)
>>   create mode 100644 drivers/virt/steal_monitor/Makefile
>>   create mode 100644 drivers/virt/steal_monitor/sm_core.c
>>   create mode 100644 drivers/virt/steal_monitor/sm_core.h
>>
>> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
>> index f29901bd7820..aff715cea42d 100644
>> --- a/drivers/virt/Makefile
>> +++ b/drivers/virt/Makefile
>> @@ -9,4 +9,5 @@ obj-y				+= vboxguest/
>>   
>>   obj-$(CONFIG_NITRO_ENCLAVES)	+= nitro_enclaves/
>>   obj-$(CONFIG_ACRN_HSM)		+= acrn/
>> +obj-$(CONFIG_PREFERRED_CPU)	+= steal_monitor/
>>   obj-y				+= coco/
>> diff --git a/drivers/virt/steal_monitor/Makefile b/drivers/virt/steal_monitor/Makefile
>> new file mode 100644
>> index 000000000000..24cee55342ce
>> --- /dev/null
>> +++ b/drivers/virt/steal_monitor/Makefile
>> @@ -0,0 +1,14 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +#
>> +# Steal time monitor to alter preferred CPU state.
>> +#
>> +# Arch can implement strong function definitions and override the
>> +# default by adding them in arch specific file. It must ensure
>> +# that preferred is always subset of active.
>> +#
>> +# It is always compiled as module if CONFIG_PREFERRED_CPU=y
>> +# One has to enable the module.
> 
> Why?
> 

Few Reasons on Why i made it as default no:

- Since it is co-operative scheme, admin user/root user will enable it.
   When he does that, we have asked him/her to ensure it is enabled on all
   VM's. At-least he/she will check other VM's kernel versions when enabling it.

- This periodic work has a bit of overhead. So enabling it by default, one may question
   why add it when user's don't need it. For example, in powerpc a VM can be brought up
   a dedicated. In that it is not expected to see any steal time. By same logic, having this
   module enabled in Host kernel doesn't make any sense since it will never see steal time.
   (If one compiles the same kernel for both guest and host)