On 29/06/26 14:56, Tomas Glozar wrote:
> st 17. 6. 2026 v 15:18 odesÃlatel Valentin Schneider
> <[email protected]> napsal:
>> +/*
>> + * osnoise_ipi_cpu_handler - this is the handler for single CPU IPI events.
>> + */
>> +static int
>> +osnoise_ipi_cpu_handler(struct trace_seq *s, struct tep_record *record,
>> + struct tep_event *event, void *context)
>> +{
>> + struct osnoise_tool *tool;
>> + struct osnoise_params *params;
>> + unsigned long long src_cpu, dst_cpu;
>> + struct trace_instance *trace = context;
>> +
>> + tool = container_of(trace, struct osnoise_tool, trace);
>> + params = to_osnoise_params(tool->params);
>> +
>> + src_cpu = record->cpu;
>> + tep_get_field_val(s, event, "cpu", record, &dst_cpu, 1);
>> +
>> + if (CPU_ISSET(dst_cpu, ¶ms->common.monitored_cpus))
>> + account_ipi(tool, src_cpu, dst_cpu);
>
> Do we need to retrieve and pass the src_cpu here? I get it if you plan
> on using it in the future, but as far as I understand, you are
> specifically tracking the destination CPU, not the source CPU. Same
> note applies to osnoise_ipi_cpumask_handler() below.
>
You're right, I fished out the src_cpu to have it available but it's not
being used ATM.
>> +
>> + return 0;
>> +}
>> +
>> +static cpu_set_t cpumask_tmp_cpus;
>> +
>> +/*
>> + * osnoise_ipi_cpumask_handler - this is the handler for broadcasted IPI
>> events.
>> + */
>> +static int
>> +osnoise_ipi_cpumask_handler(struct trace_seq *s, struct tep_record *record,
>> + struct tep_event *event, void *context)
>> +{
>> + struct trace_instance *trace = context;
>> + struct osnoise_tool *tool;
>> + struct osnoise_params *params;
>> + struct tep_format_field *field;
>> + unsigned long long src_cpu;
>> + cpu_set_t *event_cpus;
>> + int len;
>> +
>> + tool = container_of(trace, struct osnoise_tool, trace);
>> + params = to_osnoise_params(tool->params);
>> +
>> + src_cpu = record->cpu;
>> +
>> + field = tep_find_field(event, "cpumask");
>> + if (!field)
>> + return 0;
>> +
>> + event_cpus = tep_get_field_raw(s, event, "cpumask", record, &len, 1);
>> + if (!event_cpus) {
>> + err_msg("Failed to get cpumask field\n");
>> + return 0;
>> + }
>> +
>> + CPU_AND(&cpumask_tmp_cpus, event_cpus,
>> ¶ms->common.monitored_cpus);
>> +
>> + /*
>> + * Computing the mask weight is overkill but there is no leaner
>> option
>> + * provided by glibc, e.g cpumask_first() or somesuch.
>> + */
>> + if (CPU_COUNT(&cpumask_tmp_cpus)) {
>> + for (int cpu = 0; cpu < nr_cpus; cpu++) {
>> + if (CPU_ISSET(cpu, &cpumask_tmp_cpus))
>> + account_ipi(tool, src_cpu, cpu);
>> + }
>> + }
>
> Technically, the existing code already relies on the glibc cpumask
> implementation (cpu_set_t) matching the kernel "cpumask_t" type, as
> the "cpumask" field is the latter (per
> /sys/kernel/tracing/events/ipi/ipi_send_cpumask/format), not the
> former. So I wouldn't worry about the opaqueness of cpu_set_t much.
>
Right, AFAICT that's the "canonical" type for passing cpumasks around
between userspace and kernelspace. e.g. for sched_getaffinity():
manpage:
int sched_getaffinity(pid_t pid, size_t cpusetsize,
cpu_set_t *mask);
kernelside:
SYSCALL_DEFINE3(sched_getaffinity, pid_t, pid, unsigned int, len,
unsigned long __user *, user_mask_ptr)
{
cpumask_var_t mask;
sched_getaffinity(pid, mask);
copy_to_user(user_mask_ptr, cpumask_bits(mask), ...)
}
> Not sure how this is handled in other tracing tools that need to use
> cpumask, I'd have to look around a bit. It might even make sense to
> have a "tools" version of the cpumask functions like cpumask_first(),
> I guess, like we already do for e.g. lists and container_of.
>
I couldn't find anything in tools/testing/* other than the CPU_*() helpers.
>> +
>> + return 0;
>> +}
>> +
>> /*
>> * osnoise_top_handler - this is the handler for osnoise tracer events
>> */
>
> Nit: As this is extra functionality, it'd be more readable to have the
> IPI handling after the main top handler, so that someone not familiar
> with the source code will see the core logic first. That would also
> match IPI being displayed to the right of the other numbers in the top
> output.
>
Ack.
>> @@ -164,6 +251,8 @@ static void osnoise_top_header(struct osnoise_tool *top)
>> goto eol;
>>
>> trace_seq_printf(s, " IRQ Softirq Thread");
>> + if (params->common.ipi)
>> + trace_seq_printf(s, " IPI");
>>
>> eol:
>> if (pretty)
>> @@ -218,7 +307,13 @@ static void osnoise_top_print(struct osnoise_tool
>> *tool, int cpu)
>>
>> trace_seq_printf(s, "%12llu ", cpu_data->irq_count);
>> trace_seq_printf(s, "%12llu ", cpu_data->softirq_count);
>> - trace_seq_printf(s, "%12llu\n", cpu_data->thread_count);
>> + trace_seq_printf(s, "%12llu", cpu_data->thread_count);
>> + if (!params->common.ipi) {
>> + trace_seq_printf(s, "\n");
>> + return;
>> + }
>> +
>> + trace_seq_printf(s, " %12llu\n", cpu_data->ipi_count);
>
> Maybe at this point it is worth it to print the "\n" in a separate
> statement, readability-wise:
>
> trace_seq_printf(s, "%12llu ", cpu_data->irq_count);
> trace_seq_printf(s, "%12llu ", cpu_data->softirq_count);
> trace_seq_printf(s, "%12llu", cpu_data->thread_count);
> if (params->common.ipi)
> trace_seq_printf(s, " %12llu", cpu_data->ipi_count);
> trace_seq_printf(s, "\n");
>
> It would also make diffs nicer when adding new options.
>
Indeed, will do.
>> [truncated]
>
>
> Tomas
