On 22 Jan 17:10, Steven Rostedt wrote:

On Mon, 22 Jan 2024 15:22:25 -0300
"Ricardo B. Marliere" [off-list ref] wrote:

quoted

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.

quoted

Signed-off-by: Ricardo B. Marliere <redacted>
---
 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