Re: [RFC PATCH v4 4/5] perf data: Add --to-trace-dat option for converting perf.data tracepoint events into trace.dat format

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Trace parser desynchronization: Large event array lengths must be encoded as the fully padded size minus the length word.
- [High] Out of bounds rejection: CPU array limits must be scoped to `nr_cpus_avail` rather than `nr_cpus_online`.
- [High] Silent I/O truncation: Stream checks must explicitly assert identical element counts instead of binary negation.
- [High] Architecture violation: Page header offsets must dynamically query the ftrace event format to accommodate 32-bit `local_t` structures.
- [Low] Build architecture violation: The `data-convert.h` header must include a dummy inline fallback stub for `trace_convert__perf2dat`.
- [Low] Dead code elimination: The `skipped` sample counter is incremented but structurally isolated and unread.
--

--- Patch [4]: [RFC PATCH v4 4/5] perf data: Add --to-trace-dat option for converting perf.data tracepoint events into trace.dat format ---
commit 9a258038767861211db47cd6d942b553034516f1
Author: Tanushree Shah <[email protected]>

perf data: Add --to-trace-dat option for converting perf.data tracepoint events into trace.dat format

Add new command-line option to perf data convert for generating
trace.dat output files.

> diff --git a/tools/perf/builtin-data.c b/tools/perf/builtin-data.c
> index 1dd73ed6bdcb8..c9c863197d02c 100644
> --- a/tools/perf/builtin-data.c
> +++ b/tools/perf/builtin-data.c
[ ... ]
> @@ -63,10 +72,44 @@ static int cmd_data_convert(int argc, const char **argv)
[ ... ]
> +#ifdef HAVE_LIBTRACEEVENT
> +	if (trace_dat_output)
> +		return trace_convert__perf2dat(input_name ? input_name : "perf.data",
> +					       trace_dat_output, &opts);
> +#endif

[Severity: Low]
Does the data-convert.h header need a dummy inline fallback stub for
trace_convert__perf2dat()?

Without an #else block providing an inline dummy stub, could this cause a
build failure when compiling without HAVE_LIBTRACEEVENT?

> diff --git a/tools/perf/util/data-convert-trace.c b/tools/perf/util/data-convert-trace.c
> index 445479fae8884..425b3aaa3f026 100644
> --- a/tools/perf/util/data-convert-trace.c
> +++ b/tools/perf/util/data-convert-trace.c
> @@ -31,6 +32,10 @@
>  struct trace_convert {
>  	struct perf_tool tool;
>  	u64 events_count;
> +	struct perf_time_interval *ptime_range;
> +	int range_size;
> +	int range_num;
> +	u64 skipped;
>  };

[Severity: Low]
Is this skipped counter ever read?

It is incremented when events are outside the specified time range, but
appears to be structurally isolated and never emitted in the final
conversion summary.

> @@ -86,6 +91,11 @@ static int process_sample_event(const struct perf_tool *tool,
>  		cpu_buffers_initialized = true;
>  	}
>  
> +	if (perf_time__ranges_skip_sample(tc->ptime_range, tc->range_num, sample->time)) {
> +		tc->skipped++;
> +		return 0;
> +	}
> +

[Severity: High]
Are valid samples from sparse CPU topologies rejected here?

In process_sample_event(), the CPU buffers are initialized using
nr_cpus_online:
    int nr_cpus = trace_dat_session->header.env.nr_cpus_online;

But trace_dat__collect_cpu_event() rejects samples if the CPU index is >=
trace_dat_nr_cpus:
    if (!trace_cpu_data || cpu < 0 || cpu >= trace_dat_nr_cpus)
        return -EINVAL;

If a system has hotplugged CPUs (e.g. CPUs 0 and 127 are online, making
nr_cpus_online 2), a valid sample from CPU 127 will be evaluated against
cpu >= 2. This causes trace_dat__collect_cpu_event() to return -EINVAL,
aborting the conversion.

Should this use nr_cpus_avail instead of nr_cpus_online?

> diff --git a/tools/perf/util/trace-dat.c b/tools/perf/util/trace-dat.c
> index f71e03716e27e..1b236745bcacc 100644
> --- a/tools/perf/util/trace-dat.c
> +++ b/tools/perf/util/trace-dat.c
[ ... ]
> @@ -288,7 +292,7 @@ static int trace_dat__write_cpu_dat(FILE *fp, struct tep_handle *pevent,
>  		/* Check page fit BEFORE allocating data record */
>  		if (page_size_used + needed_size >
>  			trace_dat_page_size - TRACE_DAT_RECORD_HEADER_SIZE) {
> -			ret = trace_dat__write_page(fp, pevent, base_ts,
> +			ret = trace_dat__write_page(fp, pevent, page_base_ts,
>  					page_records, page_rec_sizes,
>  					nr_page_recs);

[Severity: High]
Could the length word for large trace events be miscalculated?

Later in trace_dat__write_cpu_dat(), the array length is encoded:

    /* Large events: write actual byte length after header */
    if (type_len == 0) {
        unsigned int data_len_out = to_file_u32(pevent, data_len);
        memcpy(data_rec + TRACE_DAT_WORD_SIZE, &data_len_out, TRACE_DAT_WORD_SIZE);
    }

For large events (>=29 words), the ftrace ring buffer format dictates that
the first word of the payload contains the length of the data plus the
length word itself (data_len + 4).

By writing just data_len, might this cause downstream tools like trace-cmd
to compute the event size incorrectly and lose parser synchronization?

[Severity: High]
Can a short write silently corrupt the trace.dat file?

In trace_dat__write_page(), the file write checks for a boolean false:

    if (!fwrite(page, 1, trace_dat_page_size, fp)) {
        free(page);
        return -EIO;
    }

Since fwrite() returns the number of elements written, a short write (e.g.
512 out of 4096 bytes) evaluates to true.

Will this silently drop data without reporting an error to the user if the
disk runs out of space? Should the stream check explicitly assert
fwrite(...) == trace_dat_page_size?

[Severity: High]
Will this hardcoded page header size break parsing for trace data recorded
on 32-bit architectures?

In trace_dat__write_page(), the page header offsets are hardcoded:

    /* Byte-swap page header for cross-arch compatibility */
    ts_out = to_file_u64(pevent, base_ts);
    commit_out = to_file_u64(pevent, commit);
    memcpy(page, &ts_out, sizeof(ts_out));
    memcpy(page + sizeof(ts_out), &commit_out, sizeof(commit_out));

The ftrace page header includes a local_t commit field, which is 8 bytes on
64-bit systems but 4 bytes on 32-bit systems (making the header 12 bytes).
Because this unconditionally assumes a 16-byte header and writes 8 bytes for
commit_out, trace-cmd reading 32-bit metadata will likely parse the remaining
4 bytes as the first trace event.

Should the page header offsets and sizing be dynamically queried from the
ftrace event format?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4