On 22 Jan 17:10, Steven Rostedt wrote:
> 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.
>
Hi, Steve
Certainly. I also forgot to add your Suggested-by.
>
> > 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().
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
