Re: [PATCH] libtracecmd: Fix continuing in tracecmd_iterate_events_reverse()

On Fri, 2025-11-21 at 12:09 -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
> 
> When a callback from tracecmd_iterate_events_reverse() returns non-zero it
> cause the function to exit out early. Calling tracecmd_iterate_events_reverse()
> again with cont=true, is supposed to restart where it left off. But
> because of the way the reverse traverses all the events on a page, by
> calling tracecmd_read_data() to find the next task, it does not pick the
> next task. It can pick the next task at the start of the page.
> 
> If the callback back causes the function to exit out early, record the
> page_offset of the last record read, and then set the cursor back to that
> event.
> 
> Link: https://lore.kernel.org/all/[email protected]/
> 
> Fixes: 56cbc522518a5 ("trace-cmd library: Add tracecmd_iterate_events_reverse() API")
> Signed-off-by: Steven Rostedt (Google) <[email protected]>
> ---
>  lib/trace-cmd/trace-input.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/lib/trace-cmd/trace-input.c b/lib/trace-cmd/trace-input.c
> index afdbd2aa98b6..8dfeff458485 100644
> --- a/lib/trace-cmd/trace-input.c
> +++ b/lib/trace-cmd/trace-input.c
> @@ -3036,6 +3036,7 @@ int tracecmd_iterate_events_reverse(struct tracecmd_input *handle,
>  				    void *callback_data, bool cont)
>  {
>  	unsigned long long last_timestamp = 0;
> +	unsigned long long page_offset = 0;
>  	struct tep_record **records;
>  	struct tep_record *record;
>  	int next_cpu;
> @@ -3072,6 +3073,8 @@ int tracecmd_iterate_events_reverse(struct tracecmd_input *handle,
>  			record = next_last_event(handle, records, next_cpu);;
>  			ret = call_callbacks(handle, record, next_cpu,
>  					     callback, callback_data);
> +			if (ret)
> +				page_offset = record->offset;
>  			tracecmd_free_record(record);
>  		}
>  	} while (next_cpu >= 0 && ret == 0);
> @@ -3079,6 +3082,17 @@ int tracecmd_iterate_events_reverse(struct tracecmd_input *handle,
>  	free_last_events(handle, records, cpus, cpu_size, max_cpus);
>  	free(records);
>  
> +	/*
> +	 * If the callback exited out early, then set the cursor back
> +	 * to the location of that record so that if this gets called
> +	 * again with cont = true, it will continue where it left off.
> +	 */
> +	if (page_offset) {
> +		/* Set the record to the previous record that was read */
> +		record = tracecmd_read_at(handle, page_offset - 4, NULL);

Hi,

where does the magic "- 4" come from?

> +		tracecmd_free_record(record);

I still don't get why this fixes the issue. Does tracecmd_read_at has a
side-effect to set the the internal cursor? If so, it IMHO would help
to document this side effect in tracecmd_read_at().

Felix

> +	}
> +
>  	return ret;
>  }
>  
> -- 
> 2.51.0

-- 
Siemens AG
Linux Expert Center
Friedrich-Ludwig-Bauer-Str. 3
85748 Garching, Germany