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

Cleanup __store_entry_arg() so that it is easier to understand.
The main complexity may come from combining the loops for finding
stored-entry-arg and max-offset and appending new entry.

This split those different loops into 3 parts, lookup the same
entry-arg, find the max offset and append new entry.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_probe.c |   85 +++++++++++++++++++++++---------------------
 1 file changed, 44 insertions(+), 41 deletions(-)

diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 2eeecb6c95ee..b2261cab4b94 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -770,6 +770,26 @@ static int check_prepare_btf_string_fetch(char *typename,
 
 #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
 
+static void store_entry_arg_at(struct fetch_insn *code, int argnum, int offset)
+{
+	code[0].op = FETCH_OP_ARG;
+	code[0].param = argnum;
+	code[1].op = FETCH_OP_ST_EDATA;
+	code[1].offset = offset;
+}
+
+static int get_entry_arg_max_offset(struct probe_entry_arg *earg)
+{
+	int i, max_offset = 0;
+
+	for (i = 0; i < earg->size - 1 && earg->code[i].op != FETCH_OP_END; i++) {
+		if (earg->code[i].op == FETCH_OP_ST_EDATA)
+			if (earg->code[i].offset > max_offset)
+				max_offset = earg->code[i].offset;
+	}
+	return max_offset;
+}
+
 /*
  * 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.
@@ -777,7 +797,6 @@ static int check_prepare_btf_string_fetch(char *typename,
 static int __store_entry_arg(struct trace_probe *tp, int argnum)
 {
 	struct probe_entry_arg *earg = tp->entry_arg;
-	bool match = false;
 	int i, offset;
 
 	if (!earg) {
@@ -795,6 +814,8 @@ static int __store_entry_arg(struct trace_probe *tp, int argnum)
 		for (i = 0; i < earg->size; i++)
 			earg->code[i].op = FETCH_OP_END;
 		tp->entry_arg = earg;
+		store_entry_arg_at(earg->code, argnum, 0);
+		return 0;
 	}
 
 	/*
@@ -806,40 +827,34 @@ static int __store_entry_arg(struct trace_probe *tp, int argnum)
 	 * 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.
+	 * If we hit the FETCH_OP_END without matching FETCH_OP_ARG entry,
+	 * append 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) {
-		case FETCH_OP_END:
-			earg->code[i].op = FETCH_OP_ARG;
-			earg->code[i].param = argnum;
-			earg->code[i + 1].op = FETCH_OP_ST_EDATA;
-			earg->code[i + 1].offset = offset;
-			return offset;
-		case FETCH_OP_ARG:
-			match = (earg->code[i].param == argnum);
-			break;
-		case FETCH_OP_ST_EDATA:
-			offset = earg->code[i].offset;
-			if (match)
-				return offset;
-			offset += sizeof(unsigned long);
-			break;
-		default:
-			break;
-		}
+
+	/* Search the offset for the sprcified argnum. */
+	for (i = 0; i < earg->size - 1 && earg->code[i].op != FETCH_OP_END; i++) {
+		if (earg->code[i].op != FETCH_OP_ARG || earg->code[i].param != argnum)
+			continue;
+
+		if (WARN_ON_ONCE(earg->code[i + 1].op != FETCH_OP_ST_EDATA))
+			continue;
+
+		return earg->code[i + 1].offset;
 	}
-	return -ENOSPC;
+	if (i >= earg->size - 1)
+		return -ENOSPC;
+
+	/* Not found, append new entry if possible. */
+	offset = get_entry_arg_max_offset(earg) + sizeof(unsigned long);
+	store_entry_arg_at(&earg->code[i], argnum, offset);
+	return offset;
 }
 
 int traceprobe_get_entry_data_size(struct trace_probe *tp)
 {
 	struct probe_entry_arg *earg = tp->entry_arg;
-	int i, size = 0;
 
 	if (!earg)
 		return 0;
@@ -854,19 +869,7 @@ int traceprobe_get_entry_data_size(struct trace_probe *tp)
 	 * 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:
-			goto out;
-		case FETCH_OP_ST_EDATA:
-			size = earg->code[i].offset + sizeof(unsigned long);
-			break;
-		default:
-			break;
-		}
-	}
-out:
-	return size;
+	return get_entry_arg_max_offset(earg) + sizeof(unsigned long);
 }
 
 void store_trace_entry_data(void *edata, struct trace_probe *tp, struct pt_regs *regs)

On Mon,  3 Mar 2025 18:02:29 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> +	/* Search the offset for the sprcified argnum. */
> +	for (i = 0; i < earg->size - 1 && earg->code[i].op != FETCH_OP_END; i++) {
> +		if (earg->code[i].op != FETCH_OP_ARG || earg->code[i].param != argnum)
> +			continue;

I wonder if you can save a loop by changing this to:

		if (earg->code[i].op != FETCH_OP_ARG) {
			last_offset = earg->code[i].offset;
			continue;
		}

		if (earg->code[i].param != argnum)
			continue;


> +
> +		if (WARN_ON_ONCE(earg->code[i + 1].op != FETCH_OP_ST_EDATA))
> +			continue;
> +
> +		return earg->code[i + 1].offset;
>  	}
> -	return -ENOSPC;
> +	if (i >= earg->size - 1)
> +		return -ENOSPC;
> +
> +	/* Not found, append new entry if possible. */
> +	offset = get_entry_arg_max_offset(earg) + sizeof(unsigned long);

Then here you could use last_offset instead of running the loop again?

-- Steve


> +	store_entry_arg_at(&earg->code[i], argnum, offset);
> +	return offset;
>  }
>

On Thu, 6 Mar 2025 19:14:04 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon,  3 Mar 2025 18:02:29 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > +	/* Search the offset for the sprcified argnum. */
> > +	for (i = 0; i < earg->size - 1 && earg->code[i].op != FETCH_OP_END; i++) {
> > +		if (earg->code[i].op != FETCH_OP_ARG || earg->code[i].param != argnum)
> > +			continue;
> 
> I wonder if you can save a loop by changing this to:
> 
> 		if (earg->code[i].op != FETCH_OP_ARG) {
> 			last_offset = earg->code[i].offset;
> 			continue;
> 		}
> 
> 		if (earg->code[i].param != argnum)
> 			continue;
> 
> 
> > +
> > +		if (WARN_ON_ONCE(earg->code[i + 1].op != FETCH_OP_ST_EDATA))
> > +			continue;
> > +
> > +		return earg->code[i + 1].offset;
> >  	}
> > -	return -ENOSPC;
> > +	if (i >= earg->size - 1)
> > +		return -ENOSPC;
> > +
> > +	/* Not found, append new entry if possible. */
> > +	offset = get_entry_arg_max_offset(earg) + sizeof(unsigned long);
> 
> Then here you could use last_offset instead of running the loop again?

Yeah, but I would like it easier to read and safer (adding WARN_ON)
especially, 

> 		if (earg->code[i].op != FETCH_OP_ARG) {
> 			last_offset = earg->code[i].offset;
> 			continue;
> 		}

this is a bit unsafe if we introduce another operation except for
the FETCH_OP_ARG and FETCH_OP_ST_EDATA. (and it is expecting the
offset is always increased.)

Thank you,

> 
> -- Steve
> 
> 
> > +	store_entry_arg_at(&earg->code[i], argnum, offset);
> > +	return offset;
> >  }
> >  


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