tracing: probe-events: Add comments about entry data storing code

[PATCH] tracing: probe-events: Add comments about entry data storing code

Posted by Masami Hiramatsu (Google) 11 months, 2 weeks ago

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Add comments about entry data storing code to __store_entry_arg() and
traceprobe_get_entry_data_size(). These are a bit complicated because of
building the entry data storing code and scanning it.

This just add comments, no behavior change.

Reported-by: Steven Rostedt <rostedt@goodmis.org>
Closes: https://lore.kernel.org/all/20250226102223.586d7119@gandalf.local.home/
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_probe.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 8f58ee1e8858..2eeecb6c95ee 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -770,6 +770,10 @@ static int check_prepare_btf_string_fetch(char *typename,
 
 #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
 
+/*
+ * Add the entry code to store the 'argnum'th parameter and return the offset
+ * in the entry data buffer where the data will be stored.
+ */
 static int __store_entry_arg(struct trace_probe *tp, int argnum)
 {
 	struct probe_entry_arg *earg = tp->entry_arg;
@@ -793,6 +797,20 @@ static int __store_entry_arg(struct trace_probe *tp, int argnum)
 		tp->entry_arg = earg;
 	}
 
+	/*
+	 * The entry code array is repeating the pair of
+	 * [FETCH_OP_ARG(argnum)][FETCH_OP_ST_EDATA(offset of entry data buffer)]
+	 * and the rest of entries are filled with [FETCH_OP_END].
+	 *
+	 * To reduce the redundant function parameter fetching, we scan the entry
+	 * code array to find the FETCH_OP_ARG which already fetches the 'argnum'
+	 * parameter. If it doesn't match, update 'offset' to find the last
+	 * offset.
+	 * If we find the FETCH_OP_END without matching FETCH_OP_ARG entry, we
+	 * will save the entry with FETCH_OP_ARG and FETCH_OP_ST_EDATA, and
+	 * return data offset so that caller can find the data offset in the entry
+	 * data buffer.
+	 */
 	offset = 0;
 	for (i = 0; i < earg->size - 1; i++) {
 		switch (earg->code[i].op) {
@@ -826,6 +844,16 @@ int traceprobe_get_entry_data_size(struct trace_probe *tp)
 	if (!earg)
 		return 0;
 
+	/*
+	 * earg->code[] array has an operation sequence which is run in
+	 * the entry handler.
+	 * The sequence stopped by FETCH_OP_END and each data stored in
+	 * the entry data buffer by FETCH_OP_ST_EDATA. The FETCH_OP_ST_EDATA
+	 * stores the data at the data buffer + its offset, and all data are
+	 * "unsigned long" size. The offset must be increased when a data is
+	 * stored. Thus we need to find the last FETCH_OP_ST_EDATA in the
+	 * code array.
+	 */
 	for (i = 0; i < earg->size; i++) {
 		switch (earg->code[i].op) {
 		case FETCH_OP_END:

Re: [PATCH] tracing: probe-events: Add comments about entry data storing code

Posted by Steven Rostedt 11 months, 1 week ago

On Thu, 27 Feb 2025 09:45:50 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Add comments about entry data storing code to __store_entry_arg() and
> traceprobe_get_entry_data_size(). These are a bit complicated because of
> building the entry data storing code and scanning it.
> 
> This just add comments, no behavior change.
> 
> Reported-by: Steven Rostedt <rostedt@goodmis.org>
> Closes: https://lore.kernel.org/all/20250226102223.586d7119@gandalf.local.home/
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  kernel/trace/trace_probe.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 8f58ee1e8858..2eeecb6c95ee 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -770,6 +770,10 @@ static int check_prepare_btf_string_fetch(char *typename,
>  
>  #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
>  
> +/*
> + * Add the entry code to store the 'argnum'th parameter and return the offset
> + * in the entry data buffer where the data will be stored.
> + */
>  static int __store_entry_arg(struct trace_probe *tp, int argnum)
>  {
>  	struct probe_entry_arg *earg = tp->entry_arg;
> @@ -793,6 +797,20 @@ static int __store_entry_arg(struct trace_probe *tp, int argnum)
>  		tp->entry_arg = earg;
>  	}
>  
> +	/*
> +	 * The entry code array is repeating the pair of
> +	 * [FETCH_OP_ARG(argnum)][FETCH_OP_ST_EDATA(offset of entry data buffer)]
> +	 * and the rest of entries are filled with [FETCH_OP_END].
> +	 *
> +	 * To reduce the redundant function parameter fetching, we scan the entry
> +	 * code array to find the FETCH_OP_ARG which already fetches the 'argnum'
> +	 * parameter. If it doesn't match, update 'offset' to find the last
> +	 * offset.
> +	 * If we find the FETCH_OP_END without matching FETCH_OP_ARG entry, we
> +	 * will save the entry with FETCH_OP_ARG and FETCH_OP_ST_EDATA, and
> +	 * return data offset so that caller can find the data offset in the entry
> +	 * data buffer.
> +	 */

So I'm still not 100% sure what is happening here, but at least these
comments can help me figure it out if I spend some time gawking at the code
a bit more ;-)

Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

-- Steve


>  	offset = 0;
>  	for (i = 0; i < earg->size - 1; i++) {
>  		switch (earg->code[i].op) {
> @@ -826,6 +844,16 @@ int traceprobe_get_entry_data_size(struct trace_probe *tp)
>  	if (!earg)
>  		return 0;
>  
> +	/*
> +	 * earg->code[] array has an operation sequence which is run in
> +	 * the entry handler.
> +	 * The sequence stopped by FETCH_OP_END and each data stored in
> +	 * the entry data buffer by FETCH_OP_ST_EDATA. The FETCH_OP_ST_EDATA
> +	 * stores the data at the data buffer + its offset, and all data are
> +	 * "unsigned long" size. The offset must be increased when a data is
> +	 * stored. Thus we need to find the last FETCH_OP_ST_EDATA in the
> +	 * code array.
> +	 */
>  	for (i = 0; i < earg->size; i++) {
>  		switch (earg->code[i].op) {
>  		case FETCH_OP_END:

Re: [PATCH] tracing: probe-events: Add comments about entry data storing code

Posted by Masami Hiramatsu (Google) 11 months, 1 week ago

On Fri, 28 Feb 2025 17:47:12 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 27 Feb 2025 09:45:50 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Add comments about entry data storing code to __store_entry_arg() and
> > traceprobe_get_entry_data_size(). These are a bit complicated because of
> > building the entry data storing code and scanning it.
> > 
> > This just add comments, no behavior change.
> > 
> > Reported-by: Steven Rostedt <rostedt@goodmis.org>
> > Closes: https://lore.kernel.org/all/20250226102223.586d7119@gandalf.local.home/
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  kernel/trace/trace_probe.c |   28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index 8f58ee1e8858..2eeecb6c95ee 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -770,6 +770,10 @@ static int check_prepare_btf_string_fetch(char *typename,
> >  
> >  #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> >  
> > +/*
> > + * Add the entry code to store the 'argnum'th parameter and return the offset
> > + * in the entry data buffer where the data will be stored.
> > + */
> >  static int __store_entry_arg(struct trace_probe *tp, int argnum)
> >  {
> >  	struct probe_entry_arg *earg = tp->entry_arg;
> > @@ -793,6 +797,20 @@ static int __store_entry_arg(struct trace_probe *tp, int argnum)
> >  		tp->entry_arg = earg;
> >  	}
> >  
> > +	/*
> > +	 * The entry code array is repeating the pair of
> > +	 * [FETCH_OP_ARG(argnum)][FETCH_OP_ST_EDATA(offset of entry data buffer)]
> > +	 * and the rest of entries are filled with [FETCH_OP_END].
> > +	 *
> > +	 * To reduce the redundant function parameter fetching, we scan the entry
> > +	 * code array to find the FETCH_OP_ARG which already fetches the 'argnum'
> > +	 * parameter. If it doesn't match, update 'offset' to find the last
> > +	 * offset.
> > +	 * If we find the FETCH_OP_END without matching FETCH_OP_ARG entry, we
> > +	 * will save the entry with FETCH_OP_ARG and FETCH_OP_ST_EDATA, and
> > +	 * return data offset so that caller can find the data offset in the entry
> > +	 * data buffer.
> > +	 */
> 
> So I'm still not 100% sure what is happening here, but at least these
> comments can help me figure it out if I spend some time gawking at the code
> a bit more ;-)

I think the code is a bit complicated too. I will rewrite it a bit.

> 
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Thanks you!

> 
> -- Steve
> 
> 
> >  	offset = 0;
> >  	for (i = 0; i < earg->size - 1; i++) {
> >  		switch (earg->code[i].op) {
> > @@ -826,6 +844,16 @@ int traceprobe_get_entry_data_size(struct trace_probe *tp)
> >  	if (!earg)
> >  		return 0;
> >  
> > +	/*
> > +	 * earg->code[] array has an operation sequence which is run in
> > +	 * the entry handler.
> > +	 * The sequence stopped by FETCH_OP_END and each data stored in
> > +	 * the entry data buffer by FETCH_OP_ST_EDATA. The FETCH_OP_ST_EDATA
> > +	 * stores the data at the data buffer + its offset, and all data are
> > +	 * "unsigned long" size. The offset must be increased when a data is
> > +	 * stored. Thus we need to find the last FETCH_OP_ST_EDATA in the
> > +	 * code array.
> > +	 */
> >  	for (i = 0; i < earg->size; i++) {
> >  		switch (earg->code[i].op) {
> >  		case FETCH_OP_END:
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>