út 30. 6. 2026 v 16:00 odesílatel Valentin Schneider
<[email protected]> napsal:
>
> On 30/06/26 12:14, Tomas Glozar wrote:
> > st 17. 6. 2026 v 15:18 odesílatel Valentin Schneider
> > <[email protected]> napsal:
> >> @@ -406,6 +408,33 @@ struct osnoise_tool *osnoise_init_top(struct
> >> common_params *params)
> >> goto out_err;
> >> }
> >>
> >> + /*
> >> + * If tracing on a subset of possible CPUs, leverage the kernel
> >> filtering
> >> + * infrastructure to only generate events on traced CPUs.
> >> + */
> >> + if (params->cpus) {
> >> + char filter[MAX_PATH];
> >> +
> >> + snprintf(filter, ARRAY_SIZE(filter), "cpu & CPUS{%s}\n",
> >> params->cpus);
> >> + retval = tracefs_event_file_write(tool->trace.inst,
> >> + "ipi", "ipi_send_cpu",
> >> "filter",
> >> + filter);
> >> + if (retval) {
> >
> > retval is the number of bytes written here, so this should be "retval
> > < 0" like in trace_event_enable_filter() in trace.c. Same below.
> >
>
> According to the docstring:
>
> * Return 0 on success, and -1 on error.
>
> but regardless yes that should be a '< 0' check to match existing code.
>
I double-checked that and you are correct that the docstring says so,
but it's an error in the docstring. According to the manpage, it
returns the number of bytes written (i.e. positive on success, not
zero) [1]:
"RETURN VALUE
...
tracefs_event_file_write() and tracefs_event_file_append() returns
*the number of bytes written to the system/event file* or negative on
error."
The code agrees as well: in tracefs_event_file_write() there's the
wrong docstring (likely copied from another function) [2]:
/*
* tracefs_event_file_write - write to an event file
* ...
* Return 0 on success, and -1 on error.
*/
int tracefs_event_file_write(struct tracefs_instance *instance,
const char *system, const char *event,
const char *file, const char *str)
{
....
ret = tracefs_instance_file_write(instance, path, str);
free(path);
return ret;
}
but the source of the return value is tracefs_instance_file_write(),
where the docstring is correct [3]:
/**
* tracefs_instance_file_write - Write in trace file of specific instance.
* ...
* Returns the number of written bytes, or -1 in case of an error
*/
int tracefs_instance_file_write(struct tracefs_instance *instance,
const char *file, const char *str)
{
return instance_file_write(instance, file, str, O_WRONLY | O_TRUNC);
}
instance_file_write() gets the return value from write_file() [4]
which returns the return value of write() [5].
[1] https://man7.org/linux/man-pages/man3/tracefs_event_get_file.3.html
[2]
https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/tree/src/tracefs-events.c#n686
[3]
https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/tree/src/tracefs-instance.c#n532
[4]
https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/tree/src/tracefs-instance.c#n514
[5]
https://git.kernel.org/pub/scm/libs/libtrace/libtracefs.git/tree/src/tracefs-instance.c#n496
So the "< 0" is required, the CPU filter doesn't work without it:
[tglozar@fedora rtla]$ uname -a
Linux fedora 7.0.12-101.fc43.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Jun 11
01:32:26 UTC 2026 x86_64 GNU/Linux
[tglozar@fedora rtla]$ sudo ./rtla osnoise top -q -c 0 --ipi
Could not set ipi_send_cpu CPU filter, return value: 14
Could not init osnoise tool
[tglozar@fedora rtla]$ sudo ./rtla osnoise top -q -d 5s --ipi
Operating System Noise
...
[tglozar@fedora rtla]$
(output with additional debug print)
> >> + err_msg("Could not set ipi_send_cpu CPU filter\n");
> >> + goto out_err;
> >
> > It would be useful to have --ipi work even on older kernels that don't
> > yet have your cpumask trace event filter patchset [1], for example, by
> > printing a debug message that filtering is disabled and setting a flag
> > instead of erroring out here. Then the code in
> > osnoise_ipi_cpu_handler() can preserve the CPU_ISSET check if the flag
> > is set.
> >
> > As --ipi is optional, we can choose to only support it on newer
> > kernels, but it would be nice to have it working without the filter,
> > too.
> >
> > [1]
> > https://lore.kernel.org/linux-trace-kernel/[email protected]/T/#u
> >
>
> Makes sense, will do.
>
Thanks!
> [truncated]
> >
> > I was thinking that it might make sense to enable the filters also for
> > the trace output instance. On the other hand, it would make it
> > difficult to enable the event without the filter then, as specifying
> > "-e ipi" or similar only re-enables the event but does not remove the
> > filter. Maybe the better idea is to implement an option to filter any
> > event enabled through -e/--event only to the measurement CPU, as a
> > separate feature.
> >
>
> I had actually forgotten about applying the filters for the output
> instance... I'll look into it.
>
Thanks. I gave it some more thought and realized enabling the event
without the filter should not be complicated at all. We can just
remove existing filters in trace_events_enable(), as
trace_events_enable() is called after osnoise_init_trace_tool() in
run_tool(). So that will make an explicit "-e ipi" drop the filter
from "--ipi" on the trace instance and show all IPI events. So you can
disregard my note about filtering -e options, it's not relevant here.
Tomas
