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

[Gabriele Monaco <gmonaco@redhat.com>]

On Tue, 2025-07-15 at 16:48 +0200, Nam Cao wrote:
> 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
> 

Yeah indeed it is, thanks for finding out, it somehow slipped my
tests..

> > +        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"
> 

Good point, will do.

Thanks,
Gabriele