On Wed, 2025-08-27 at 13:33 +0200, Tomas Glozar wrote:
> čt 21. 8. 2025 v 5:58 odesílatel Crystal Wood <crw...@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 <tglo...@redhat.com>
Thanks!
-Crystal
