tracing: Hist trigger var-ref cleanup and comments

From: Tom Zanussi <[email protected]>

There's no need to use strlen() for static strings when the length is already known, so update trace_events_hist.c with sizeof() for those cases.

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

kernel/trace/trace_events_hist.c +3 -3

index d29bf8a8e1dd..25d06b3ae1f6 100644
@@ -507,7 +507,7 @@ static int synth_field_string_size(char *type)
 	start = strstr(type, "char[");
 	if (start == NULL)
 		return -EINVAL;
-	start += strlen("char[");
+	start += sizeof("char[") - 1;
 
 	end = strchr(type, ']');
 	if (!end || end < start)

@@ -1843,8 +1843,8 @@ static int parse_action(char *str, struct hist_trigger_attrs *attrs)
 	if (attrs->n_actions >= HIST_ACTIONS_MAX)
 		return ret;
 
-	if ((strncmp(str, "onmatch(", strlen("onmatch(")) == 0) ||
-	    (strncmp(str, "onmax(", strlen("onmax(")) == 0)) {
+	if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) ||
+	    (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
 		attrs->action_str[attrs->n_actions] = kstrdup(str, GFP_KERNEL);
 		if (!attrs->action_str[attrs->n_actions]) {
 			ret = -ENOMEM;

kernel/trace/trace_events_hist.c +14 -14

 		if (!attrs->keys_str) {
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if ((strncmp(str, "val=", strlen("val=")) == 0) ||
-		 (strncmp(str, "vals=", strlen("vals=")) == 0) ||
-		 (strncmp(str, "values=", strlen("values=")) == 0)) {
+	} else if ((strncmp(str, "val=", sizeof("val=") - 1) == 0) ||
+		 (strncmp(str, "vals=", sizeof("vals=") - 1) == 0) ||
+		 (strncmp(str, "values=", sizeof("values=") - 1) == 0)) {
 		attrs->vals_str = kstrdup(str, GFP_KERNEL);
 		if (!attrs->vals_str) {
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if (strncmp(str, "sort=", strlen("sort=")) == 0) {
+	} else if (strncmp(str, "sort=", sizeof("sort=") - 1) == 0) {
 		attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
 		if (!attrs->sort_key_str) {
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if (strncmp(str, "name=", strlen("name=")) == 0) {
+	} else if (strncmp(str, "name=", sizeof("name=") - 1) == 0) {
 		attrs->name = kstrdup(str, GFP_KERNEL);
 		if (!attrs->name) {
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if (strncmp(str, "clock=", strlen("clock=")) == 0) {
+	} else if (strncmp(str, "clock=", sizeof("clock=") - 1) == 0) {
 		strsep(&str, "=");
 		if (!str) {
 			ret = -EINVAL;

@@ -1901,7 +1901,7 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if (strncmp(str, "size=", strlen("size=")) == 0) {
+	} else if (strncmp(str, "size=", sizeof("size=") - 1) == 0) {
 		int map_bits = parse_map_size(str);
 
 		if (map_bits < 0) {

@@ -3497,7 +3497,7 @@ static struct action_data *onmax_parse(char *str)
 	if (!onmax_fn_name || !str)
 		goto free;
 
-	if (strncmp(onmax_fn_name, "save", strlen("save")) == 0) {
+	if (strncmp(onmax_fn_name, "save", sizeof("save") - 1) == 0) {
 		char *params = strsep(&str, ")");
 
 		if (!params) {

@@ -4302,8 +4302,8 @@ static int parse_actions(struct hist_trigger_data *hist_data)
 	for (i = 0; i < hist_data->attrs->n_actions; i++) {
 		str = hist_data->attrs->action_str[i];
 
-		if (strncmp(str, "onmatch(", strlen("onmatch(")) == 0) {
-			char *action_str = str + strlen("onmatch(");
+		if (strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) {
+			char *action_str = str + sizeof("onmatch(") - 1;
 
 			data = onmatch_parse(tr, action_str);
 			if (IS_ERR(data)) {

@@ -4311,8 +4311,8 @@ static int parse_actions(struct hist_trigger_data *hist_data)
 				break;
 			}
 			data->fn = action_trace;
-		} else if (strncmp(str, "onmax(", strlen("onmax(")) == 0) {
-			char *action_str = str + strlen("onmax(");
+		} else if (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0) {
+			char *action_str = str + sizeof("onmax(") - 1;
 
 			data = onmax_parse(action_str);
 			if (IS_ERR(data)) {

@@ -5548,9 +5548,9 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 			p++;
 			continue;
 		}
-		if (p >= param + strlen(param) - strlen("if") - 1)
+		if (p >= param + strlen(param) - (sizeof("if") - 1) - 1)
 			return -EINVAL;
-		if (*(p + strlen("if")) != ' ' && *(p + strlen("if")) != '\t') {
+		if (*(p + sizeof("if") - 1) != ' ' && *(p + sizeof("if") - 1) != '\t') {
 			p++;
 			continue;
 		}

Quoted 43 lines · On Tue, 18 Dec 2018 14:33:21 -0600 Tom Zanussi <[email protected]> wrote:

From: Tom Zanussi <[email protected]>

There's no need to use strlen() for static strings when the length is
already known, so update trace_events_hist.c with sizeof() for those
cases.

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++---------
----------
 1 file changed, 19 insertions(+), 19 deletions(-)

kernel/trace/trace_events_hist.c +2 -3

index d29bf8a8e1dd..25d06b3ae1f6 100644
@@ -507,7 +507,7 @@ static int synth_field_string_size(char *type)
 	start = strstr(type, "char[");
 	if (start == NULL)
 		return -EINVAL;
-	start += strlen("char[");
+	start += sizeof("char[") - 1;
 
 	end = strchr(type, ']');
 	if (!end || end < start)

@@ -1843,8 +1843,8 @@ static int parse_action(char *str, struct
 hist_trigger_attrs *attrs)
 	if (attrs->n_actions >= HIST_ACTIONS_MAX)
 		return ret;
 
-	if ((strncmp(str, "onmatch(", strlen("onmatch(")) == 0) ||
-	    (strncmp(str, "onmax(", strlen("onmax(")) == 0)) {
+	if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) ==

0) ||

kernel/trace/trace_events_hist.c +1 -0

+	    (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
 		attrs->action_str[attrs->n_actions] = kstrdup(str,

GFP_KERNEL);

kernel/trace/trace_events_hist.c +0 -0

 		if (!attrs->action_str[attrs->n_actions]) {
 			ret = -ENOMEM;

kernel/trace/trace_events_hist.c static int parse_assignment(char *str, · line 1867

Quoted 9 lines

kernel/trace/trace_events_hist.c +2 -2

@@ -1861,34 +1861,34 @@ static int parse_assignment(char *str,
 struct hist_trigger_attrs *attrs)
 {
 	int ret = 0;
 
-	if ((strncmp(str, "key=", strlen("key=")) == 0) ||
-	    (strncmp(str, "keys=", strlen("keys=")) == 0)) {
+	if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
+	    (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
 		attrs->keys_str = kstrdup(str, GFP_KERNEL);

I'll apply this as is, but since there's a lot of these, I wonder if we
should make a marcro for this:

#define strcmp_const(str, str_const) strncmp(str, str_const, sizeof(str_const) - 1)

?

This would help prevent bugs due to typos and bad cut and paste.

Hi Steve,

Quoted 70 lines · Steven Rostedt

Quoted 49 lines · On Tue, 18 Dec 2018 14:33:21 -0600 Tom Zanussi <[email protected]> wrote:

From: Tom Zanussi <[email protected]>

There's no need to use strlen() for static strings when the length
is
already known, so update trace_events_hist.c with sizeof() for those
cases.

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++-------
--
----------
 1 file changed, 19 insertions(+), 19 deletions(-)

kernel/trace/trace_events_hist.c +1 -1

index d29bf8a8e1dd..25d06b3ae1f6 100644
@@ -507,7 +507,7 @@ static int synth_field_string_size(char
 *type)
 	start = strstr(type, "char[");
 	if (start == NULL)
 		return -EINVAL;
-	start += strlen("char[");
+	start += sizeof("char[") - 1;
 
 	end = strchr(type, ']');

kernel/trace/trace_events_hist.c +0 -1

 	if (!end || end < start)

@@ -1843,8 +1843,8 @@ static int parse_action(char *str, struct
 hist_trigger_attrs *attrs)
 	if (attrs->n_actions >= HIST_ACTIONS_MAX)
 		return ret;
 
-	if ((strncmp(str, "onmatch(", strlen("onmatch(")) == 0)

||

kernel/trace/trace_events_hist.c +1 -1

-	    (strncmp(str, "onmax(", strlen("onmax(")) == 0)) {
+	if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) ==

0) ||

kernel/trace/trace_events_hist.c +1 -0

+	    (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0))

{
attrs->action_str[attrs->n_actions] =
kstrdup(str,
GFP_KERNEL);

kernel/trace/trace_events_hist.c +0 -0

 		if (!attrs->action_str[attrs->n_actions]) {
 			ret = -ENOMEM;

kernel/trace/trace_events_hist.c static int parse_assignment(char *str, · line 1867

Quoted 9 lines

kernel/trace/trace_events_hist.c +2 -2

@@ -1861,34 +1861,34 @@ static int parse_assignment(char *str,
 struct hist_trigger_attrs *attrs)
 {
 	int ret = 0;
 
-	if ((strncmp(str, "key=", strlen("key=")) == 0) ||
-	    (strncmp(str, "keys=", strlen("keys=")) == 0)) {
+	if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
+	    (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
 		attrs->keys_str = kstrdup(str, GFP_KERNEL);

I'll apply this as is, but since there's a lot of these, I wonder if
we
should make a marcro for this:

#define strcmp_const(str, str_const) strncmp(str, str_const, sizeof(str_const) - 1)

Quoted 4 lines

?

This would help prevent bugs due to typos and bad cut and paste.

Yeah, I had considered it but wasn't sure it was worth it. Since you're suggesting it is, I can send another patch on top of these, or feel free if you want to too. ;-)

Tom

How's this?

[PATCH] tracing: Introduce and use strcmp_const() for hist triggers

Provide a new strcmp_const() macro and make use of it instead of the longer and more error-prone strncmp(str, "str", sizeof("str") - 1).

Quoted 7 lines · Tom Zanussi

How's this?

[PATCH] tracing: Introduce and use strcmp_const() for hist triggers

Provide a new strcmp_const() macro and make use of it instead of the longer and more error-prone strncmp(str, "str", sizeof("str") - 1).

[]

kernel/trace/trace_events_hist.c

Quoted 1 line

kernel/trace/trace_events_hist.c +0 -0

[]

Quoted 2 lines

kernel/trace/trace_events_hist.c +0 -0

@@ -22,6 +22,9 @@
 

kernel/trace/trace_events_hist.c line 26

Quoted 4 lines

kernel/trace/trace_events_hist.c +2 -0

 #define STR_VAR_LEN_MAX		32 /* must be multiple of sizeof(u64) */
 
+#define strcmp_const(str, str_const) \
+	strncmp(str, str_const, sizeof(str_const) - 1)  

Not good as it's too easy to pass a pointer as str_const and sizeof(pointer) - 1 isn't likely the string length.

Quoted 12 lines · On Wed, 19 Dec 2018 12:22:38 -0800 Joe Perches <[email protected]> wrote:

Quoted 6 lines · Tom Zanussi

How's this?

[PATCH] tracing: Introduce and use strcmp_const() for hist triggers

Provide a new strcmp_const() macro and make use of it instead of the longer and more error-prone strncmp(str, "str", sizeof("str") - 1).

[]

kernel/trace/trace_events_hist.c

Quoted 1 line

kernel/trace/trace_events_hist.c +0 -0

[]

Quoted 2 lines

kernel/trace/trace_events_hist.c +0 -0

@@ -22,6 +22,9 @@
 

Quoted 7 lines

kernel/trace/trace_events_hist.c line 26

Quoted 4 lines

kernel/trace/trace_events_hist.c +2 -0

 #define STR_VAR_LEN_MAX		32 /* must be multiple of sizeof(u64) */
 
+#define strcmp_const(str, str_const) \
+	strncmp(str, str_const, sizeof(str_const) - 1)  

Not good as it's too easy to pass a pointer as str_const and sizeof(pointer) - 1 isn't likely the string length.

Agreed. And I noticed that this is used all over the kernel, so I'm not going to add this patch. I'm going to add a:

#define strncmp_prefix(str, prefix) \
strncmp(str, prefix, strlen(prefix))

in include/linux/string.h

And go around and use that throughout the kernel. By doing a quick grep, I already spotted a few bugs.

Quoted 7 lines

#define strncmp_prefix(str, prefix) \
strncmp(str, prefix, strlen(prefix))

in include/linux/string.h

And go around and use that throughout the kernel. By doing a quick grep, I already spotted a few bugs.

I hope you also convert the existing uses like

	strncmp(str1, "str2", 4)

where the length value is precalculated to the strlen of the const string

But there seem to be _a lot_ of those...

$ git grep -P "\bstrncmp\s*\([^,]+,[^,]+,\s*\d+\s*\)" | wc -l
1681

Quoted 4 lines

Yeah, I had considered it but wasn't sure it was worth it. Since you're suggesting it is, I can send another patch on top of these, or feel free if you want to too. ;-)

How's this?

[PATCH] tracing: Introduce and use strcmp_const() for hist triggers

Provide a new strcmp_const() macro and make use of it instead of the longer and more error-prone strncmp(str, "str", sizeof("str") - 1).

Idea-and-macro-by Steve Rostedt <[email protected]>

Signed-off-by Tom Zanussi <[email protected]>

---

Hi Steve,

Quoted 63 lines · Steven Rostedt

Quoted 43 lines · On Tue, 18 Dec 2018 14:33:21 -0600 Tom Zanussi <[email protected]> wrote:

From: Tom Zanussi <[email protected]>

There's no need to use strlen() for static strings when the length is
already known, so update trace_events_hist.c with sizeof() for those
cases.

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++---------
----------
 1 file changed, 19 insertions(+), 19 deletions(-)

kernel/trace/trace_events_hist.c +2 -3

index d29bf8a8e1dd..25d06b3ae1f6 100644
@@ -507,7 +507,7 @@ static int synth_field_string_size(char *type)
 	start = strstr(type, "char[");
 	if (start == NULL)
 		return -EINVAL;
-	start += strlen("char[");
+	start += sizeof("char[") - 1;
 
 	end = strchr(type, ']');
 	if (!end || end < start)

@@ -1843,8 +1843,8 @@ static int parse_action(char *str, struct
 hist_trigger_attrs *attrs)
 	if (attrs->n_actions >= HIST_ACTIONS_MAX)
 		return ret;
 
-	if ((strncmp(str, "onmatch(", strlen("onmatch(")) == 0) ||
-	    (strncmp(str, "onmax(", strlen("onmax(")) == 0)) {
+	if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) ==

0) ||

kernel/trace/trace_events_hist.c +1 -0

+	    (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
 		attrs->action_str[attrs->n_actions] = kstrdup(str,

GFP_KERNEL);

kernel/trace/trace_events_hist.c +0 -0

 		if (!attrs->action_str[attrs->n_actions]) {
 			ret = -ENOMEM;

kernel/trace/trace_events_hist.c static int parse_assignment(char *str, · line 1867

Quoted 9 lines

kernel/trace/trace_events_hist.c +2 -2

@@ -1861,34 +1861,34 @@ static int parse_assignment(char *str,
 struct hist_trigger_attrs *attrs)
 {
 	int ret = 0;
 
-	if ((strncmp(str, "key=", strlen("key=")) == 0) ||
-	    (strncmp(str, "keys=", strlen("keys=")) == 0)) {
+	if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
+	    (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
 		attrs->keys_str = kstrdup(str, GFP_KERNEL);

I'll apply this as is, but since there's a lot of these, I wonder if we
should make a marcro for this:

#define strcmp_const(str, str_const) strncmp(str, str_const, sizeof(str_const) - 1)

Quoted 4 lines

?

This would help prevent bugs due to typos and bad cut and paste.

Yeah, I had considered it but wasn't sure it was worth it. Since you're suggesting it is, I can send another patch on top of these, or feel free if you want to too. ;-)

Quoted 3 lines

Yeah, I had considered it but wasn't sure it was worth it. Since you're suggesting it is, I can send another patch on top of these, or feel free if you want to too. ;-)

I believe the 'strlen("foo") -> sizeof("foo") - 1' conversions do not change objects at all.

strlen("constant") is already optimized by gcc to a constant value when fed a constant string.

the strcmp_const macro does seem to make sense as the copy/paste/typo possibility is real.

Quoted 8 lines · On Wed, 19 Dec 2018 12:20:19 -0800 Joe Perches <[email protected]> wrote:

Quoted 4 lines

Yeah, I had considered it but wasn't sure it was worth it. Since you're suggesting it is, I can send another patch on top of these, or
feel free if you want to too. ;-)

I believe the 'strlen("foo") -> sizeof("foo") - 1' conversions do not change objects at all.

Quoted 2 lines

strlen("constant") is already optimized by gcc to a constant value when fed a constant string.

If that's the case (and it probably is), then yeah, strlen is probably
better. As it can handle the "not a constant" that you stated in another email.

-- Steve

Quoted 3 lines

the strcmp_const macro does seem to make sense as the copy/paste/typo possibility is real.

Quoted 14 lines · Steven Rostedt

Quoted 10 lines · On Wed, 19 Dec 2018 12:20:19 -0800 Joe Perches <[email protected]> wrote:

Quoted 4 lines

Yeah, I had considered it but wasn't sure it was worth it. Since you're suggesting it is, I can send another patch on top of these, or
feel free if you want to too. ;-)

I believe the 'strlen("foo") -> sizeof("foo") - 1' conversions do not change objects at all.

strlen("constant") is already optimized by gcc to a constant value when fed a constant string.

Quoted 5 lines

If that's the case (and it probably is), then yeah, strlen is probably
better. As it can handle the "not a constant" that you stated in another email.

OK, so I guess that means we should just drop this patch ('[PATCH 2/7]
tracing: Change strlen to sizeof for hist trigger static strings').

Quoted 21 lines · On Wed, 19 Dec 2018 14:38:39 -0600 Tom Zanussi <[email protected]> wrote:

Quoted 19 lines · Steven Rostedt

Quoted 10 lines · On Wed, 19 Dec 2018 12:20:19 -0800 Joe Perches <[email protected]> wrote:

Quoted 4 lines

Yeah, I had considered it but wasn't sure it was worth it. Since you're suggesting it is, I can send another patch on top of these, or
feel free if you want to too. ;-)

I believe the 'strlen("foo") -> sizeof("foo") - 1' conversions do not change objects at all.

strlen("constant") is already optimized by gcc to a constant value when fed a constant string.

If that's the case (and it probably is), then yeah, strlen is probably
better. As it can handle the "not a constant" that you stated in another email.

Quoted 3 lines

OK, so I guess that means we should just drop this patch ('[PATCH 2/7]
tracing: Change strlen to sizeof for hist trigger static strings').

No, that patch is fine, the macro was not. I've already applied your patch set. Just need to run it through my tests.

Yeah, I had considered it but wasn't sure it was worth it. Since you're suggesting it is, I can send another patch on top of these, or feel free if you want to too. ;-)

From 664457f5cc776aa624502bc628285ea6d70ac8c9 Mon Sep 17 00:00:00 2001

From: Tom Zanussi <[email protected]>

Since all the variable reference hist_fields are collected into hist_data->var_refs[] array, there's no need to go through all the fields looking for them, or in separate arrays like synth_var_refs[], which will be going away soon anyway.

This also allows us to get rid of some unnecessary code and functions currently used for the same purpose.

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace_events_hist.c | 57 +++++-----------------------------------
 1 file changed, 7 insertions(+), 50 deletions(-)

kernel/trace/trace_events_hist.c +1 -33

index 25d06b3ae1f6..ee839c52bd3f 100644
@@ -1299,45 +1299,11 @@ check_field_for_var_ref(struct hist_field *hist_field,
 {
 	struct hist_field *found = NULL;
 
-	if (hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF) {
-		if (hist_field->var.idx == var_idx &&
-		    hist_field->var.hist_data == var_data) {
-			found = hist_field;
-		}
-	}
-
-	return found;
-}
-
-static struct hist_field *
-check_field_for_var_refs(struct hist_trigger_data *hist_data,
-			 struct hist_field *hist_field,
-			 struct hist_trigger_data *var_data,
-			 unsigned int var_idx,
-			 unsigned int level)
-{
-	struct hist_field *found = NULL;
-	unsigned int i;
-
-	if (level > 3)
-		return found;
-
-	if (!hist_field)
-		return found;
+	WARN_ON(!(hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF));
 
-	found = check_field_for_var_ref(hist_field, var_data, var_idx);
-	if (found)
-		return found;
-
-	for (i = 0; i < HIST_FIELD_OPERANDS_MAX; i++) {
-		struct hist_field *operand;
-
-		operand = hist_field->operands[i];

kernel/trace/trace_events_hist.c +0 -8

 }

@@ -1349,18 +1315,9 @@ static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,
 	struct hist_field *hist_field, *found = NULL;
 	unsigned int i;
 
-	for_each_hist_field(i, hist_data) {
-		hist_field = hist_data->fields[i];
-		found = check_field_for_var_refs(hist_data, hist_field,
-						 var_data, var_idx, 0);
-		if (found)
-			return found;
-	}
-

Signature · 1 line

2.14.1

Hi Tom,

Quoted 11 lines · On Tue, 18 Dec 2018 14:33:22 -0600 Tom Zanussi <[email protected]> wrote:

From: Tom Zanussi <[email protected]>

Since all the variable reference hist_fields are collected into hist_data->var_refs[] array, there's no need to go through all the fields looking for them, or in separate arrays like synth_var_refs[],
which will be going away soon anyway.

This also allows us to get rid of some unnecessary code and functions
currently used for the same purpose.

I just have a nitpick.

Quoted 66 lines

Signed-off-by Tom Zanussi <[email protected]>

---
 kernel/trace/trace_events_hist.c | 57 +++++-----------------------
------------
 1 file changed, 7 insertions(+), 50 deletions(-)

kernel/trace/trace_events_hist.c +4 -38

index 25d06b3ae1f6..ee839c52bd3f 100644
@@ -1299,45 +1299,11 @@ check_field_for_var_ref(struct hist_field
 *hist_field,
 {
 	struct hist_field *found = NULL;
 
-	if (hist_field && hist_field->flags &
 HIST_FIELD_FL_VAR_REF) {
-		if (hist_field->var.idx == var_idx &&
-		    hist_field->var.hist_data == var_data) {
-			found = hist_field;
-		}
-	}
-
-	return found;
-}
-
-static struct hist_field *
-check_field_for_var_refs(struct hist_trigger_data *hist_data,
-			 struct hist_field *hist_field,
-			 struct hist_trigger_data *var_data,
-			 unsigned int var_idx,
-			 unsigned int level)
-{
-	struct hist_field *found = NULL;
-	unsigned int i;
-
-	if (level > 3)
-		return found;
-
-	if (!hist_field)
-		return found;
+	WARN_ON(!(hist_field && hist_field->flags &
 HIST_FIELD_FL_VAR_REF));
 
-	found = check_field_for_var_ref(hist_field, var_data,
 var_idx);
-	if (found)
-		return found;
-
-	for (i = 0; i < HIST_FIELD_OPERANDS_MAX; i++) {
-		struct hist_field *operand;
-
-		operand = hist_field->operands[i];
-		found = check_field_for_var_refs(hist_data,
 operand, var_data,
-						 var_idx, level +
 1);
-		if (found)
-			return found;
-	}
+	if (hist_field && hist_field->var.idx == var_idx &&
+	    hist_field->var.hist_data == var_data)
+		found = hist_field;

Quoted 1 line

return found;

It seems we don't need "found" var here. Just return hist_field or NULL.

Quoted 9 lines

kernel/trace/trace_events_hist.c check_field_for_var_ref(struct hist_field *hist_field, · line 1308

Quoted 5 lines

kernel/trace/trace_events_hist.c +3 -0

+	if (hist_field && hist_field->var.idx == var_idx &&
+	    hist_field->var.hist_data == var_data)
+		found = hist_field;
 
 	return found;  

It seems we don't need "found" var here. Just return hist_field or NULL.

OK, will change these and resubmit shortly.

Quoted 4 lines · On Wed, 19 Dec 2018 09:01:43 -0600 Tom Zanussi <[email protected]> wrote:

Quoted 4 lines

Quoted 4 lines

kernel/trace/trace_events_hist.c +3 -0

+	if (hist_field && hist_field->var.idx == var_idx &&
+	    hist_field->var.hist_data == var_data)
+		found = hist_field;

Quoted 8 lines

Quoted 6 lines

Quoted 1 line

return found;

It seems we don't need "found" var here. Just return hist_field or
NULL.

OK, will change these and resubmit shortly.

Tom,

If this is the only patch in the series that needs updating, can you just reply to this patch with the v2 of 3/7? That is have a subject of:

[PATCH v2 3/7] tracing: Use var_refs[] for hist trigger reference checking

and reply to the [PATCH 3/7]

This isn't the normal way of updates, but I want to start taking this patches in ASAP, and I figured this may be the easiest for both of us.

It's fine to send a v2 of the entire patch series (which other maintainers require), so I'll leave it up to you.

If there's any other patch that needs changing, then a full v2 series is required. But if this is the only change and you want to send just this patch, then I'll take that as well.

-- Steve

From: Tom Zanussi <[email protected]>

Hi,

This patchset is a standalone series broken out of the v8 version of the 'tracing: Hist trigger snapshot and onchange additions' patchset.

It's a series of changes resulting from some suggestions from Namhyung for making the variable-reference handling code more understandable through some refactoring and comments.

It also added a new patch changing all strlen() to sizeof() for string constants, in trace_events_hist.c

Also, in the 'tracing: Remove open-coding of hist trigger var_ref management' patch, in create_var_ref(), moved the saving of ref_field and update of ref_field->var_ref_idx into the 'if' as pointed out by Dan Carpenter/smatch 0-day robot.

It doesn't introduce any functional changes and can be applied independently of the other patchset.

The following changes since commit 5d6ddf6acce68d1290112cb08b12fd78b201e7d5:

arm64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack (2018-12-08 22:21:31 -0500)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/hist-var-ref-cleanup-v1

Tom Zanussi (7):
tracing: Remove unnecessary hist trigger struct field
tracing: Change strlen to sizeof for hist trigger static strings
tracing: Use var_refs[] for hist trigger reference checking
tracing: Remove open-coding of hist trigger var_ref management
tracing: Use hist trigger's var_ref array to destroy var_refs
tracing: Remove hist trigger synth_var_refs
tracing: Add hist trigger comments for variable-related fields

 kernel/trace/trace_events_hist.c | 267 +++++++++++++++++++++++----------------
 1 file changed, 156 insertions(+), 111 deletions(-)

Signature · 1 line

2.14.1

Hi Tom,

Quoted 25 lines · Tom Zanussi

From: Tom Zanussi <[email protected]>

Hi,

This patchset is a standalone series broken out of the v8 version of
the 'tracing: Hist trigger snapshot and onchange additions' patchset.

It's a series of changes resulting from some suggestions from Namhyung
for making the variable-reference handling code more understandable through some refactoring and comments.

It also added a new patch changing all strlen() to sizeof() for string
constants, in trace_events_hist.c

Also, in the 'tracing: Remove open-coding of hist trigger var_ref management' patch, in create_var_ref(), moved the saving of ref_field
and update of ref_field->var_ref_idx into the 'if' as pointed out by
Dan Carpenter/smatch 0-day robot.

Quoted 2 lines

It doesn't introduce any functional changes and can be applied independently of the other patchset.

Acked-by Namhyung Kim <[email protected]>

From: Tom Zanussi <[email protected]>

Hi,

This patchset is a standalone series broken out of the v8 version of the 'tracing: Hist trigger snapshot and onchange additions' patchset.

It's a series of changes resulting from some suggestions from Namhyung for making the variable-reference handling code more understandable through some refactoring and comments.

It also added a new patch changing all strlen() to sizeof() for string constants, in trace_events_hist.c

Also, in the 'tracing: Remove open-coding of hist trigger var_ref management' patch, in create_var_ref(), moved the saving of ref_field and update of ref_field->var_ref_idx into the 'if' as pointed out by Dan Carpenter/smatch 0-day robot.

It doesn't introduce any functional changes and can be applied independently of the other patchset.

Tom

The following changes since commit 5d6ddf6acce68d1290112cb08b12fd78b201e7d5:

arm64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack (2018-12-08 22:21:31 -0500)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/hist-var-ref-cleanup-v1

Tom Zanussi (7):
tracing: Remove unnecessary hist trigger struct field
tracing: Change strlen to sizeof for hist trigger static strings
tracing: Use var_refs[] for hist trigger reference checking
tracing: Remove open-coding of hist trigger var_ref management
tracing: Use hist trigger's var_ref array to destroy var_refs
tracing: Remove hist trigger synth_var_refs
tracing: Add hist trigger comments for variable-related fields

 kernel/trace/trace_events_hist.c | 267 +++++++++++++++++++++++----------------
 1 file changed, 156 insertions(+), 111 deletions(-)

Signature · 1 line

2.14.1

Hi Tom,

Quoted 25 lines · On Tue, 18 Dec 2018 14:33:19 -0600 Tom Zanussi <[email protected]> wrote:

From: Tom Zanussi <[email protected]>

Hi,

This patchset is a standalone series broken out of the v8 version of
the 'tracing: Hist trigger snapshot and onchange additions' patchset.

It's a series of changes resulting from some suggestions from Namhyung
for making the variable-reference handling code more understandable through some refactoring and comments.

It also added a new patch changing all strlen() to sizeof() for string
constants, in trace_events_hist.c

Also, in the 'tracing: Remove open-coding of hist trigger var_ref management' patch, in create_var_ref(), moved the saving of ref_field
and update of ref_field->var_ref_idx into the 'if' as pointed out by
Dan Carpenter/smatch 0-day robot.

Quoted 2 lines

It doesn't introduce any functional changes and can be applied independently of the other patchset.

OK, now it is very clear to me :-)

Reviewed-by Masami Hiramatsu <[email protected]>

for the series.