On Mon, 22 Jan 2024 15:22:25 -0300
"Ricardo B. Marliere" <rica...@marliere.net> wrote:
> Currently, trace_seq_init may be called many times with the intent of
> resetting the buffer. Add a function trace_seq_reset that does that and
> replace the relevant occurrences to use it instead.
>
Hi Ricardo!
It's also OK to add to the commit log that the goal of this is to later
extend trace_seq to be more flexible in the size of the buffer it holds. To
do that, we need to differentiate between initializing a trace_seq and just
resetting it.
> Signed-off-by: Ricardo B. Marliere <rica...@marliere.net>
> ---
> include/linux/trace_seq.h | 8 ++++++++
> include/trace/trace_events.h | 2 +-
> kernel/trace/trace.c | 8 ++++----
> kernel/trace/trace_seq.c | 2 +-
> 4 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h
> index 9ec229dfddaa..c28e0ccb50bd 100644
> --- a/include/linux/trace_seq.h
> +++ b/include/linux/trace_seq.h
> @@ -29,6 +29,14 @@ trace_seq_init(struct trace_seq *s)
> s->readpos = 0;
> }
>
> +static inline void
> +trace_seq_reset(struct trace_seq *s)
> +{
> + seq_buf_clear(&s->seq);
> + s->full = 0;
> + s->readpos = 0;
> +}
> +
> /**
> * trace_seq_used - amount of actual data written to buffer
> * @s: trace sequence descriptor
> diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
> index c2f9cabf154d..2bc79998e5ab 100644
> --- a/include/trace/trace_events.h
> +++ b/include/trace/trace_events.h
> @@ -227,7 +227,7 @@ trace_raw_output_##call(struct trace_iterator *iter, int
> flags, \
> \
> field = (typeof(field))entry; \
> \
> - trace_seq_init(p); \
> + trace_seq_reset(p); \
> return trace_output_call(iter, #call, print); \
Note, p = &iter->tmp_seq
where it has never been initialized. To do this, we need to add:
trace_seq_init(&iter->tmp_seq);
where ever iter is created. You need to look at all the locations where
iter is created ("iter =") and differentiate where it is first used from
just passing pointers around.
The iter = kzalloc() will be easy, but for example, both seq and tmp_seq
need to be initialized in tracing_buffers_open().
Perhaps we need a:
if (WARN_ON_ONCE(!s->seq.size))
seq_buf_init(&s->seq, s->buffer, TRACE_SEQ_BUFFER_SIZE);
else
seq_buf_clear(&s->seq);
in the trace_seq_reset() to catch any place that doesn't have it
initialized yet.
-- Steve
> } \
> static struct trace_event_functions trace_event_type_funcs_##call = {
> \
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 46dbe22121e9..9147f3717b9a 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -6946,7 +6946,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
> /* reset all but tr, trace, and overruns */
> trace_iterator_reset(iter);
> cpumask_clear(iter->started);
> - trace_seq_init(&iter->seq);
> + trace_seq_reset(&iter->seq);
>
> trace_event_read_lock();
> trace_access_lock(iter->cpu_file);
> @@ -6993,7 +6993,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf,
> /* Now copy what we have to the user */
> sret = trace_seq_to_user(&iter->seq, ubuf, cnt);
> if (iter->seq.readpos >= trace_seq_used(&iter->seq))
> - trace_seq_init(&iter->seq);
> + trace_seq_reset(&iter->seq);
>
> /*
> * If there was nothing to send to user, in spite of consuming trace
> @@ -7125,7 +7125,7 @@ static ssize_t tracing_splice_read_pipe(struct file
> *filp,
> spd.partial[i].offset = 0;
> spd.partial[i].len = trace_seq_used(&iter->seq);
>
> - trace_seq_init(&iter->seq);
> + trace_seq_reset(&iter->seq);
> }
>
> trace_access_unlock(iter->cpu_file);
> @@ -10274,7 +10274,7 @@ trace_printk_seq(struct trace_seq *s)
>
> printk(KERN_TRACE "%s", s->buffer);
>
> - trace_seq_init(s);
> + trace_seq_reset(s);
> }
>
> void trace_init_global_iter(struct trace_iterator *iter)
> diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c
> index c158d65a8a88..741b2f3d76c0 100644
> --- a/kernel/trace/trace_seq.c
> +++ b/kernel/trace/trace_seq.c
> @@ -59,7 +59,7 @@ int trace_print_seq(struct seq_file *m, struct trace_seq *s)
> * do something else with the contents.
> */
> if (!ret)
> - trace_seq_init(s);
> + trace_seq_reset(s);
>
> return ret;
> }
