Re: [PATCH 3/7] tools/rtla: Create common_apply_config()

[Crystal Wood <crwood@redhat.com>]

On Wed, 2025-08-27 at 13:33 +0200, Tomas Glozar wrote:
> čt 21. 8. 2025 v 5:58 odesílatel Crystal Wood <crwood@redhat.com> napsal:
> > At some point it would be nice to have all of the common code named and
> > located correctly, but it's a start.  Do we want to stick with "common" or
> > go with something less vague like "osn" for things that relate to the
> > broader osnoise mechanism rather than the specific osnoise tracer?
> 
> For functions that set tracer options that reside in
> /sys/kernel/tracing/osnoise and are used by both osnoise and timerlat
> tracers (like osnoise_set_cpus and osnoise_set_workload), I think we
> can call them tracer options, and make the function names
> "tracer_set_cpus" etc. Or just use "common", that seems fine to me,
> too.

That could add confusion though, when saying things like "the specific
osnoise tracer".

> 
> > +++ b/tools/tracing/rtla/src/common.c
> > @@ -0,0 +1,64 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#define _GNU_SOURCE
> 
> Nit: the newline is unnecessary after the SPDX identifier, other rtla
> source files that don't start with a copyright comment after the SPDX
> identifier (e.g. timerlat_bpf.c) don't have it.

It just looked weird without it, since when there is a block comment at
the top, we usually do get a blank line before the code starts.  I don't
really care either way, though.

> 
> > @@ -44,4 +103,12 @@  struct common_params {
> >  int output_divisor;
> >  int pretty_output;
> >  int quiet;
> > + int kernel_workload;
> >  };
> 
> It stood out to me that kernel_workload is moved to common, while
> user_workload is not. On a second look though, osnoise has to make
> sure kernel workload is created, but has no user workload option, so
> it makes sense.

FWIW, user_workload moves to common in the next patch.

> 
> Reviewed-by: Tomas Glozar <tglozar@redhat.com>

Thanks!

-Crystal