On 01/07/26 08:45, Tomas Glozar wrote:
> ú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)
>
Darnit, you're right! I'm surprised I didn't catch this while
testing. Thanks!
>> >> + 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.
>
That makes sense, that's the most intuitive option.
> Tomas
