Re: [PATCH] tracing: add trace_seq_reset function

[Ricardo B. Marliere]

On 22 Jan 17:10, Steven Rostedt wrote:
> On Mon, 22 Jan 2024 15:22:25 -0300
> "Ricardo B. Marliere" <ricardo@xxxxxxxxxxxx> 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.
> 

Hi, Steve

Certainly. I also forgot to add your Suggested-by.

> 
> > Signed-off-by: Ricardo B. Marliere <ricardo@xxxxxxxxxxxx>
> > ---
> >  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().

That makes sense, I will work on a v2.

> 
> 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.

But that would be temporary, right? Kind of a "trace_seq_init_or_reset".
If that's the case it would be best to simply work out all the places
instead. Would the current tests [1] cover everything or should
something else be made to make sure no place was missing from the patch?

Thanks for reviewing!
-	Ricardo

--
[1] https://github.com/rostedt/ftrace-ktests