Re: [PATCH v3 08/17] verification/rvgen: Organise Kconfig entries for nested monitors

[Nam Cao <namcao@linutronix.de>]

On Tue, Jul 15, 2025 at 09:14:25AM +0200, Gabriele Monaco wrote:
> The current behaviour of rvgen when running with the -a option is to
> append the necessary lines at the end of the configuration for Kconfig,
> Makefile and tracepoints.
> This is not always the desired behaviour in case of nested monitors:
> while tracepoints are not affected by nesting and the Makefile's only
> requirement is that the parent monitor is built before its children, in
> the Kconfig it is better to have children defined right after their
> parent, otherwise the result has wrong indentation:
> 
> [*]   foo_parent monitor
> [*]     foo_child1 monitor
> [*]     foo_child2 monitor
> [*]   bar_parent monitor
> [*]     bar_child1 monitor
> [*]     bar_child2 monitor
> [*]   foo_child3 monitor
> [*]   foo_child4 monitor
> 
> Adapt rvgen to look for a different marker for nested monitors in the
> Kconfig file and append the line right after the last sibling, instead
> of the last monitor.
> Also add the marker when creating a new parent monitor.
> 
> Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
> ---
>  kernel/trace/rv/Kconfig                     |  5 +++++
>  tools/verification/rvgen/rvgen/container.py | 13 +++++++++++++
>  tools/verification/rvgen/rvgen/generator.py | 13 ++++++++-----
>  3 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/trace/rv/Kconfig b/kernel/trace/rv/Kconfig
> index c11bf7e61ebf0..26017378f79b8 100644
> --- a/kernel/trace/rv/Kconfig
> +++ b/kernel/trace/rv/Kconfig
> @@ -43,6 +43,7 @@ config RV_PER_TASK_MONITORS
>  
>  source "kernel/trace/rv/monitors/wip/Kconfig"
>  source "kernel/trace/rv/monitors/wwnr/Kconfig"
> +
>  source "kernel/trace/rv/monitors/sched/Kconfig"
>  source "kernel/trace/rv/monitors/tss/Kconfig"
>  source "kernel/trace/rv/monitors/sco/Kconfig"
> @@ -50,9 +51,13 @@ source "kernel/trace/rv/monitors/snroc/Kconfig"
>  source "kernel/trace/rv/monitors/scpd/Kconfig"
>  source "kernel/trace/rv/monitors/snep/Kconfig"
>  source "kernel/trace/rv/monitors/sncid/Kconfig"
> +# Add new sched monitors here
> +
>  source "kernel/trace/rv/monitors/rtapp/Kconfig"
>  source "kernel/trace/rv/monitors/pagefault/Kconfig"
>  source "kernel/trace/rv/monitors/sleep/Kconfig"
> +# Add new rtapp monitors here
> +
>  # Add new monitors here
>  
>  config RV_REACTORS
> diff --git a/tools/verification/rvgen/rvgen/container.py b/tools/verification/rvgen/rvgen/container.py
> index 47d8ab2ad3ec4..fee493f89fde6 100644
> --- a/tools/verification/rvgen/rvgen/container.py
> +++ b/tools/verification/rvgen/rvgen/container.py
> @@ -20,3 +20,16 @@ class Container(generator.RVGenerator):
>          main_h = self.main_h
>          main_h = main_h.replace("%%MODEL_NAME%%", self.name)
>          return main_h
> +
> +    def fill_kconfig_tooltip(self):
> +        """Override to produce a marker for this container in the Kconfig"""
> +        container_marker = f"# Add new {self.name} monitors here\n"
> +        if self.auto_patch:
> +            self._patch_file("Kconfig",
> +                            "# Add new monitors here", "")

Isn't this one excessive? I gave it a try, but I had double blank line:

python3 tools/verification/rvgen -a container -n hello

   61 source "kernel/trace/rv/monitors/sleep/Kconfig"
   62 # Add new rtapp monitors here
   63 
+  64 
+  65 source "kernel/trace/rv/monitors/hello/Kconfig"
+  66 # Add new hello monitors here
+  67 
   68 # Add new monitors here
   69 
   70 config RV_REACTORS

> +        result = super().fill_kconfig_tooltip()
> +        if self.auto_patch:
> +            self._patch_file("Kconfig",
> +                            "# Add new monitors here", container_marker)
> +            return result
> +        return result + container_marker
> diff --git a/tools/verification/rvgen/rvgen/generator.py b/tools/verification/rvgen/rvgen/generator.py
> index 19d0078a38032..ac97c4505ff00 100644
> --- a/tools/verification/rvgen/rvgen/generator.py
> +++ b/tools/verification/rvgen/rvgen/generator.py
> @@ -137,7 +137,8 @@ class RVGenerator:
>          kconfig = kconfig.replace("%%MONITOR_DEPS%%", monitor_deps)
>          return kconfig
>  
> -    def __patch_file(self, file, marker, line):
> +    def _patch_file(self, file, marker, line):
> +        assert(self.auto_patch)
>          file_to_patch = os.path.join(self.rv_dir, file)
>          content = self._read_file(file_to_patch)
>          content = content.replace(marker, line + "\n" + marker)
> @@ -146,7 +147,7 @@ class RVGenerator:
>      def fill_tracepoint_tooltip(self):
>          monitor_class_type = self.fill_monitor_class_type()
>          if self.auto_patch:
> -            self.__patch_file("rv_trace.h",
> +            self._patch_file("rv_trace.h",
>                              "// Add new monitors based on CONFIG_%s here" % monitor_class_type,
>                              "#include <monitors/%s/%s_trace.h>" % (self.name, self.name))
>              return "  - Patching %s/rv_trace.h, double check the result" % self.rv_dir
> @@ -158,8 +159,10 @@ Add this line where other tracepoints are included and %s is defined:
>  
>      def fill_kconfig_tooltip(self):
>          if self.auto_patch:
> -            self.__patch_file("Kconfig",
> -                            "# Add new monitors here",
> +            # monitors with a container should stay together in the Kconfig
> +            self._patch_file("Kconfig",
> +                            "# Add new %smonitors here" %
> +                              (self.parent + " " if self.parent else ""),

This one must be kept in sync with container_marker in
Container.fill_kconfig_tooltip(). I think this is hard to maintain later
on.

How about we keep in centralized with a helper function, something like:

def container_marker(container: str) -> str:
    return f"# Add new {container} monitors here\n"

Nam