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(-)
--
2.14.1
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(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index d29bf8a8e1dd..25d06b3ae1f6 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -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;
@@ -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);
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;
}
--
2.14.1
From: Tom Zanussi <[email protected]>
hist_field.var_idx is completely unused, so remove it.
Signed-off-by: Tom Zanussi <[email protected]>
---
kernel/trace/trace_events_hist.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 82e72c48a5a9..d29bf8a8e1dd 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -61,7 +61,6 @@ struct hist_field {
char *system;
char *event_name;
char *name;
- unsigned int var_idx;
unsigned int var_ref_idx;
bool read_once;
};
--
2.14.1
From: Tom Zanussi <[email protected]>
Since every var ref for a trigger has an entry in the var_ref[] array,
use that to destroy the var_refs, instead of piecemeal via the field
expressions.
This allows us to avoid having to keep and treat differently separate
lists for the action-related references, which future patches will
remove.
Signed-off-by: Tom Zanussi <[email protected]>
---
kernel/trace/trace_events_hist.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 9d4cf7a8a95b..725d22f5372f 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -2193,6 +2193,15 @@ static int contains_operator(char *str)
return field_op;
}
+static void __destroy_hist_field(struct hist_field *hist_field)
+{
+ kfree(hist_field->var.name);
+ kfree(hist_field->name);
+ kfree(hist_field->type);
+
+ kfree(hist_field);
+}
+
static void destroy_hist_field(struct hist_field *hist_field,
unsigned int level)
{
@@ -2204,14 +2213,13 @@ static void destroy_hist_field(struct hist_field *hist_field,
if (!hist_field)
return;
+ if (hist_field->flags & HIST_FIELD_FL_VAR_REF)
+ return; /* var refs will be destroyed separately */
+
for (i = 0; i < HIST_FIELD_OPERANDS_MAX; i++)
destroy_hist_field(hist_field->operands[i], level + 1);
- kfree(hist_field->var.name);
- kfree(hist_field->name);
- kfree(hist_field->type);
-
- kfree(hist_field);
+ __destroy_hist_field(hist_field);
}
static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data,
@@ -2338,6 +2346,12 @@ static void destroy_hist_fields(struct hist_trigger_data *hist_data)
hist_data->fields[i] = NULL;
}
}
+
+ for (i = 0; i < hist_data->n_var_refs; i++) {
+ WARN_ON(!(hist_data->var_refs[i]->flags & HIST_FIELD_FL_VAR_REF));
+ __destroy_hist_field(hist_data->var_refs[i]);
+ hist_data->var_refs[i] = NULL;
+ }
}
static int init_var_ref(struct hist_field *ref_field,
--
2.14.1
From: Tom Zanussi <[email protected]>
Have create_var_ref() manage the hist trigger's var_ref list, rather
than having similar code doing it in multiple places. This cleans up
the code and makes sure var_refs are always accounted properly.
Also, document the var_ref-related functions to make what their
purpose clearer.
Signed-off-by: Tom Zanussi <[email protected]>
---
kernel/trace/trace_events_hist.c | 93 ++++++++++++++++++++++++++++++++--------
1 file changed, 75 insertions(+), 18 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index ee839c52bd3f..9d4cf7a8a95b 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1292,6 +1292,17 @@ static u64 hist_field_cpu(struct hist_field *hist_field,
return cpu;
}
+/**
+ * check_field_for_var_ref - Check if a VAR_REF field references a variable
+ * @hist_field: The VAR_REF field to check
+ * @var_data: The hist trigger that owns the variable
+ * @var_idx: The trigger variable identifier
+ *
+ * Check the given VAR_REF field to see whether or not it references
+ * the given variable associated with the given trigger.
+ *
+ * Return: The VAR_REF field if it does reference the variable, NULL if not
+ */
static struct hist_field *
check_field_for_var_ref(struct hist_field *hist_field,
struct hist_trigger_data *var_data,
@@ -1308,6 +1319,18 @@ check_field_for_var_ref(struct hist_field *hist_field,
return found;
}
+/**
+ * find_var_ref - Check if a trigger has a reference to a trigger variable
+ * @hist_data: The hist trigger that might have a reference to the variable
+ * @var_data: The hist trigger that owns the variable
+ * @var_idx: The trigger variable identifier
+ *
+ * Check the list of var_refs[] on the first hist trigger to see
+ * whether any of them are references to the variable on the second
+ * trigger.
+ *
+ * Return: The VAR_REF field referencing the variable if so, NULL if not
+ */
static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,
struct hist_trigger_data *var_data,
unsigned int var_idx)
@@ -1325,6 +1348,20 @@ static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,
return found;
}
+/**
+ * find_any_var_ref - Check if there is a reference to a given trigger variable
+ * @hist_data: The hist trigger
+ * @var_idx: The trigger variable identifier
+ *
+ * Check to see whether the given variable is currently referenced by
+ * any other trigger.
+ *
+ * The trigger the variable is defined on is explicitly excluded - the
+ * assumption being that a self-reference doesn't prevent a trigger
+ * from being removed.
+ *
+ * Return: The VAR_REF field referencing the variable if so, NULL if not
+ */
static struct hist_field *find_any_var_ref(struct hist_trigger_data *hist_data,
unsigned int var_idx)
{
@@ -1343,6 +1380,19 @@ static struct hist_field *find_any_var_ref(struct hist_trigger_data *hist_data,
return found;
}
+/**
+ * check_var_refs - Check if there is a reference to any of trigger's variables
+ * @hist_data: The hist trigger
+ *
+ * A trigger can define one or more variables. If any one of them is
+ * currently referenced by any other trigger, this function will
+ * determine that.
+
+ * Typically used to determine whether or not a trigger can be removed
+ * - if there are any references to a trigger's variables, it cannot.
+ *
+ * Return: True if there is a reference to any of trigger's variables
+ */
static bool check_var_refs(struct hist_trigger_data *hist_data)
{
struct hist_field *field;
@@ -2346,7 +2396,23 @@ static int init_var_ref(struct hist_field *ref_field,
goto out;
}
-static struct hist_field *create_var_ref(struct hist_field *var_field,
+/**
+ * create_var_ref - Create a variable reference and attach it to trigger
+ * @hist_data: The trigger that will be referencing the variable
+ * @var_field: The VAR field to create a reference to
+ * @system: The optional system string
+ * @event_name: The optional event_name string
+ *
+ * Given a variable hist_field, create a VAR_REF hist_field that
+ * represents a reference to it.
+ *
+ * This function also adds the reference to the trigger that
+ * now references the variable.
+ *
+ * Return: The VAR_REF field if successful, NULL if not
+ */
+static struct hist_field *create_var_ref(struct hist_trigger_data *hist_data,
+ struct hist_field *var_field,
char *system, char *event_name)
{
unsigned long flags = HIST_FIELD_FL_VAR_REF;
@@ -2358,6 +2424,9 @@ static struct hist_field *create_var_ref(struct hist_field *var_field,
destroy_hist_field(ref_field, 0);
return NULL;
}
+
+ hist_data->var_refs[hist_data->n_var_refs] = ref_field;
+ ref_field->var_ref_idx = hist_data->n_var_refs++;
}
return ref_field;
@@ -2431,7 +2500,8 @@ static struct hist_field *parse_var_ref(struct hist_trigger_data *hist_data,
var_field = find_event_var(hist_data, system, event_name, var_name);
if (var_field)
- ref_field = create_var_ref(var_field, system, event_name);
+ ref_field = create_var_ref(hist_data, var_field,
+ system, event_name);
if (!ref_field)
hist_err_event("Couldn't find variable: $",
@@ -2549,8 +2619,6 @@ static struct hist_field *parse_atom(struct hist_trigger_data *hist_data,
if (!s) {
hist_field = parse_var_ref(hist_data, ref_system, ref_event, ref_var);
if (hist_field) {
- hist_data->var_refs[hist_data->n_var_refs] = hist_field;
- hist_field->var_ref_idx = hist_data->n_var_refs++;
if (var_name) {
hist_field = create_alias(hist_data, hist_field, var_name);
if (!hist_field) {
@@ -3324,7 +3392,6 @@ static int onmax_create(struct hist_trigger_data *hist_data,
unsigned int var_ref_idx = hist_data->n_var_refs;
struct field_var *field_var;
char *onmax_var_str, *param;
- unsigned long flags;
unsigned int i;
int ret = 0;
@@ -3341,18 +3408,10 @@ static int onmax_create(struct hist_trigger_data *hist_data,
return -EINVAL;
}
- flags = HIST_FIELD_FL_VAR_REF;
- ref_field = create_hist_field(hist_data, NULL, flags, NULL);
+ ref_field = create_var_ref(hist_data, var_field, NULL, NULL);
if (!ref_field)
return -ENOMEM;
- if (init_var_ref(ref_field, var_field, NULL, NULL)) {
- destroy_hist_field(ref_field, 0);
- ret = -ENOMEM;
- goto out;
- }
- hist_data->var_refs[hist_data->n_var_refs] = ref_field;
- ref_field->var_ref_idx = hist_data->n_var_refs++;
data->onmax.var = ref_field;
data->fn = onmax_save;
@@ -3541,9 +3600,6 @@ static void save_synth_var_ref(struct hist_trigger_data *hist_data,
struct hist_field *var_ref)
{
hist_data->synth_var_refs[hist_data->n_synth_var_refs++] = var_ref;
-
- hist_data->var_refs[hist_data->n_var_refs] = var_ref;
- var_ref->var_ref_idx = hist_data->n_var_refs++;
}
static int check_synth_field(struct synth_event *event,
@@ -3697,7 +3753,8 @@ static int onmatch_create(struct hist_trigger_data *hist_data,
}
if (check_synth_field(event, hist_field, field_pos) == 0) {
- var_ref = create_var_ref(hist_field, system, event_name);
+ var_ref = create_var_ref(hist_data, hist_field,
+ system, event_name);
if (!var_ref) {
kfree(p);
ret = -ENOMEM;
--
2.14.1
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(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 25d06b3ae1f6..ee839c52bd3f 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -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;
return found;
}
@@ -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;
- }
-
- for (i = 0; i < hist_data->n_synth_var_refs; i++) {
- hist_field = hist_data->synth_var_refs[i];
- found = check_field_for_var_refs(hist_data, hist_field,
- var_data, var_idx, 0);
+ for (i = 0; i < hist_data->n_var_refs; i++) {
+ hist_field = hist_data->var_refs[i];
+ found = check_field_for_var_ref(hist_field, var_data, var_idx);
if (found)
return found;
}
--
2.14.1
From: Tom Zanussi <[email protected]>
Add a few comments to help clarify how variable and variable reference
fields are used in the code.
Signed-off-by: Tom Zanussi <[email protected]>
---
kernel/trace/trace_events_hist.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index e66dd764e057..b813da145dbd 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -40,6 +40,16 @@ enum field_op_id {
FIELD_OP_UNARY_MINUS,
};
+/*
+ * A hist_var (histogram variable) contains variable information for
+ * hist_fields having the HIST_FIELD_FL_VAR or HIST_FIELD_FL_VAR_REF
+ * flag set. A hist_var has a variable name e.g. ts0, and is
+ * associated with a given histogram trigger, as specified by
+ * hist_data. The hist_var idx is the unique index assigned to the
+ * variable by the hist trigger's tracing_map. The idx is what is
+ * used to set a variable's value and, by a variable reference, to
+ * retrieve it.
+ */
struct hist_var {
char *name;
struct hist_trigger_data *hist_data;
@@ -56,11 +66,29 @@ struct hist_field {
const char *type;
struct hist_field *operands[HIST_FIELD_OPERANDS_MAX];
struct hist_trigger_data *hist_data;
+
+ /*
+ * Variable fields contain variable-specific info in var.
+ */
struct hist_var var;
enum field_op_id operator;
char *system;
char *event_name;
+
+ /*
+ * The name field is used for EXPR and VAR_REF fields. VAR
+ * fields contain the variable name in var.name.
+ */
char *name;
+
+ /*
+ * When a histogram trigger is hit, if it has any references
+ * to variables, the values of those variables are collected
+ * into a var_ref_vals array by resolve_var_refs(). The
+ * current value of each variable is read from the tracing_map
+ * using the hist field's hist_var.idx and entered into the
+ * var_ref_idx entry i.e. var_ref_vals[var_ref_idx].
+ */
unsigned int var_ref_idx;
bool read_once;
};
@@ -365,6 +393,14 @@ struct action_data {
union {
struct {
+ /*
+ * When a histogram trigger is hit, the values of any
+ * references to variables, including variables being passed
+ * as parameters to synthetic events, are collected into a
+ * var_ref_vals array. This var_ref_idx is the index of the
+ * first param in the array to be passed to the synthetic
+ * event invocation.
+ */
unsigned int var_ref_idx;
char *match_event;
char *match_event_system;
--
2.14.1
From: Tom Zanussi <[email protected]>
All var_refs are now handled uniformly and there's no reason to treat
the synth_refs in a special way now, so remove them and associated
functions.
Signed-off-by: Tom Zanussi <[email protected]>
---
kernel/trace/trace_events_hist.c | 18 ------------------
1 file changed, 18 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 725d22f5372f..e66dd764e057 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -279,8 +279,6 @@ struct hist_trigger_data {
struct action_data *actions[HIST_ACTIONS_MAX];
unsigned int n_actions;
- struct hist_field *synth_var_refs[SYNTH_FIELDS_MAX];
- unsigned int n_synth_var_refs;
struct field_var *field_vars[SYNTH_FIELDS_MAX];
unsigned int n_field_vars;
unsigned int n_field_var_str;
@@ -3602,20 +3600,6 @@ static void save_field_var(struct hist_trigger_data *hist_data,
}
-static void destroy_synth_var_refs(struct hist_trigger_data *hist_data)
-{
- unsigned int i;
-
- for (i = 0; i < hist_data->n_synth_var_refs; i++)
- destroy_hist_field(hist_data->synth_var_refs[i], 0);
-}
-
-static void save_synth_var_ref(struct hist_trigger_data *hist_data,
- struct hist_field *var_ref)
-{
- hist_data->synth_var_refs[hist_data->n_synth_var_refs++] = var_ref;
-}
-
static int check_synth_field(struct synth_event *event,
struct hist_field *hist_field,
unsigned int field_pos)
@@ -3775,7 +3759,6 @@ static int onmatch_create(struct hist_trigger_data *hist_data,
goto err;
}
- save_synth_var_ref(hist_data, var_ref);
field_pos++;
kfree(p);
continue;
@@ -4519,7 +4502,6 @@ static void destroy_hist_data(struct hist_trigger_data *hist_data)
destroy_actions(hist_data);
destroy_field_vars(hist_data);
destroy_field_var_hists(hist_data);
- destroy_synth_var_refs(hist_data);
kfree(hist_data);
}
--
2.14.1
Hi Tom,
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.
>
> Signed-off-by: Tom Zanussi <[email protected]>
> ---
> kernel/trace/trace_events_hist.c | 57 +++++-----------------------------------
> 1 file changed, 7 insertions(+), 50 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 25d06b3ae1f6..ee839c52bd3f 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -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;
>
> return found;
It seems we don't need "found" var here. Just return hist_field or NULL.
> }
> @@ -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;
> - }
> -
> - for (i = 0; i < hist_data->n_synth_var_refs; i++) {
> - hist_field = hist_data->synth_var_refs[i];
> - found = check_field_for_var_refs(hist_data, hist_field,
> - var_data, var_idx, 0);
> + for (i = 0; i < hist_data->n_var_refs; i++) {
> + hist_field = hist_data->var_refs[i];
> + found = check_field_for_var_ref(hist_field, var_data, var_idx);
> if (found)
> return found;
> }
Here too. If you return soon after found it, you don't need to assign it.
Except for that, it looks good to me.
Reviewed-by: Masami Hiramatsu <[email protected]>
Thank you,
> --
> 2.14.1
>
--
Masami Hiramatsu <[email protected]>
Hi Tom,
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.
>
> 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.
Thank you,
>
> 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(-)
>
> --
> 2.14.1
>
--
Masami Hiramatsu <[email protected]>
Hi Tom,
On Tue, Dec 18, 2018 at 02:33:19PM -0600, Tom Zanussi 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.
>
> It doesn't introduce any functional changes and can be applied
> independently of the other patchset.
Acked-by: Namhyung Kim <[email protected]>
Thanks,
Namhyung
>
> 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(-)
>
> --
> 2.14.1
>
On Wed, 2018-12-19 at 21:27 +0900, Masami Hiramatsu wrote:
> Hi Tom,
>
> 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.
> >
> > 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.
>
Thanks, Masami!
Tom
Hi Namhyung,
On Wed, 2018-12-19 at 20:45 +0900, Namhyung Kim wrote:
> Hi Tom,
>
> On Tue, Dec 18, 2018 at 02:33:19PM -0600, Tom Zanussi 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.
> >
> > It doesn't introduce any functional changes and can be applied
> > independently of the other patchset.
>
> Acked-by: Namhyung Kim <[email protected]>
>
Thanks!
Tom
Hi Masami,
On Wed, 2018-12-19 at 21:22 +0900, Masami Hiramatsu wrote:
> Hi Tom,
>
> 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.
>
> >
> > Signed-off-by: Tom Zanussi <[email protected]>
> > ---
> > kernel/trace/trace_events_hist.c | 57 +++++-----------------------
> > ------------
> > 1 file changed, 7 insertions(+), 50 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events_hist.c
> > b/kernel/trace/trace_events_hist.c
> > index 25d06b3ae1f6..ee839c52bd3f 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -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;
> >
> > return found;
>
> It seems we don't need "found" var here. Just return hist_field or
> NULL.
>
OK, will change these and resubmit shortly.
Thanks,
Tom
On Wed, 19 Dec 2018 09:01:43 -0600
Tom Zanussi <[email protected]> wrote:
> > > + 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.
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
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 | 68 +++++++---------------------------------
1 file changed, 11 insertions(+), 57 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 25d06b3ae1f6..f3caf6e484f4 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1297,75 +1297,29 @@ check_field_for_var_ref(struct hist_field *hist_field,
struct hist_trigger_data *var_data,
unsigned int var_idx)
{
- 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;
-
- 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;
+ WARN_ON(!(hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF));
- 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)
+ return hist_field;
- return found;
+ return NULL;
}
static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,
struct hist_trigger_data *var_data,
unsigned int var_idx)
{
- struct hist_field *hist_field, *found = NULL;
+ struct hist_field *hist_field;
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;
- }
-
- for (i = 0; i < hist_data->n_synth_var_refs; i++) {
- hist_field = hist_data->synth_var_refs[i];
- found = check_field_for_var_refs(hist_data, hist_field,
- var_data, var_idx, 0);
- if (found)
- return found;
+ for (i = 0; i < hist_data->n_var_refs; i++) {
+ hist_field = hist_data->var_refs[i];
+ if (check_field_for_var_ref(hist_field, var_data, var_idx))
+ return hist_field;
}
- return found;
+ return NULL;
}
static struct hist_field *find_any_var_ref(struct hist_trigger_data *hist_data,
--
2.14.1
Hi Steve,
On Wed, 2018-12-19 at 10:36 -0500, Steven Rostedt wrote:
> On Wed, 19 Dec 2018 09:01:43 -0600
> Tom Zanussi <[email protected]> wrote:
>
> > > > + 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.
>
> 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.
>
Sorry for the delay - had an emergency appointment for the dog at the
vet this morning..
Anyway, yeah, I'll reply with the patch - it's the only one that needs
updating in this series.
Thanks,
Tom
> -- Steve
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(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index d29bf8a8e1dd..25d06b3ae1f6 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -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;
> @@ -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.
-- Steve
> 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;
> }
Hi Steve,
On Wed, 2018-12-19 at 14:40 -0500, Steven Rostedt wrote:
> 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(-)
> >
> > diff --git a/kernel/trace/trace_events_hist.c
> > b/kernel/trace/trace_events_hist.c
> > index d29bf8a8e1dd..25d06b3ae1f6 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -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;
> > @@ -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.
>
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
On Wed, 2018-12-19 at 13:46 -0600, Tom Zanussi wrote:
> Hi Steve,
>
> On Wed, 2018-12-19 at 14:40 -0500, Steven Rostedt wrote:
> > 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(-)
> > >
> > > diff --git a/kernel/trace/trace_events_hist.c
> > > b/kernel/trace/trace_events_hist.c
> > > index d29bf8a8e1dd..25d06b3ae1f6 100644
> > > --- a/kernel/trace/trace_events_hist.c
> > > +++ b/kernel/trace/trace_events_hist.c
> > > @@ -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;
> > > @@ -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.
> >
>
> 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]>
---
kernel/trace/trace_events_hist.c | 31 +++++++++++++++++--------------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index c5448c103770..aaf0d2ac4aab 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -22,6 +22,9 @@
#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)
+
struct hist_field;
typedef u64 (*hist_field_fn_t) (struct hist_field *field,
@@ -1881,8 +1884,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(", sizeof("onmatch(") - 1) == 0) ||
- (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
+ if ((strcmp_const(str, "onmatch(") == 0) ||
+ (strcmp_const(str, "onmax(") == 0)) {
attrs->action_str[attrs->n_actions] = kstrdup(str, GFP_KERNEL);
if (!attrs->action_str[attrs->n_actions]) {
ret = -ENOMEM;
@@ -1899,34 +1902,34 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
{
int ret = 0;
- if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
- (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
+ if ((strcmp_const(str, "key=") == 0) ||
+ (strcmp_const(str, "keys=") == 0)) {
attrs->keys_str = kstrdup(str, GFP_KERNEL);
if (!attrs->keys_str) {
ret = -ENOMEM;
goto out;
}
- } else if ((strncmp(str, "val=", sizeof("val=") - 1) == 0) ||
- (strncmp(str, "vals=", sizeof("vals=") - 1) == 0) ||
- (strncmp(str, "values=", sizeof("values=") - 1) == 0)) {
+ } else if ((strcmp_const(str, "val=") == 0) ||
+ (strcmp_const(str, "vals=") == 0) ||
+ (strcmp_const(str, "values=") == 0)) {
attrs->vals_str = kstrdup(str, GFP_KERNEL);
if (!attrs->vals_str) {
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "sort=", sizeof("sort=") - 1) == 0) {
+ } else if (strcmp_const(str, "sort=") == 0) {
attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
if (!attrs->sort_key_str) {
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "name=", sizeof("name=") - 1) == 0) {
+ } else if (strcmp_const(str, "name=") == 0) {
attrs->name = kstrdup(str, GFP_KERNEL);
if (!attrs->name) {
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "clock=", sizeof("clock=") - 1) == 0) {
+ } else if (strcmp_const(str, "clock=") == 0) {
strsep(&str, "=");
if (!str) {
ret = -EINVAL;
@@ -1939,7 +1942,7 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "size=", sizeof("size=") - 1) == 0) {
+ } else if (strcmp_const(str, "size=") == 0) {
int map_bits = parse_map_size(str);
if (map_bits < 0) {
@@ -3558,7 +3561,7 @@ static struct action_data *onmax_parse(char *str)
if (!onmax_fn_name || !str)
goto free;
- if (strncmp(onmax_fn_name, "save", sizeof("save") - 1) == 0) {
+ if (strcmp_const(onmax_fn_name, "save") == 0) {
char *params = strsep(&str, ")");
if (!params) {
@@ -4346,7 +4349,7 @@ 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(", sizeof("onmatch(") - 1) == 0) {
+ if (strcmp_const(str, "onmatch(") == 0) {
char *action_str = str + sizeof("onmatch(") - 1;
data = onmatch_parse(tr, action_str);
@@ -4355,7 +4358,7 @@ static int parse_actions(struct hist_trigger_data *hist_data)
break;
}
data->fn = action_trace;
- } else if (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0) {
+ } else if (strcmp_const(str, "onmax(") == 0) {
char *action_str = str + sizeof("onmax(") - 1;
data = onmax_parse(action_str);
--
2.14.1
> Tom
On Wed, 2018-12-19 at 13:46 -0600, Tom Zanussi wrote:
> Hi Steve,
>
> On Wed, 2018-12-19 at 14:40 -0500, Steven Rostedt wrote:
> > 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(-)
> > >
> > > diff --git a/kernel/trace/trace_events_hist.c
> > > b/kernel/trace/trace_events_hist.c
> > > index d29bf8a8e1dd..25d06b3ae1f6 100644
> > > --- a/kernel/trace/trace_events_hist.c
> > > +++ b/kernel/trace/trace_events_hist.c
> > > @@ -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;
> > > @@ -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.
> >
>
> 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.
On Wed, 19 Dec 2018 13:46:49 -0600
Tom Zanussi <[email protected]> wrote:
> 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. ;-)
>
Here, want to ack/review it?
-- Steve
From 664457f5cc776aa624502bc628285ea6d70ac8c9 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (VMware)" <[email protected]>
Date: Wed, 19 Dec 2018 15:13:33 -0500
Subject: [PATCH] tracing: Add strncmp_const() macro to prevent typos and cut
paste errors
The trace_events_hist.c file has a lot of strncmp()s of the form:
if (strncmp(str, "constant", sizeof("constant") - 1) == 0)
To prevent typos and errors with the constant string being different from
the string use in sizeof(), and also to condense the code, add a macro that
does this:
#define strncmp_const(str, str_const) \
strncmp(str, str_const, sizeof(str_const) - 1)
Signed-off-by: Steven Rostedt (VMware) <[email protected]>
---
kernel/trace/trace_events_hist.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index c5448c103770..40e76053d3d7 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -22,6 +22,10 @@
#define STR_VAR_LEN_MAX 32 /* must be multiple of sizeof(u64) */
+/* Safe way to compare string constants (from typos and cut and paste errors) */
+#define strncmp_const(str, str_const) \
+ strncmp(str, str_const, sizeof(str_const) - 1)
+
struct hist_field;
typedef u64 (*hist_field_fn_t) (struct hist_field *field,
@@ -1881,8 +1885,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(", sizeof("onmatch(") - 1) == 0) ||
- (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
+ if ((strncmp_const(str, "onmatch(") == 0) ||
+ (strncmp_const(str, "onmax(") == 0)) {
attrs->action_str[attrs->n_actions] = kstrdup(str, GFP_KERNEL);
if (!attrs->action_str[attrs->n_actions]) {
ret = -ENOMEM;
@@ -1899,34 +1903,34 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
{
int ret = 0;
- if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
- (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
+ if ((strncmp_const(str, "key=") == 0) ||
+ (strncmp_const(str, "keys=") == 0)) {
attrs->keys_str = kstrdup(str, GFP_KERNEL);
if (!attrs->keys_str) {
ret = -ENOMEM;
goto out;
}
- } else if ((strncmp(str, "val=", sizeof("val=") - 1) == 0) ||
- (strncmp(str, "vals=", sizeof("vals=") - 1) == 0) ||
- (strncmp(str, "values=", sizeof("values=") - 1) == 0)) {
+ } else if ((strncmp_const(str, "val=") == 0) ||
+ (strncmp_const(str, "vals=") == 0) ||
+ (strncmp_const(str, "values=") == 0)) {
attrs->vals_str = kstrdup(str, GFP_KERNEL);
if (!attrs->vals_str) {
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "sort=", sizeof("sort=") - 1) == 0) {
+ } else if (strncmp_const(str, "sort=") == 0) {
attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
if (!attrs->sort_key_str) {
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "name=", sizeof("name=") - 1) == 0) {
+ } else if (strncmp_const(str, "name=") == 0) {
attrs->name = kstrdup(str, GFP_KERNEL);
if (!attrs->name) {
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "clock=", sizeof("clock=") - 1) == 0) {
+ } else if (strncmp_const(str, "clock=") == 0) {
strsep(&str, "=");
if (!str) {
ret = -EINVAL;
@@ -1939,7 +1943,7 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "size=", sizeof("size=") - 1) == 0) {
+ } else if (strncmp_const(str, "size=") == 0) {
int map_bits = parse_map_size(str);
if (map_bits < 0) {
@@ -3558,7 +3562,7 @@ static struct action_data *onmax_parse(char *str)
if (!onmax_fn_name || !str)
goto free;
- if (strncmp(onmax_fn_name, "save", sizeof("save") - 1) == 0) {
+ if (strncmp_const(onmax_fn_name, "save") == 0) {
char *params = strsep(&str, ")");
if (!params) {
@@ -4346,7 +4350,7 @@ 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(", sizeof("onmatch(") - 1) == 0) {
+ if (strncmp_const(str, "onmatch(") == 0) {
char *action_str = str + sizeof("onmatch(") - 1;
data = onmatch_parse(tr, action_str);
@@ -4355,7 +4359,7 @@ static int parse_actions(struct hist_trigger_data *hist_data)
break;
}
data->fn = action_trace;
- } else if (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0) {
+ } else if (strncmp_const(str, "onmax(") == 0) {
char *action_str = str + sizeof("onmax(") - 1;
data = onmax_parse(action_str);
--
2.19.1
On Wed, 19 Dec 2018 12:20:19 -0800
Joe Perches <[email protected]> wrote:
> > 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.
-- Steve
>
> the strcmp_const macro does seem to make sense as
> the copy/paste/typo possibility is real.
On Wed, 19 Dec 2018 12:22:38 -0800
Joe Perches <[email protected]> wrote:
> On Wed, 2018-12-19 at 14:16 -0600, Tom Zanussi wrote:
> >
> > 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).
> []
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> []
> > @@ -22,6 +22,9 @@
> >
> > #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.
-- Steve
On Wed, 2018-12-19 at 15:30 -0500, Steven Rostedt wrote:
> On Wed, 19 Dec 2018 12:20:19 -0800
> Joe Perches <[email protected]> wrote:
>
> > > 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.
>
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').
Tom
> -- Steve
>
>
> >
> > the strcmp_const macro does seem to make sense as
> > the copy/paste/typo possibility is real.
>
>
On Wed, 2018-12-19 at 15:34 -0500, Steven Rostedt wrote:
> On Wed, 19 Dec 2018 12:22:38 -0800
> Joe Perches <[email protected]> wrote:
>
> > On Wed, 2018-12-19 at 14:16 -0600, Tom Zanussi wrote:
> > > 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).
> > []
> > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > []
> > > @@ -22,6 +22,9 @@
> > >
> > > #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.
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
On Wed, 19 Dec 2018 14:38:39 -0600
Tom Zanussi <[email protected]> wrote:
> On Wed, 2018-12-19 at 15:30 -0500, Steven Rostedt wrote:
> > On Wed, 19 Dec 2018 12:20:19 -0800
> > Joe Perches <[email protected]> wrote:
> >
> > > > 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.
> >
>
> 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.
-- Steve
On Wed, 19 Dec 2018 12:51:59 -0800
Joe Perches <[email protected]> wrote:
> > #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
Yeah sure.
>
> But there seem to be _a lot_ of those...
>
> $ git grep -P "\bstrncmp\s*\([^,]+,[^,]+,\s*\d+\s*\)" | wc -l
> 1681
>
Right.
I'm going to first create the macro and probably just apply it to the
tracing directory. Then when the macro is added to the next merge
window, I'll post a bunch of patches that clean up the rest of the code.
-- Steve
On Wed, 2018-12-19 at 16:01 -0500, Steven Rostedt wrote:
> On Wed, 19 Dec 2018 14:38:39 -0600
> Tom Zanussi <[email protected]> wrote:
>
> > On Wed, 2018-12-19 at 15:30 -0500, Steven Rostedt wrote:
> > > On Wed, 19 Dec 2018 12:20:19 -0800
> > > Joe Perches <[email protected]> wrote:
> > >
> > > > > 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.
> > >
> >
> > 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.
The patch included:
- start += strlen("char[");
+ start += sizeof("char[") - 1;
So, that 2/7 patch is unnecessary as there is no object code
change and using 'sizeof("const") - 1' is less intelligible
than strlen("const")
If you convert the strncmp(ptr, "const", strlen("const"))
uses to strncmp_prefix(ptr, "const") eventually, the patch
just makes more variations to change.
On Wed, 19 Dec 2018 14:16:31 -0600
Tom Zanussi <[email protected]> wrote:
> > 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]>
> ---
And I just sent you my version :-) (cross emails).
Note, I changed it to strncmp_const() because it should note that it's
a strncmp() and not an strcmp().
Pretty much identical patches too ;-)
-- Steve
On Wed, 2018-12-19 at 14:16 -0600, Tom Zanussi wrote:
>
> 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).
[]
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
[]
> @@ -22,6 +22,9 @@
>
> #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.