diff mbox series

tracing: add trace_seq_reset function

Message ID 20240122182225.69061-2-ricardo@marliere.net (mailing list archive)
State Superseded
Headers show
Series tracing: add trace_seq_reset function | expand

Commit Message

Ricardo B. Marliere Jan. 22, 2024, 6:22 p.m. UTC
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.

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

Comments

Steven Rostedt Jan. 22, 2024, 10:10 p.m. UTC | #1
On Mon, 22 Jan 2024 15:22:25 -0300
"Ricardo B. Marliere" <ricardo@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 <ricardo@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;
>  }
Ricardo B. Marliere Jan. 22, 2024, 10:45 p.m. UTC | #2
On 22 Jan 17:10, Steven Rostedt wrote:
> On Mon, 22 Jan 2024 15:22:25 -0300
> "Ricardo B. Marliere" <ricardo@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 <ricardo@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
Steven Rostedt Jan. 22, 2024, 10:57 p.m. UTC | #3
On Mon, 22 Jan 2024 19:45:41 -0300
"Ricardo B. Marliere" <ricardo@marliere.net> wrote:

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

No it would be permanent. And no, it is *not* a "trace_seq_init_or_reset".
That WARN_ON_ONCE() means this should never happen, but if it does, the
if () statement keeps it from crashing.

If we later make it dynamic, where there is no s->buffer, that would then
change to:

	if (WARN_ON_ONCE(!s->seq.size))
		seq_buf_init(&s->seq, NULL, 0);
	else
		seq_buf_clear(&s->seq);

Where the trace_seq is basically disabled from that point on.

I would try to fix everything, but this will find those places you missed. ;-)

The kernel is like this, because it's not like any other application. If an
application crashes, you may get annoyed, and just restart it. If the
kernel crashes, you can easily lose data and you have to reboot your
machine. Thus, we treat things that could cause harm much more carefully.

We do not want to crash the kernel just because we missed a location that
did not initialize trace_seq. That's why this would be a permanent change.

-- Steve
diff mbox series

Patch

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);			\
 }									\
 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;
 }