* [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
@ 2020-01-08 14:20 ` Leo Yan
0 siblings, 0 replies; 18+ messages in thread
From: Leo Yan @ 2020-01-08 14:20 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa, Mathieu Poirier,
Suzuki K Poulose, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Namhyung Kim, Ian Rogers, Adrian Hunter,
Thomas Gleixner, Andi Kleen, linux-arm-kernel, linux-kernel,
Mike Leach
Cc: Leo Yan
The struct perf_evsel_config_term::val is a union which contains
multiple variables for corresponding types. This leads the union to
be complex and also causes complex code logic.
This patch refactors the structure to use two general variables in the
'val' union: one is 'num' for unsigned 64-bit integer and another is
'str' for string variable. This can simplify the data structure and
the related code, this also can benefit for possibly extension.
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
tools/perf/arch/arm/util/cs-etm.c | 2 +-
tools/perf/builtin-top.c | 2 +-
tools/perf/util/auxtrace.c | 2 +-
tools/perf/util/evsel.c | 24 +++++++-------
tools/perf/util/evsel_config.h | 17 ++--------
tools/perf/util/parse-events.c | 55 ++++++++++++++++++-------------
6 files changed, 49 insertions(+), 53 deletions(-)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index ede040cf82ad..2898cfdf8fe1 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -226,7 +226,7 @@ static int cs_etm_set_sink_attr(struct perf_pmu *pmu,
if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
continue;
- sink = term->val.drv_cfg;
+ sink = term->val.str;
snprintf(path, PATH_MAX, "sinks/%s", sink);
ret = perf_pmu__scan_file(pmu, path, "%x", &hash);
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 795e353de095..459be44ca2ff 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -947,7 +947,7 @@ static int perf_top__overwrite_check(struct perf_top *top)
config_terms = &evsel->config_terms;
list_for_each_entry(term, config_terms, list) {
if (term->type == PERF_EVSEL__CONFIG_TERM_OVERWRITE)
- set = term->val.overwrite ? 1 : 0;
+ set = term->val.num ? 1 : 0;
}
/* no term for current and previous event (likely) */
diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index eb087e7df6f4..a5c945aadfa7 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -772,7 +772,7 @@ int auxtrace_parse_sample_options(struct auxtrace_record *itr,
term = perf_evsel__get_config_term(evsel, AUX_SAMPLE_SIZE);
if (term) {
has_aux_sample_size = true;
- evsel->core.attr.aux_sample_size = term->val.aux_sample_size;
+ evsel->core.attr.aux_sample_size = term->val.num;
/* If possible, group with the AUX event */
if (aux_evsel && evsel->core.attr.aux_sample_size)
perf_evlist__regroup(evlist, aux_evsel, evsel);
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index a69e64236120..5f27f6b7ed94 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -789,43 +789,43 @@ static void apply_config_terms(struct evsel *evsel,
switch (term->type) {
case PERF_EVSEL__CONFIG_TERM_PERIOD:
if (!(term->weak && opts->user_interval != ULLONG_MAX)) {
- attr->sample_period = term->val.period;
+ attr->sample_period = term->val.num;
attr->freq = 0;
perf_evsel__reset_sample_bit(evsel, PERIOD);
}
break;
case PERF_EVSEL__CONFIG_TERM_FREQ:
if (!(term->weak && opts->user_freq != UINT_MAX)) {
- attr->sample_freq = term->val.freq;
+ attr->sample_freq = term->val.num;
attr->freq = 1;
perf_evsel__set_sample_bit(evsel, PERIOD);
}
break;
case PERF_EVSEL__CONFIG_TERM_TIME:
- if (term->val.time)
+ if (term->val.num)
perf_evsel__set_sample_bit(evsel, TIME);
else
perf_evsel__reset_sample_bit(evsel, TIME);
break;
case PERF_EVSEL__CONFIG_TERM_CALLGRAPH:
- callgraph_buf = term->val.callgraph;
+ callgraph_buf = term->val.str;
break;
case PERF_EVSEL__CONFIG_TERM_BRANCH:
- if (term->val.branch && strcmp(term->val.branch, "no")) {
+ if (term->val.str && strcmp(term->val.str, "no")) {
perf_evsel__set_sample_bit(evsel, BRANCH_STACK);
- parse_branch_str(term->val.branch,
+ parse_branch_str(term->val.str,
&attr->branch_sample_type);
} else
perf_evsel__reset_sample_bit(evsel, BRANCH_STACK);
break;
case PERF_EVSEL__CONFIG_TERM_STACK_USER:
- dump_size = term->val.stack_user;
+ dump_size = term->val.num;
break;
case PERF_EVSEL__CONFIG_TERM_MAX_STACK:
- max_stack = term->val.max_stack;
+ max_stack = term->val.num;
break;
case PERF_EVSEL__CONFIG_TERM_MAX_EVENTS:
- evsel->max_events = term->val.max_events;
+ evsel->max_events = term->val.num;
break;
case PERF_EVSEL__CONFIG_TERM_INHERIT:
/*
@@ -834,17 +834,17 @@ static void apply_config_terms(struct evsel *evsel,
* inherit using config terms, override global
* opt->no_inherit setting.
*/
- attr->inherit = term->val.inherit ? 1 : 0;
+ attr->inherit = term->val.num ? 1 : 0;
break;
case PERF_EVSEL__CONFIG_TERM_OVERWRITE:
- attr->write_backward = term->val.overwrite ? 1 : 0;
+ attr->write_backward = term->val.num ? 1 : 0;
break;
case PERF_EVSEL__CONFIG_TERM_DRV_CFG:
break;
case PERF_EVSEL__CONFIG_TERM_PERCORE:
break;
case PERF_EVSEL__CONFIG_TERM_AUX_OUTPUT:
- attr->aux_output = term->val.aux_output ? 1 : 0;
+ attr->aux_output = term->val.num ? 1 : 0;
break;
case PERF_EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE:
/* Already applied by auxtrace */
diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
index 1f8d2fe0b66e..4e5b3ebf09cf 100644
--- a/tools/perf/util/evsel_config.h
+++ b/tools/perf/util/evsel_config.h
@@ -33,21 +33,8 @@ struct perf_evsel_config_term {
struct list_head list;
enum evsel_term_type type;
union {
- u64 period;
- u64 freq;
- bool time;
- char *callgraph;
- char *drv_cfg;
- u64 stack_user;
- int max_stack;
- bool inherit;
- bool overwrite;
- char *branch;
- unsigned long max_events;
- bool percore;
- bool aux_output;
- u32 aux_sample_size;
- u64 cfg_chg;
+ u64 num;
+ char *str;
} val;
bool weak;
};
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index ed7c008b9c8b..caf38518762f 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1219,8 +1219,7 @@ static int config_attr(struct perf_event_attr *attr,
static int get_config_terms(struct list_head *head_config,
struct list_head *head_terms __maybe_unused)
{
-#define ADD_CONFIG_TERM(__type, __name, __val) \
-do { \
+#define ADD_CONFIG_TERM(__type) \
struct perf_evsel_config_term *__t; \
\
__t = zalloc(sizeof(*__t)); \
@@ -1229,9 +1228,19 @@ do { \
\
INIT_LIST_HEAD(&__t->list); \
__t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \
- __t->val.__name = __val; \
__t->weak = term->weak; \
- list_add_tail(&__t->list, head_terms); \
+ list_add_tail(&__t->list, head_terms)
+
+#define ADD_CONFIG_TERM_VAL(__type, __val) \
+do { \
+ ADD_CONFIG_TERM(__type); \
+ __t->val.num = __val; \
+} while (0)
+
+#define ADD_CONFIG_TERM_STR(__type, __val) \
+do { \
+ ADD_CONFIG_TERM(__type); \
+ __t->val.str = __val; \
} while (0)
struct parse_events_term *term;
@@ -1239,53 +1248,53 @@ do { \
list_for_each_entry(term, head_config, list) {
switch (term->type_term) {
case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
- ADD_CONFIG_TERM(PERIOD, period, term->val.num);
+ ADD_CONFIG_TERM_VAL(PERIOD, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
- ADD_CONFIG_TERM(FREQ, freq, term->val.num);
+ ADD_CONFIG_TERM_VAL(FREQ, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_TIME:
- ADD_CONFIG_TERM(TIME, time, term->val.num);
+ ADD_CONFIG_TERM_VAL(TIME, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
- ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
+ ADD_CONFIG_TERM_STR(CALLGRAPH, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
- ADD_CONFIG_TERM(BRANCH, branch, term->val.str);
+ ADD_CONFIG_TERM_STR(BRANCH, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
- ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
+ ADD_CONFIG_TERM_VAL(STACK_USER, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_INHERIT:
- ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(INHERIT, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
- ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 1);
+ ADD_CONFIG_TERM_VAL(INHERIT, term->val.num ? 0 : 1);
break;
case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
- ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
+ ADD_CONFIG_TERM_VAL(MAX_STACK, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS:
- ADD_CONFIG_TERM(MAX_EVENTS, max_events, term->val.num);
+ ADD_CONFIG_TERM_VAL(MAX_EVENTS, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
- ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(OVERWRITE, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
- ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
+ ADD_CONFIG_TERM_VAL(OVERWRITE, term->val.num ? 0 : 1);
break;
case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
- ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
+ ADD_CONFIG_TERM_STR(DRV_CFG, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_PERCORE:
- ADD_CONFIG_TERM(PERCORE, percore,
- term->val.num ? true : false);
+ ADD_CONFIG_TERM_VAL(PERCORE,
+ term->val.num ? true : false);
break;
case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
- ADD_CONFIG_TERM(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(AUX_OUTPUT, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
- ADD_CONFIG_TERM(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
+ ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, term->val.num);
break;
default:
break;
@@ -1322,7 +1331,7 @@ static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config,
}
if (bits)
- ADD_CONFIG_TERM(CFG_CHG, cfg_chg, bits);
+ ADD_CONFIG_TERM_VAL(CFG_CHG, bits);
#undef ADD_CONFIG_TERM
return 0;
@@ -1387,7 +1396,7 @@ static bool config_term_percore(struct list_head *config_terms)
list_for_each_entry(term, config_terms, list) {
if (term->type == PERF_EVSEL__CONFIG_TERM_PERCORE)
- return term->val.percore;
+ return term->val.num;
}
return false;
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
@ 2020-01-08 14:20 ` Leo Yan
0 siblings, 0 replies; 18+ messages in thread
From: Leo Yan @ 2020-01-08 14:20 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa, Mathieu Poirier,
Suzuki K Poulose, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Namhyung Kim, Ian Rogers, Adrian Hunter,
Thomas Gleixner, Andi Kleen, linux-arm-kernel, linux-kernel,
Mike Leach
Cc: Leo Yan
The struct perf_evsel_config_term::val is a union which contains
multiple variables for corresponding types. This leads the union to
be complex and also causes complex code logic.
This patch refactors the structure to use two general variables in the
'val' union: one is 'num' for unsigned 64-bit integer and another is
'str' for string variable. This can simplify the data structure and
the related code, this also can benefit for possibly extension.
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
tools/perf/arch/arm/util/cs-etm.c | 2 +-
tools/perf/builtin-top.c | 2 +-
tools/perf/util/auxtrace.c | 2 +-
tools/perf/util/evsel.c | 24 +++++++-------
tools/perf/util/evsel_config.h | 17 ++--------
tools/perf/util/parse-events.c | 55 ++++++++++++++++++-------------
6 files changed, 49 insertions(+), 53 deletions(-)
diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
index ede040cf82ad..2898cfdf8fe1 100644
--- a/tools/perf/arch/arm/util/cs-etm.c
+++ b/tools/perf/arch/arm/util/cs-etm.c
@@ -226,7 +226,7 @@ static int cs_etm_set_sink_attr(struct perf_pmu *pmu,
if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
continue;
- sink = term->val.drv_cfg;
+ sink = term->val.str;
snprintf(path, PATH_MAX, "sinks/%s", sink);
ret = perf_pmu__scan_file(pmu, path, "%x", &hash);
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 795e353de095..459be44ca2ff 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -947,7 +947,7 @@ static int perf_top__overwrite_check(struct perf_top *top)
config_terms = &evsel->config_terms;
list_for_each_entry(term, config_terms, list) {
if (term->type == PERF_EVSEL__CONFIG_TERM_OVERWRITE)
- set = term->val.overwrite ? 1 : 0;
+ set = term->val.num ? 1 : 0;
}
/* no term for current and previous event (likely) */
diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
index eb087e7df6f4..a5c945aadfa7 100644
--- a/tools/perf/util/auxtrace.c
+++ b/tools/perf/util/auxtrace.c
@@ -772,7 +772,7 @@ int auxtrace_parse_sample_options(struct auxtrace_record *itr,
term = perf_evsel__get_config_term(evsel, AUX_SAMPLE_SIZE);
if (term) {
has_aux_sample_size = true;
- evsel->core.attr.aux_sample_size = term->val.aux_sample_size;
+ evsel->core.attr.aux_sample_size = term->val.num;
/* If possible, group with the AUX event */
if (aux_evsel && evsel->core.attr.aux_sample_size)
perf_evlist__regroup(evlist, aux_evsel, evsel);
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index a69e64236120..5f27f6b7ed94 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -789,43 +789,43 @@ static void apply_config_terms(struct evsel *evsel,
switch (term->type) {
case PERF_EVSEL__CONFIG_TERM_PERIOD:
if (!(term->weak && opts->user_interval != ULLONG_MAX)) {
- attr->sample_period = term->val.period;
+ attr->sample_period = term->val.num;
attr->freq = 0;
perf_evsel__reset_sample_bit(evsel, PERIOD);
}
break;
case PERF_EVSEL__CONFIG_TERM_FREQ:
if (!(term->weak && opts->user_freq != UINT_MAX)) {
- attr->sample_freq = term->val.freq;
+ attr->sample_freq = term->val.num;
attr->freq = 1;
perf_evsel__set_sample_bit(evsel, PERIOD);
}
break;
case PERF_EVSEL__CONFIG_TERM_TIME:
- if (term->val.time)
+ if (term->val.num)
perf_evsel__set_sample_bit(evsel, TIME);
else
perf_evsel__reset_sample_bit(evsel, TIME);
break;
case PERF_EVSEL__CONFIG_TERM_CALLGRAPH:
- callgraph_buf = term->val.callgraph;
+ callgraph_buf = term->val.str;
break;
case PERF_EVSEL__CONFIG_TERM_BRANCH:
- if (term->val.branch && strcmp(term->val.branch, "no")) {
+ if (term->val.str && strcmp(term->val.str, "no")) {
perf_evsel__set_sample_bit(evsel, BRANCH_STACK);
- parse_branch_str(term->val.branch,
+ parse_branch_str(term->val.str,
&attr->branch_sample_type);
} else
perf_evsel__reset_sample_bit(evsel, BRANCH_STACK);
break;
case PERF_EVSEL__CONFIG_TERM_STACK_USER:
- dump_size = term->val.stack_user;
+ dump_size = term->val.num;
break;
case PERF_EVSEL__CONFIG_TERM_MAX_STACK:
- max_stack = term->val.max_stack;
+ max_stack = term->val.num;
break;
case PERF_EVSEL__CONFIG_TERM_MAX_EVENTS:
- evsel->max_events = term->val.max_events;
+ evsel->max_events = term->val.num;
break;
case PERF_EVSEL__CONFIG_TERM_INHERIT:
/*
@@ -834,17 +834,17 @@ static void apply_config_terms(struct evsel *evsel,
* inherit using config terms, override global
* opt->no_inherit setting.
*/
- attr->inherit = term->val.inherit ? 1 : 0;
+ attr->inherit = term->val.num ? 1 : 0;
break;
case PERF_EVSEL__CONFIG_TERM_OVERWRITE:
- attr->write_backward = term->val.overwrite ? 1 : 0;
+ attr->write_backward = term->val.num ? 1 : 0;
break;
case PERF_EVSEL__CONFIG_TERM_DRV_CFG:
break;
case PERF_EVSEL__CONFIG_TERM_PERCORE:
break;
case PERF_EVSEL__CONFIG_TERM_AUX_OUTPUT:
- attr->aux_output = term->val.aux_output ? 1 : 0;
+ attr->aux_output = term->val.num ? 1 : 0;
break;
case PERF_EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE:
/* Already applied by auxtrace */
diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
index 1f8d2fe0b66e..4e5b3ebf09cf 100644
--- a/tools/perf/util/evsel_config.h
+++ b/tools/perf/util/evsel_config.h
@@ -33,21 +33,8 @@ struct perf_evsel_config_term {
struct list_head list;
enum evsel_term_type type;
union {
- u64 period;
- u64 freq;
- bool time;
- char *callgraph;
- char *drv_cfg;
- u64 stack_user;
- int max_stack;
- bool inherit;
- bool overwrite;
- char *branch;
- unsigned long max_events;
- bool percore;
- bool aux_output;
- u32 aux_sample_size;
- u64 cfg_chg;
+ u64 num;
+ char *str;
} val;
bool weak;
};
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index ed7c008b9c8b..caf38518762f 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1219,8 +1219,7 @@ static int config_attr(struct perf_event_attr *attr,
static int get_config_terms(struct list_head *head_config,
struct list_head *head_terms __maybe_unused)
{
-#define ADD_CONFIG_TERM(__type, __name, __val) \
-do { \
+#define ADD_CONFIG_TERM(__type) \
struct perf_evsel_config_term *__t; \
\
__t = zalloc(sizeof(*__t)); \
@@ -1229,9 +1228,19 @@ do { \
\
INIT_LIST_HEAD(&__t->list); \
__t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \
- __t->val.__name = __val; \
__t->weak = term->weak; \
- list_add_tail(&__t->list, head_terms); \
+ list_add_tail(&__t->list, head_terms)
+
+#define ADD_CONFIG_TERM_VAL(__type, __val) \
+do { \
+ ADD_CONFIG_TERM(__type); \
+ __t->val.num = __val; \
+} while (0)
+
+#define ADD_CONFIG_TERM_STR(__type, __val) \
+do { \
+ ADD_CONFIG_TERM(__type); \
+ __t->val.str = __val; \
} while (0)
struct parse_events_term *term;
@@ -1239,53 +1248,53 @@ do { \
list_for_each_entry(term, head_config, list) {
switch (term->type_term) {
case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
- ADD_CONFIG_TERM(PERIOD, period, term->val.num);
+ ADD_CONFIG_TERM_VAL(PERIOD, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
- ADD_CONFIG_TERM(FREQ, freq, term->val.num);
+ ADD_CONFIG_TERM_VAL(FREQ, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_TIME:
- ADD_CONFIG_TERM(TIME, time, term->val.num);
+ ADD_CONFIG_TERM_VAL(TIME, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
- ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
+ ADD_CONFIG_TERM_STR(CALLGRAPH, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
- ADD_CONFIG_TERM(BRANCH, branch, term->val.str);
+ ADD_CONFIG_TERM_STR(BRANCH, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
- ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
+ ADD_CONFIG_TERM_VAL(STACK_USER, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_INHERIT:
- ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(INHERIT, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
- ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 1);
+ ADD_CONFIG_TERM_VAL(INHERIT, term->val.num ? 0 : 1);
break;
case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
- ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
+ ADD_CONFIG_TERM_VAL(MAX_STACK, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS:
- ADD_CONFIG_TERM(MAX_EVENTS, max_events, term->val.num);
+ ADD_CONFIG_TERM_VAL(MAX_EVENTS, term->val.num);
break;
case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
- ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(OVERWRITE, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
- ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
+ ADD_CONFIG_TERM_VAL(OVERWRITE, term->val.num ? 0 : 1);
break;
case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
- ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
+ ADD_CONFIG_TERM_STR(DRV_CFG, term->val.str);
break;
case PARSE_EVENTS__TERM_TYPE_PERCORE:
- ADD_CONFIG_TERM(PERCORE, percore,
- term->val.num ? true : false);
+ ADD_CONFIG_TERM_VAL(PERCORE,
+ term->val.num ? true : false);
break;
case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
- ADD_CONFIG_TERM(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
+ ADD_CONFIG_TERM_VAL(AUX_OUTPUT, term->val.num ? 1 : 0);
break;
case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
- ADD_CONFIG_TERM(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
+ ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, term->val.num);
break;
default:
break;
@@ -1322,7 +1331,7 @@ static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config,
}
if (bits)
- ADD_CONFIG_TERM(CFG_CHG, cfg_chg, bits);
+ ADD_CONFIG_TERM_VAL(CFG_CHG, bits);
#undef ADD_CONFIG_TERM
return 0;
@@ -1387,7 +1396,7 @@ static bool config_term_percore(struct list_head *config_terms)
list_for_each_entry(term, config_terms, list) {
if (term->type == PERF_EVSEL__CONFIG_TERM_PERCORE)
- return term->val.percore;
+ return term->val.num;
}
return false;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 2/2] perf parse: Copy string to perf_evsel_config_term
2020-01-08 14:20 ` Leo Yan
@ 2020-01-08 14:20 ` Leo Yan
-1 siblings, 0 replies; 18+ messages in thread
From: Leo Yan @ 2020-01-08 14:20 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa, Mathieu Poirier,
Suzuki K Poulose, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Namhyung Kim, Ian Rogers, Adrian Hunter,
Thomas Gleixner, Andi Kleen, linux-arm-kernel, linux-kernel,
Mike Leach
Cc: Leo Yan
perf with CoreSight fails to record trace data with command:
perf record -e cs_etm/@tmc_etr0/u --per-thread ls
failed to set sink "" on event cs_etm/@tmc_etr0/u with 21 (Is a
directory)/perf/
This failure is root caused with the commit 1dc925568f01 ("perf
parse: Add a deep delete for parse event terms").
The log shows, cs_etm fails to parse the sink attribution; cs_etm event
relies on the event configuration to pass sink name, but the event
specific configuration data cannot be passed properly with flow:
get_config_terms()
ADD_CONFIG_TERM(DRV_CFG, term->val.str);
__t->val.str = term->val.str;
`> __t->val.str is assigned to term->val.str;
parse_events_terms__purge()
parse_events_term__delete()
zfree(&term->val.str);
`> term->val.str is freed and assigned to NULL pointer;
cs_etm_set_sink_attr()
sink = __t->val.str;
`> sink string has been freed.
To fix this issue, in the function get_config_terms(), this patch
changes to use strdup() for allocation a new duplicate string rather
than directly assignment string pointer.
This patch addes a new field 'free_str' in the data structure
perf_evsel_config_term; 'free_str' is set to true when the union is used
as a string pointer; thus it can tell perf_evsel__free_config_terms() to
free the string.
Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
Suggested-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
tools/perf/util/evsel.c | 2 ++
tools/perf/util/evsel_config.h | 1 +
tools/perf/util/parse-events.c | 7 ++++++-
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 5f27f6b7ed94..6b56b35876e7 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
list_del_init(&term->list);
+ if (term->free_str)
+ free(term->val.str);
free(term);
}
}
diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
index 4e5b3ebf09cf..12af1bca0cad 100644
--- a/tools/perf/util/evsel_config.h
+++ b/tools/perf/util/evsel_config.h
@@ -32,6 +32,7 @@ enum evsel_term_type {
struct perf_evsel_config_term {
struct list_head list;
enum evsel_term_type type;
+ bool free_str;
union {
u64 num;
char *str;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index caf38518762f..3353e9e8b134 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1240,7 +1240,12 @@ do { \
#define ADD_CONFIG_TERM_STR(__type, __val) \
do { \
ADD_CONFIG_TERM(__type); \
- __t->val.str = __val; \
+ __t->val.str = strdup(__val); \
+ if (!__t->val.str) { \
+ zfree(&__t); \
+ return -ENOMEM; \
+ } \
+ __t->free_str = true; \
} while (0)
struct parse_events_term *term;
--
2.17.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 2/2] perf parse: Copy string to perf_evsel_config_term
@ 2020-01-08 14:20 ` Leo Yan
0 siblings, 0 replies; 18+ messages in thread
From: Leo Yan @ 2020-01-08 14:20 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Jiri Olsa, Mathieu Poirier,
Suzuki K Poulose, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Namhyung Kim, Ian Rogers, Adrian Hunter,
Thomas Gleixner, Andi Kleen, linux-arm-kernel, linux-kernel,
Mike Leach
Cc: Leo Yan
perf with CoreSight fails to record trace data with command:
perf record -e cs_etm/@tmc_etr0/u --per-thread ls
failed to set sink "" on event cs_etm/@tmc_etr0/u with 21 (Is a
directory)/perf/
This failure is root caused with the commit 1dc925568f01 ("perf
parse: Add a deep delete for parse event terms").
The log shows, cs_etm fails to parse the sink attribution; cs_etm event
relies on the event configuration to pass sink name, but the event
specific configuration data cannot be passed properly with flow:
get_config_terms()
ADD_CONFIG_TERM(DRV_CFG, term->val.str);
__t->val.str = term->val.str;
`> __t->val.str is assigned to term->val.str;
parse_events_terms__purge()
parse_events_term__delete()
zfree(&term->val.str);
`> term->val.str is freed and assigned to NULL pointer;
cs_etm_set_sink_attr()
sink = __t->val.str;
`> sink string has been freed.
To fix this issue, in the function get_config_terms(), this patch
changes to use strdup() for allocation a new duplicate string rather
than directly assignment string pointer.
This patch addes a new field 'free_str' in the data structure
perf_evsel_config_term; 'free_str' is set to true when the union is used
as a string pointer; thus it can tell perf_evsel__free_config_terms() to
free the string.
Fixes: 1dc925568f01 ("perf parse: Add a deep delete for parse event terms")
Suggested-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
tools/perf/util/evsel.c | 2 ++
tools/perf/util/evsel_config.h | 1 +
tools/perf/util/parse-events.c | 7 ++++++-
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 5f27f6b7ed94..6b56b35876e7 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1265,6 +1265,8 @@ static void perf_evsel__free_config_terms(struct evsel *evsel)
list_for_each_entry_safe(term, h, &evsel->config_terms, list) {
list_del_init(&term->list);
+ if (term->free_str)
+ free(term->val.str);
free(term);
}
}
diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
index 4e5b3ebf09cf..12af1bca0cad 100644
--- a/tools/perf/util/evsel_config.h
+++ b/tools/perf/util/evsel_config.h
@@ -32,6 +32,7 @@ enum evsel_term_type {
struct perf_evsel_config_term {
struct list_head list;
enum evsel_term_type type;
+ bool free_str;
union {
u64 num;
char *str;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index caf38518762f..3353e9e8b134 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1240,7 +1240,12 @@ do { \
#define ADD_CONFIG_TERM_STR(__type, __val) \
do { \
ADD_CONFIG_TERM(__type); \
- __t->val.str = __val; \
+ __t->val.str = strdup(__val); \
+ if (!__t->val.str) { \
+ zfree(&__t); \
+ return -ENOMEM; \
+ } \
+ __t->free_str = true; \
} while (0)
struct parse_events_term *term;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
2020-01-08 14:20 ` Leo Yan
@ 2020-01-08 17:58 ` Mathieu Poirier
-1 siblings, 0 replies; 18+ messages in thread
From: Mathieu Poirier @ 2020-01-08 17:58 UTC (permalink / raw)
To: Leo Yan
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Suzuki K Poulose,
Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Namhyung Kim, Ian Rogers, Adrian Hunter, Thomas Gleixner,
Andi Kleen, linux-arm-kernel, Linux Kernel Mailing List,
Mike Leach
On Wed, 8 Jan 2020 at 07:20, Leo Yan <leo.yan@linaro.org> wrote:
>
> The struct perf_evsel_config_term::val is a union which contains
> multiple variables for corresponding types. This leads the union to
> be complex and also causes complex code logic.
>
> This patch refactors the structure to use two general variables in the
> 'val' union: one is 'num' for unsigned 64-bit integer and another is
> 'str' for string variable. This can simplify the data structure and
> the related code, this also can benefit for possibly extension.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/arch/arm/util/cs-etm.c | 2 +-
> tools/perf/builtin-top.c | 2 +-
> tools/perf/util/auxtrace.c | 2 +-
> tools/perf/util/evsel.c | 24 +++++++-------
> tools/perf/util/evsel_config.h | 17 ++--------
> tools/perf/util/parse-events.c | 55 ++++++++++++++++++-------------
> 6 files changed, 49 insertions(+), 53 deletions(-)
>
> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> index ede040cf82ad..2898cfdf8fe1 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -226,7 +226,7 @@ static int cs_etm_set_sink_attr(struct perf_pmu *pmu,
> if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
> continue;
>
> - sink = term->val.drv_cfg;
> + sink = term->val.str;
> snprintf(path, PATH_MAX, "sinks/%s", sink);
>
> ret = perf_pmu__scan_file(pmu, path, "%x", &hash);
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 795e353de095..459be44ca2ff 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -947,7 +947,7 @@ static int perf_top__overwrite_check(struct perf_top *top)
> config_terms = &evsel->config_terms;
> list_for_each_entry(term, config_terms, list) {
> if (term->type == PERF_EVSEL__CONFIG_TERM_OVERWRITE)
> - set = term->val.overwrite ? 1 : 0;
> + set = term->val.num ? 1 : 0;
> }
>
> /* no term for current and previous event (likely) */
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index eb087e7df6f4..a5c945aadfa7 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -772,7 +772,7 @@ int auxtrace_parse_sample_options(struct auxtrace_record *itr,
> term = perf_evsel__get_config_term(evsel, AUX_SAMPLE_SIZE);
> if (term) {
> has_aux_sample_size = true;
> - evsel->core.attr.aux_sample_size = term->val.aux_sample_size;
> + evsel->core.attr.aux_sample_size = term->val.num;
> /* If possible, group with the AUX event */
> if (aux_evsel && evsel->core.attr.aux_sample_size)
> perf_evlist__regroup(evlist, aux_evsel, evsel);
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index a69e64236120..5f27f6b7ed94 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -789,43 +789,43 @@ static void apply_config_terms(struct evsel *evsel,
> switch (term->type) {
> case PERF_EVSEL__CONFIG_TERM_PERIOD:
> if (!(term->weak && opts->user_interval != ULLONG_MAX)) {
> - attr->sample_period = term->val.period;
> + attr->sample_period = term->val.num;
> attr->freq = 0;
> perf_evsel__reset_sample_bit(evsel, PERIOD);
> }
> break;
> case PERF_EVSEL__CONFIG_TERM_FREQ:
> if (!(term->weak && opts->user_freq != UINT_MAX)) {
> - attr->sample_freq = term->val.freq;
> + attr->sample_freq = term->val.num;
> attr->freq = 1;
> perf_evsel__set_sample_bit(evsel, PERIOD);
> }
> break;
> case PERF_EVSEL__CONFIG_TERM_TIME:
> - if (term->val.time)
> + if (term->val.num)
> perf_evsel__set_sample_bit(evsel, TIME);
> else
> perf_evsel__reset_sample_bit(evsel, TIME);
> break;
> case PERF_EVSEL__CONFIG_TERM_CALLGRAPH:
> - callgraph_buf = term->val.callgraph;
> + callgraph_buf = term->val.str;
> break;
> case PERF_EVSEL__CONFIG_TERM_BRANCH:
> - if (term->val.branch && strcmp(term->val.branch, "no")) {
> + if (term->val.str && strcmp(term->val.str, "no")) {
> perf_evsel__set_sample_bit(evsel, BRANCH_STACK);
> - parse_branch_str(term->val.branch,
> + parse_branch_str(term->val.str,
> &attr->branch_sample_type);
> } else
> perf_evsel__reset_sample_bit(evsel, BRANCH_STACK);
> break;
> case PERF_EVSEL__CONFIG_TERM_STACK_USER:
> - dump_size = term->val.stack_user;
> + dump_size = term->val.num;
> break;
> case PERF_EVSEL__CONFIG_TERM_MAX_STACK:
> - max_stack = term->val.max_stack;
> + max_stack = term->val.num;
> break;
> case PERF_EVSEL__CONFIG_TERM_MAX_EVENTS:
> - evsel->max_events = term->val.max_events;
> + evsel->max_events = term->val.num;
> break;
> case PERF_EVSEL__CONFIG_TERM_INHERIT:
> /*
> @@ -834,17 +834,17 @@ static void apply_config_terms(struct evsel *evsel,
> * inherit using config terms, override global
> * opt->no_inherit setting.
> */
> - attr->inherit = term->val.inherit ? 1 : 0;
> + attr->inherit = term->val.num ? 1 : 0;
> break;
> case PERF_EVSEL__CONFIG_TERM_OVERWRITE:
> - attr->write_backward = term->val.overwrite ? 1 : 0;
> + attr->write_backward = term->val.num ? 1 : 0;
> break;
> case PERF_EVSEL__CONFIG_TERM_DRV_CFG:
> break;
> case PERF_EVSEL__CONFIG_TERM_PERCORE:
> break;
> case PERF_EVSEL__CONFIG_TERM_AUX_OUTPUT:
> - attr->aux_output = term->val.aux_output ? 1 : 0;
> + attr->aux_output = term->val.num ? 1 : 0;
> break;
> case PERF_EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE:
> /* Already applied by auxtrace */
> diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> index 1f8d2fe0b66e..4e5b3ebf09cf 100644
> --- a/tools/perf/util/evsel_config.h
> +++ b/tools/perf/util/evsel_config.h
> @@ -33,21 +33,8 @@ struct perf_evsel_config_term {
> struct list_head list;
> enum evsel_term_type type;
> union {
> - u64 period;
> - u64 freq;
> - bool time;
> - char *callgraph;
> - char *drv_cfg;
> - u64 stack_user;
> - int max_stack;
> - bool inherit;
> - bool overwrite;
> - char *branch;
> - unsigned long max_events;
> - bool percore;
> - bool aux_output;
> - u32 aux_sample_size;
> - u64 cfg_chg;
> + u64 num;
> + char *str;
That is a lot more than just dealing with the "char *" members. Given
the pervasiveness of the changes I would have been happy to leave
other members alone for the time being. I will let Jiri make the
final call but if we are to proceed this way I think we should have a
member per type to avoid casting issues.
Thanks,
Mathieu
> } val;
> bool weak;
> };
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index ed7c008b9c8b..caf38518762f 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1219,8 +1219,7 @@ static int config_attr(struct perf_event_attr *attr,
> static int get_config_terms(struct list_head *head_config,
> struct list_head *head_terms __maybe_unused)
> {
> -#define ADD_CONFIG_TERM(__type, __name, __val) \
> -do { \
> +#define ADD_CONFIG_TERM(__type) \
> struct perf_evsel_config_term *__t; \
> \
> __t = zalloc(sizeof(*__t)); \
> @@ -1229,9 +1228,19 @@ do { \
> \
> INIT_LIST_HEAD(&__t->list); \
> __t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \
> - __t->val.__name = __val; \
> __t->weak = term->weak; \
> - list_add_tail(&__t->list, head_terms); \
> + list_add_tail(&__t->list, head_terms)
> +
> +#define ADD_CONFIG_TERM_VAL(__type, __val) \
> +do { \
> + ADD_CONFIG_TERM(__type); \
> + __t->val.num = __val; \
> +} while (0)
> +
> +#define ADD_CONFIG_TERM_STR(__type, __val) \
> +do { \
> + ADD_CONFIG_TERM(__type); \
> + __t->val.str = __val; \
> } while (0)
>
> struct parse_events_term *term;
> @@ -1239,53 +1248,53 @@ do { \
> list_for_each_entry(term, head_config, list) {
> switch (term->type_term) {
> case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
> - ADD_CONFIG_TERM(PERIOD, period, term->val.num);
> + ADD_CONFIG_TERM_VAL(PERIOD, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
> - ADD_CONFIG_TERM(FREQ, freq, term->val.num);
> + ADD_CONFIG_TERM_VAL(FREQ, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_TIME:
> - ADD_CONFIG_TERM(TIME, time, term->val.num);
> + ADD_CONFIG_TERM_VAL(TIME, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
> - ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
> + ADD_CONFIG_TERM_STR(CALLGRAPH, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
> - ADD_CONFIG_TERM(BRANCH, branch, term->val.str);
> + ADD_CONFIG_TERM_STR(BRANCH, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
> - ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
> + ADD_CONFIG_TERM_VAL(STACK_USER, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_INHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(INHERIT, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 1);
> + ADD_CONFIG_TERM_VAL(INHERIT, term->val.num ? 0 : 1);
> break;
> case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
> - ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
> + ADD_CONFIG_TERM_VAL(MAX_STACK, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS:
> - ADD_CONFIG_TERM(MAX_EVENTS, max_events, term->val.num);
> + ADD_CONFIG_TERM_VAL(MAX_EVENTS, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(OVERWRITE, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
> + ADD_CONFIG_TERM_VAL(OVERWRITE, term->val.num ? 0 : 1);
> break;
> case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
> - ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
> + ADD_CONFIG_TERM_STR(DRV_CFG, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_PERCORE:
> - ADD_CONFIG_TERM(PERCORE, percore,
> - term->val.num ? true : false);
> + ADD_CONFIG_TERM_VAL(PERCORE,
> + term->val.num ? true : false);
> break;
> case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
> - ADD_CONFIG_TERM(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(AUX_OUTPUT, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
> - ADD_CONFIG_TERM(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
> + ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, term->val.num);
> break;
> default:
> break;
> @@ -1322,7 +1331,7 @@ static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config,
> }
>
> if (bits)
> - ADD_CONFIG_TERM(CFG_CHG, cfg_chg, bits);
> + ADD_CONFIG_TERM_VAL(CFG_CHG, bits);
>
> #undef ADD_CONFIG_TERM
> return 0;
> @@ -1387,7 +1396,7 @@ static bool config_term_percore(struct list_head *config_terms)
>
> list_for_each_entry(term, config_terms, list) {
> if (term->type == PERF_EVSEL__CONFIG_TERM_PERCORE)
> - return term->val.percore;
> + return term->val.num;
> }
>
> return false;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
@ 2020-01-08 17:58 ` Mathieu Poirier
0 siblings, 0 replies; 18+ messages in thread
From: Mathieu Poirier @ 2020-01-08 17:58 UTC (permalink / raw)
To: Leo Yan
Cc: Mark Rutland, Ian Rogers, Andi Kleen, Suzuki K Poulose,
Peter Zijlstra, Adrian Hunter, Linux Kernel Mailing List,
Arnaldo Carvalho de Melo, Alexander Shishkin, Ingo Molnar,
Jiri Olsa, Namhyung Kim, Thomas Gleixner, linux-arm-kernel,
Mike Leach
On Wed, 8 Jan 2020 at 07:20, Leo Yan <leo.yan@linaro.org> wrote:
>
> The struct perf_evsel_config_term::val is a union which contains
> multiple variables for corresponding types. This leads the union to
> be complex and also causes complex code logic.
>
> This patch refactors the structure to use two general variables in the
> 'val' union: one is 'num' for unsigned 64-bit integer and another is
> 'str' for string variable. This can simplify the data structure and
> the related code, this also can benefit for possibly extension.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> tools/perf/arch/arm/util/cs-etm.c | 2 +-
> tools/perf/builtin-top.c | 2 +-
> tools/perf/util/auxtrace.c | 2 +-
> tools/perf/util/evsel.c | 24 +++++++-------
> tools/perf/util/evsel_config.h | 17 ++--------
> tools/perf/util/parse-events.c | 55 ++++++++++++++++++-------------
> 6 files changed, 49 insertions(+), 53 deletions(-)
>
> diff --git a/tools/perf/arch/arm/util/cs-etm.c b/tools/perf/arch/arm/util/cs-etm.c
> index ede040cf82ad..2898cfdf8fe1 100644
> --- a/tools/perf/arch/arm/util/cs-etm.c
> +++ b/tools/perf/arch/arm/util/cs-etm.c
> @@ -226,7 +226,7 @@ static int cs_etm_set_sink_attr(struct perf_pmu *pmu,
> if (term->type != PERF_EVSEL__CONFIG_TERM_DRV_CFG)
> continue;
>
> - sink = term->val.drv_cfg;
> + sink = term->val.str;
> snprintf(path, PATH_MAX, "sinks/%s", sink);
>
> ret = perf_pmu__scan_file(pmu, path, "%x", &hash);
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 795e353de095..459be44ca2ff 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -947,7 +947,7 @@ static int perf_top__overwrite_check(struct perf_top *top)
> config_terms = &evsel->config_terms;
> list_for_each_entry(term, config_terms, list) {
> if (term->type == PERF_EVSEL__CONFIG_TERM_OVERWRITE)
> - set = term->val.overwrite ? 1 : 0;
> + set = term->val.num ? 1 : 0;
> }
>
> /* no term for current and previous event (likely) */
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index eb087e7df6f4..a5c945aadfa7 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -772,7 +772,7 @@ int auxtrace_parse_sample_options(struct auxtrace_record *itr,
> term = perf_evsel__get_config_term(evsel, AUX_SAMPLE_SIZE);
> if (term) {
> has_aux_sample_size = true;
> - evsel->core.attr.aux_sample_size = term->val.aux_sample_size;
> + evsel->core.attr.aux_sample_size = term->val.num;
> /* If possible, group with the AUX event */
> if (aux_evsel && evsel->core.attr.aux_sample_size)
> perf_evlist__regroup(evlist, aux_evsel, evsel);
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index a69e64236120..5f27f6b7ed94 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -789,43 +789,43 @@ static void apply_config_terms(struct evsel *evsel,
> switch (term->type) {
> case PERF_EVSEL__CONFIG_TERM_PERIOD:
> if (!(term->weak && opts->user_interval != ULLONG_MAX)) {
> - attr->sample_period = term->val.period;
> + attr->sample_period = term->val.num;
> attr->freq = 0;
> perf_evsel__reset_sample_bit(evsel, PERIOD);
> }
> break;
> case PERF_EVSEL__CONFIG_TERM_FREQ:
> if (!(term->weak && opts->user_freq != UINT_MAX)) {
> - attr->sample_freq = term->val.freq;
> + attr->sample_freq = term->val.num;
> attr->freq = 1;
> perf_evsel__set_sample_bit(evsel, PERIOD);
> }
> break;
> case PERF_EVSEL__CONFIG_TERM_TIME:
> - if (term->val.time)
> + if (term->val.num)
> perf_evsel__set_sample_bit(evsel, TIME);
> else
> perf_evsel__reset_sample_bit(evsel, TIME);
> break;
> case PERF_EVSEL__CONFIG_TERM_CALLGRAPH:
> - callgraph_buf = term->val.callgraph;
> + callgraph_buf = term->val.str;
> break;
> case PERF_EVSEL__CONFIG_TERM_BRANCH:
> - if (term->val.branch && strcmp(term->val.branch, "no")) {
> + if (term->val.str && strcmp(term->val.str, "no")) {
> perf_evsel__set_sample_bit(evsel, BRANCH_STACK);
> - parse_branch_str(term->val.branch,
> + parse_branch_str(term->val.str,
> &attr->branch_sample_type);
> } else
> perf_evsel__reset_sample_bit(evsel, BRANCH_STACK);
> break;
> case PERF_EVSEL__CONFIG_TERM_STACK_USER:
> - dump_size = term->val.stack_user;
> + dump_size = term->val.num;
> break;
> case PERF_EVSEL__CONFIG_TERM_MAX_STACK:
> - max_stack = term->val.max_stack;
> + max_stack = term->val.num;
> break;
> case PERF_EVSEL__CONFIG_TERM_MAX_EVENTS:
> - evsel->max_events = term->val.max_events;
> + evsel->max_events = term->val.num;
> break;
> case PERF_EVSEL__CONFIG_TERM_INHERIT:
> /*
> @@ -834,17 +834,17 @@ static void apply_config_terms(struct evsel *evsel,
> * inherit using config terms, override global
> * opt->no_inherit setting.
> */
> - attr->inherit = term->val.inherit ? 1 : 0;
> + attr->inherit = term->val.num ? 1 : 0;
> break;
> case PERF_EVSEL__CONFIG_TERM_OVERWRITE:
> - attr->write_backward = term->val.overwrite ? 1 : 0;
> + attr->write_backward = term->val.num ? 1 : 0;
> break;
> case PERF_EVSEL__CONFIG_TERM_DRV_CFG:
> break;
> case PERF_EVSEL__CONFIG_TERM_PERCORE:
> break;
> case PERF_EVSEL__CONFIG_TERM_AUX_OUTPUT:
> - attr->aux_output = term->val.aux_output ? 1 : 0;
> + attr->aux_output = term->val.num ? 1 : 0;
> break;
> case PERF_EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE:
> /* Already applied by auxtrace */
> diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> index 1f8d2fe0b66e..4e5b3ebf09cf 100644
> --- a/tools/perf/util/evsel_config.h
> +++ b/tools/perf/util/evsel_config.h
> @@ -33,21 +33,8 @@ struct perf_evsel_config_term {
> struct list_head list;
> enum evsel_term_type type;
> union {
> - u64 period;
> - u64 freq;
> - bool time;
> - char *callgraph;
> - char *drv_cfg;
> - u64 stack_user;
> - int max_stack;
> - bool inherit;
> - bool overwrite;
> - char *branch;
> - unsigned long max_events;
> - bool percore;
> - bool aux_output;
> - u32 aux_sample_size;
> - u64 cfg_chg;
> + u64 num;
> + char *str;
That is a lot more than just dealing with the "char *" members. Given
the pervasiveness of the changes I would have been happy to leave
other members alone for the time being. I will let Jiri make the
final call but if we are to proceed this way I think we should have a
member per type to avoid casting issues.
Thanks,
Mathieu
> } val;
> bool weak;
> };
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index ed7c008b9c8b..caf38518762f 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1219,8 +1219,7 @@ static int config_attr(struct perf_event_attr *attr,
> static int get_config_terms(struct list_head *head_config,
> struct list_head *head_terms __maybe_unused)
> {
> -#define ADD_CONFIG_TERM(__type, __name, __val) \
> -do { \
> +#define ADD_CONFIG_TERM(__type) \
> struct perf_evsel_config_term *__t; \
> \
> __t = zalloc(sizeof(*__t)); \
> @@ -1229,9 +1228,19 @@ do { \
> \
> INIT_LIST_HEAD(&__t->list); \
> __t->type = PERF_EVSEL__CONFIG_TERM_ ## __type; \
> - __t->val.__name = __val; \
> __t->weak = term->weak; \
> - list_add_tail(&__t->list, head_terms); \
> + list_add_tail(&__t->list, head_terms)
> +
> +#define ADD_CONFIG_TERM_VAL(__type, __val) \
> +do { \
> + ADD_CONFIG_TERM(__type); \
> + __t->val.num = __val; \
> +} while (0)
> +
> +#define ADD_CONFIG_TERM_STR(__type, __val) \
> +do { \
> + ADD_CONFIG_TERM(__type); \
> + __t->val.str = __val; \
> } while (0)
>
> struct parse_events_term *term;
> @@ -1239,53 +1248,53 @@ do { \
> list_for_each_entry(term, head_config, list) {
> switch (term->type_term) {
> case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
> - ADD_CONFIG_TERM(PERIOD, period, term->val.num);
> + ADD_CONFIG_TERM_VAL(PERIOD, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
> - ADD_CONFIG_TERM(FREQ, freq, term->val.num);
> + ADD_CONFIG_TERM_VAL(FREQ, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_TIME:
> - ADD_CONFIG_TERM(TIME, time, term->val.num);
> + ADD_CONFIG_TERM_VAL(TIME, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
> - ADD_CONFIG_TERM(CALLGRAPH, callgraph, term->val.str);
> + ADD_CONFIG_TERM_STR(CALLGRAPH, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
> - ADD_CONFIG_TERM(BRANCH, branch, term->val.str);
> + ADD_CONFIG_TERM_STR(BRANCH, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
> - ADD_CONFIG_TERM(STACK_USER, stack_user, term->val.num);
> + ADD_CONFIG_TERM_VAL(STACK_USER, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_INHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(INHERIT, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
> - ADD_CONFIG_TERM(INHERIT, inherit, term->val.num ? 0 : 1);
> + ADD_CONFIG_TERM_VAL(INHERIT, term->val.num ? 0 : 1);
> break;
> case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
> - ADD_CONFIG_TERM(MAX_STACK, max_stack, term->val.num);
> + ADD_CONFIG_TERM_VAL(MAX_STACK, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS:
> - ADD_CONFIG_TERM(MAX_EVENTS, max_events, term->val.num);
> + ADD_CONFIG_TERM_VAL(MAX_EVENTS, term->val.num);
> break;
> case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(OVERWRITE, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> - ADD_CONFIG_TERM(OVERWRITE, overwrite, term->val.num ? 0 : 1);
> + ADD_CONFIG_TERM_VAL(OVERWRITE, term->val.num ? 0 : 1);
> break;
> case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
> - ADD_CONFIG_TERM(DRV_CFG, drv_cfg, term->val.str);
> + ADD_CONFIG_TERM_STR(DRV_CFG, term->val.str);
> break;
> case PARSE_EVENTS__TERM_TYPE_PERCORE:
> - ADD_CONFIG_TERM(PERCORE, percore,
> - term->val.num ? true : false);
> + ADD_CONFIG_TERM_VAL(PERCORE,
> + term->val.num ? true : false);
> break;
> case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
> - ADD_CONFIG_TERM(AUX_OUTPUT, aux_output, term->val.num ? 1 : 0);
> + ADD_CONFIG_TERM_VAL(AUX_OUTPUT, term->val.num ? 1 : 0);
> break;
> case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
> - ADD_CONFIG_TERM(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num);
> + ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, term->val.num);
> break;
> default:
> break;
> @@ -1322,7 +1331,7 @@ static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config,
> }
>
> if (bits)
> - ADD_CONFIG_TERM(CFG_CHG, cfg_chg, bits);
> + ADD_CONFIG_TERM_VAL(CFG_CHG, bits);
>
> #undef ADD_CONFIG_TERM
> return 0;
> @@ -1387,7 +1396,7 @@ static bool config_term_percore(struct list_head *config_terms)
>
> list_for_each_entry(term, config_terms, list) {
> if (term->type == PERF_EVSEL__CONFIG_TERM_PERCORE)
> - return term->val.percore;
> + return term->val.num;
> }
>
> return false;
> --
> 2.17.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
2020-01-08 17:58 ` Mathieu Poirier
@ 2020-01-09 5:08 ` Leo Yan
-1 siblings, 0 replies; 18+ messages in thread
From: Leo Yan @ 2020-01-09 5:08 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Suzuki K Poulose,
Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Namhyung Kim, Ian Rogers, Adrian Hunter, Thomas Gleixner,
Andi Kleen, linux-arm-kernel, Linux Kernel Mailing List,
Mike Leach
Hi Mathieu,
On Wed, Jan 08, 2020 at 10:58:31AM -0700, Mathieu Poirier wrote:
[...]
> > diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> > index 1f8d2fe0b66e..4e5b3ebf09cf 100644
> > --- a/tools/perf/util/evsel_config.h
> > +++ b/tools/perf/util/evsel_config.h
> > @@ -33,21 +33,8 @@ struct perf_evsel_config_term {
> > struct list_head list;
> > enum evsel_term_type type;
> > union {
> > - u64 period;
> > - u64 freq;
> > - bool time;
> > - char *callgraph;
> > - char *drv_cfg;
> > - u64 stack_user;
> > - int max_stack;
> > - bool inherit;
> > - bool overwrite;
> > - char *branch;
> > - unsigned long max_events;
> > - bool percore;
> > - bool aux_output;
> > - u32 aux_sample_size;
> > - u64 cfg_chg;
> > + u64 num;
> > + char *str;
>
> That is a lot more than just dealing with the "char *" members. Given
> the pervasiveness of the changes I would have been happy to leave
> other members alone for the time being.
I think actually you are suggesting like below which add general
members and also keep the old members. If so, I prefer to add two
general members 'num' and 'str'.
struct perf_evsel_config_term {
struct list_head list;
enum evsel_term_type type;
union {
u64 period;
u64 freq;
bool time;
char *callgraph;
char *drv_cfg;
u64 stack_user;
int max_stack;
bool inherit;
bool overwrite;
char *branch;
unsigned long max_events;
bool percore;
bool aux_output;
u32 aux_sample_size;
u64 cfg_chg;
+ u64 num;
+ char *str;
} val;
bool weak;
};
> I will let Jiri make the
> final call but if we are to proceed this way I think we should have a
> member per type to avoid casting issues.
Yeah, let's see what's Jiri thinking.
Just note, with this change, I don't see any casting warning or errors
when built perf on arm64/arm32.
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
@ 2020-01-09 5:08 ` Leo Yan
0 siblings, 0 replies; 18+ messages in thread
From: Leo Yan @ 2020-01-09 5:08 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Mark Rutland, Ian Rogers, Andi Kleen, Suzuki K Poulose,
Peter Zijlstra, Adrian Hunter, Linux Kernel Mailing List,
Arnaldo Carvalho de Melo, Alexander Shishkin, Ingo Molnar,
Jiri Olsa, Namhyung Kim, Thomas Gleixner, linux-arm-kernel,
Mike Leach
Hi Mathieu,
On Wed, Jan 08, 2020 at 10:58:31AM -0700, Mathieu Poirier wrote:
[...]
> > diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> > index 1f8d2fe0b66e..4e5b3ebf09cf 100644
> > --- a/tools/perf/util/evsel_config.h
> > +++ b/tools/perf/util/evsel_config.h
> > @@ -33,21 +33,8 @@ struct perf_evsel_config_term {
> > struct list_head list;
> > enum evsel_term_type type;
> > union {
> > - u64 period;
> > - u64 freq;
> > - bool time;
> > - char *callgraph;
> > - char *drv_cfg;
> > - u64 stack_user;
> > - int max_stack;
> > - bool inherit;
> > - bool overwrite;
> > - char *branch;
> > - unsigned long max_events;
> > - bool percore;
> > - bool aux_output;
> > - u32 aux_sample_size;
> > - u64 cfg_chg;
> > + u64 num;
> > + char *str;
>
> That is a lot more than just dealing with the "char *" members. Given
> the pervasiveness of the changes I would have been happy to leave
> other members alone for the time being.
I think actually you are suggesting like below which add general
members and also keep the old members. If so, I prefer to add two
general members 'num' and 'str'.
struct perf_evsel_config_term {
struct list_head list;
enum evsel_term_type type;
union {
u64 period;
u64 freq;
bool time;
char *callgraph;
char *drv_cfg;
u64 stack_user;
int max_stack;
bool inherit;
bool overwrite;
char *branch;
unsigned long max_events;
bool percore;
bool aux_output;
u32 aux_sample_size;
u64 cfg_chg;
+ u64 num;
+ char *str;
} val;
bool weak;
};
> I will let Jiri make the
> final call but if we are to proceed this way I think we should have a
> member per type to avoid casting issues.
Yeah, let's see what's Jiri thinking.
Just note, with this change, I don't see any casting warning or errors
when built perf on arm64/arm32.
Thanks,
Leo Yan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
2020-01-09 5:08 ` Leo Yan
@ 2020-01-09 16:34 ` Mathieu Poirier
-1 siblings, 0 replies; 18+ messages in thread
From: Mathieu Poirier @ 2020-01-09 16:34 UTC (permalink / raw)
To: Leo Yan
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Suzuki K Poulose,
Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Namhyung Kim, Ian Rogers, Adrian Hunter, Thomas Gleixner,
Andi Kleen, linux-arm-kernel, Linux Kernel Mailing List,
Mike Leach
On Thu, Jan 09, 2020 at 01:08:06PM +0800, Leo Yan wrote:
> Hi Mathieu,
>
> On Wed, Jan 08, 2020 at 10:58:31AM -0700, Mathieu Poirier wrote:
>
> [...]
>
> > > diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> > > index 1f8d2fe0b66e..4e5b3ebf09cf 100644
> > > --- a/tools/perf/util/evsel_config.h
> > > +++ b/tools/perf/util/evsel_config.h
> > > @@ -33,21 +33,8 @@ struct perf_evsel_config_term {
> > > struct list_head list;
> > > enum evsel_term_type type;
> > > union {
> > > - u64 period;
> > > - u64 freq;
> > > - bool time;
> > > - char *callgraph;
> > > - char *drv_cfg;
> > > - u64 stack_user;
> > > - int max_stack;
> > > - bool inherit;
> > > - bool overwrite;
> > > - char *branch;
> > > - unsigned long max_events;
> > > - bool percore;
> > > - bool aux_output;
> > > - u32 aux_sample_size;
> > > - u64 cfg_chg;
> > > + u64 num;
> > > + char *str;
> >
> > That is a lot more than just dealing with the "char *" members. Given
> > the pervasiveness of the changes I would have been happy to leave
> > other members alone for the time being.
>
> I think actually you are suggesting like below which add general
> members and also keep the old members. If so, I prefer to add two
> general members 'num' and 'str'.
If we are to deal with all flields of the union, I think it should be as below:
union {
bool cfg_bool;
int cfg_int;
unsigned long cfg_ulong;
u32 cfg_u32;
char *cfg_str;
} val;
But just dealing with the "char *" as below would also be fine with me:
union {
u64 period;
u64 freq;
bool time;
u64 stack_user;
int max_stack;
bool inherit;
bool overwrite;
unsigned long max_events;
bool percore;
bool aux_output;
u32 aux_sample_size;
u64 cfg_chg;
u64 num;
char *str;
} val;
>
> struct perf_evsel_config_term {
> struct list_head list;
> enum evsel_term_type type;
> union {
> u64 period;
> u64 freq;
> bool time;
> char *callgraph;
> char *drv_cfg;
> u64 stack_user;
> int max_stack;
> bool inherit;
> bool overwrite;
> char *branch;
> unsigned long max_events;
> bool percore;
> bool aux_output;
> u32 aux_sample_size;
> u64 cfg_chg;
> + u64 num;
> + char *str;
> } val;
> bool weak;
> };
>
> > I will let Jiri make the
> > final call but if we are to proceed this way I think we should have a
> > member per type to avoid casting issues.
>
> Yeah, let's see what's Jiri thinking.
>
> Just note, with this change, I don't see any casting warning or errors
> when built perf on arm64/arm32.
At this time you may not, but they will happen and it will be very hard to
debug.
>
> Thanks,
> Leo Yan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
@ 2020-01-09 16:34 ` Mathieu Poirier
0 siblings, 0 replies; 18+ messages in thread
From: Mathieu Poirier @ 2020-01-09 16:34 UTC (permalink / raw)
To: Leo Yan
Cc: Mark Rutland, Ian Rogers, Andi Kleen, Suzuki K Poulose,
Peter Zijlstra, Adrian Hunter, Linux Kernel Mailing List,
Arnaldo Carvalho de Melo, Alexander Shishkin, Ingo Molnar,
Jiri Olsa, Namhyung Kim, Thomas Gleixner, linux-arm-kernel,
Mike Leach
On Thu, Jan 09, 2020 at 01:08:06PM +0800, Leo Yan wrote:
> Hi Mathieu,
>
> On Wed, Jan 08, 2020 at 10:58:31AM -0700, Mathieu Poirier wrote:
>
> [...]
>
> > > diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> > > index 1f8d2fe0b66e..4e5b3ebf09cf 100644
> > > --- a/tools/perf/util/evsel_config.h
> > > +++ b/tools/perf/util/evsel_config.h
> > > @@ -33,21 +33,8 @@ struct perf_evsel_config_term {
> > > struct list_head list;
> > > enum evsel_term_type type;
> > > union {
> > > - u64 period;
> > > - u64 freq;
> > > - bool time;
> > > - char *callgraph;
> > > - char *drv_cfg;
> > > - u64 stack_user;
> > > - int max_stack;
> > > - bool inherit;
> > > - bool overwrite;
> > > - char *branch;
> > > - unsigned long max_events;
> > > - bool percore;
> > > - bool aux_output;
> > > - u32 aux_sample_size;
> > > - u64 cfg_chg;
> > > + u64 num;
> > > + char *str;
> >
> > That is a lot more than just dealing with the "char *" members. Given
> > the pervasiveness of the changes I would have been happy to leave
> > other members alone for the time being.
>
> I think actually you are suggesting like below which add general
> members and also keep the old members. If so, I prefer to add two
> general members 'num' and 'str'.
If we are to deal with all flields of the union, I think it should be as below:
union {
bool cfg_bool;
int cfg_int;
unsigned long cfg_ulong;
u32 cfg_u32;
char *cfg_str;
} val;
But just dealing with the "char *" as below would also be fine with me:
union {
u64 period;
u64 freq;
bool time;
u64 stack_user;
int max_stack;
bool inherit;
bool overwrite;
unsigned long max_events;
bool percore;
bool aux_output;
u32 aux_sample_size;
u64 cfg_chg;
u64 num;
char *str;
} val;
>
> struct perf_evsel_config_term {
> struct list_head list;
> enum evsel_term_type type;
> union {
> u64 period;
> u64 freq;
> bool time;
> char *callgraph;
> char *drv_cfg;
> u64 stack_user;
> int max_stack;
> bool inherit;
> bool overwrite;
> char *branch;
> unsigned long max_events;
> bool percore;
> bool aux_output;
> u32 aux_sample_size;
> u64 cfg_chg;
> + u64 num;
> + char *str;
> } val;
> bool weak;
> };
>
> > I will let Jiri make the
> > final call but if we are to proceed this way I think we should have a
> > member per type to avoid casting issues.
>
> Yeah, let's see what's Jiri thinking.
>
> Just note, with this change, I don't see any casting warning or errors
> when built perf on arm64/arm32.
At this time you may not, but they will happen and it will be very hard to
debug.
>
> Thanks,
> Leo Yan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
2020-01-09 16:34 ` Mathieu Poirier
@ 2020-01-10 15:04 ` Jiri Olsa
-1 siblings, 0 replies; 18+ messages in thread
From: Jiri Olsa @ 2020-01-10 15:04 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Leo Yan, Arnaldo Carvalho de Melo, Jiri Olsa, Suzuki K Poulose,
Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Namhyung Kim, Ian Rogers, Adrian Hunter, Thomas Gleixner,
Andi Kleen, linux-arm-kernel, Linux Kernel Mailing List,
Mike Leach
On Thu, Jan 09, 2020 at 09:34:24AM -0700, Mathieu Poirier wrote:
SNIP
>
> If we are to deal with all flields of the union, I think it should be as below:
>
> union {
> bool cfg_bool;
> int cfg_int;
> unsigned long cfg_ulong;
> u32 cfg_u32;
> char *cfg_str;
> } val;
>
> But just dealing with the "char *" as below would also be fine with me:
>
> union {
> u64 period;
> u64 freq;
> bool time;
> u64 stack_user;
> int max_stack;
> bool inherit;
> bool overwrite;
> unsigned long max_events;
> bool percore;
> bool aux_output;
> u32 aux_sample_size;
> u64 cfg_chg;
> u64 num;
> char *str;
> } val;
>
> >
> > struct perf_evsel_config_term {
> > struct list_head list;
> > enum evsel_term_type type;
> > union {
> > u64 period;
> > u64 freq;
> > bool time;
> > char *callgraph;
> > char *drv_cfg;
> > u64 stack_user;
> > int max_stack;
> > bool inherit;
> > bool overwrite;
> > char *branch;
> > unsigned long max_events;
> > bool percore;
> > bool aux_output;
> > u32 aux_sample_size;
> > u64 cfg_chg;
> > + u64 num;
> > + char *str;
> > } val;
> > bool weak;
> > };
> >
> > > I will let Jiri make the
> > > final call but if we are to proceed this way I think we should have a
> > > member per type to avoid casting issues.
> >
> > Yeah, let's see what's Jiri thinking.
> >
> > Just note, with this change, I don't see any casting warning or errors
> > when built perf on arm64/arm32.
>
> At this time you may not, but they will happen and it will be very hard to
> debug.
hi,
sry for late reply..
I think ;-) we should either add all different types to the union
or just add 'str' pointer to handle strings correctly.. which seems
better, because it's less changes and there's no real issue that
would need that other bigger change
jirka
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
@ 2020-01-10 15:04 ` Jiri Olsa
0 siblings, 0 replies; 18+ messages in thread
From: Jiri Olsa @ 2020-01-10 15:04 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Mark Rutland, Ian Rogers, Andi Kleen, Suzuki K Poulose,
Peter Zijlstra, Adrian Hunter, Linux Kernel Mailing List,
Arnaldo Carvalho de Melo, Alexander Shishkin, Ingo Molnar,
Jiri Olsa, Leo Yan, Namhyung Kim, Thomas Gleixner,
linux-arm-kernel, Mike Leach
On Thu, Jan 09, 2020 at 09:34:24AM -0700, Mathieu Poirier wrote:
SNIP
>
> If we are to deal with all flields of the union, I think it should be as below:
>
> union {
> bool cfg_bool;
> int cfg_int;
> unsigned long cfg_ulong;
> u32 cfg_u32;
> char *cfg_str;
> } val;
>
> But just dealing with the "char *" as below would also be fine with me:
>
> union {
> u64 period;
> u64 freq;
> bool time;
> u64 stack_user;
> int max_stack;
> bool inherit;
> bool overwrite;
> unsigned long max_events;
> bool percore;
> bool aux_output;
> u32 aux_sample_size;
> u64 cfg_chg;
> u64 num;
> char *str;
> } val;
>
> >
> > struct perf_evsel_config_term {
> > struct list_head list;
> > enum evsel_term_type type;
> > union {
> > u64 period;
> > u64 freq;
> > bool time;
> > char *callgraph;
> > char *drv_cfg;
> > u64 stack_user;
> > int max_stack;
> > bool inherit;
> > bool overwrite;
> > char *branch;
> > unsigned long max_events;
> > bool percore;
> > bool aux_output;
> > u32 aux_sample_size;
> > u64 cfg_chg;
> > + u64 num;
> > + char *str;
> > } val;
> > bool weak;
> > };
> >
> > > I will let Jiri make the
> > > final call but if we are to proceed this way I think we should have a
> > > member per type to avoid casting issues.
> >
> > Yeah, let's see what's Jiri thinking.
> >
> > Just note, with this change, I don't see any casting warning or errors
> > when built perf on arm64/arm32.
>
> At this time you may not, but they will happen and it will be very hard to
> debug.
hi,
sry for late reply..
I think ;-) we should either add all different types to the union
or just add 'str' pointer to handle strings correctly.. which seems
better, because it's less changes and there's no real issue that
would need that other bigger change
jirka
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
2020-01-08 14:20 ` Leo Yan
@ 2020-01-10 15:04 ` Jiri Olsa
-1 siblings, 0 replies; 18+ messages in thread
From: Jiri Olsa @ 2020-01-10 15:04 UTC (permalink / raw)
To: Leo Yan
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Mathieu Poirier,
Suzuki K Poulose, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Namhyung Kim, Ian Rogers, Adrian Hunter,
Thomas Gleixner, Andi Kleen, linux-arm-kernel, linux-kernel,
Mike Leach
On Wed, Jan 08, 2020 at 10:20:09PM +0800, Leo Yan wrote:
> The struct perf_evsel_config_term::val is a union which contains
> multiple variables for corresponding types. This leads the union to
> be complex and also causes complex code logic.
>
> This patch refactors the structure to use two general variables in the
> 'val' union: one is 'num' for unsigned 64-bit integer and another is
> 'str' for string variable. This can simplify the data structure and
> the related code, this also can benefit for possibly extension.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
there's some arch code that needs to be changed.. please
change other archs as well
CC arch/x86/util/intel-pt.o
arch/x86/util/intel-pt.c: In function ‘intel_pt_config_sample_mode’:
arch/x86/util/intel-pt.c:563:24: error: ‘union <anonymous>’ has no member named ‘cfg_chg’
563 | user_bits = term->val.cfg_chg;
thanks,
jirka
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
@ 2020-01-10 15:04 ` Jiri Olsa
0 siblings, 0 replies; 18+ messages in thread
From: Jiri Olsa @ 2020-01-10 15:04 UTC (permalink / raw)
To: Leo Yan
Cc: Mark Rutland, Ian Rogers, Andi Kleen, Mathieu Poirier,
Suzuki K Poulose, Peter Zijlstra, Adrian Hunter, linux-kernel,
Arnaldo Carvalho de Melo, Alexander Shishkin, Ingo Molnar,
Jiri Olsa, Namhyung Kim, Thomas Gleixner, linux-arm-kernel,
Mike Leach
On Wed, Jan 08, 2020 at 10:20:09PM +0800, Leo Yan wrote:
> The struct perf_evsel_config_term::val is a union which contains
> multiple variables for corresponding types. This leads the union to
> be complex and also causes complex code logic.
>
> This patch refactors the structure to use two general variables in the
> 'val' union: one is 'num' for unsigned 64-bit integer and another is
> 'str' for string variable. This can simplify the data structure and
> the related code, this also can benefit for possibly extension.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
there's some arch code that needs to be changed.. please
change other archs as well
CC arch/x86/util/intel-pt.o
arch/x86/util/intel-pt.c: In function ‘intel_pt_config_sample_mode’:
arch/x86/util/intel-pt.c:563:24: error: ‘union <anonymous>’ has no member named ‘cfg_chg’
563 | user_bits = term->val.cfg_chg;
thanks,
jirka
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
2020-01-10 15:04 ` Jiri Olsa
@ 2020-01-13 15:21 ` Leo Yan
-1 siblings, 0 replies; 18+ messages in thread
From: Leo Yan @ 2020-01-13 15:21 UTC (permalink / raw)
To: Jiri Olsa
Cc: Mathieu Poirier, Arnaldo Carvalho de Melo, Jiri Olsa,
Suzuki K Poulose, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Namhyung Kim, Ian Rogers, Adrian Hunter,
Thomas Gleixner, Andi Kleen, linux-arm-kernel,
Linux Kernel Mailing List, Mike Leach
On Fri, Jan 10, 2020 at 04:04:10PM +0100, Jiri Olsa wrote:
> On Thu, Jan 09, 2020 at 09:34:24AM -0700, Mathieu Poirier wrote:
>
> SNIP
>
> >
> > If we are to deal with all flields of the union, I think it should be as below:
> >
> > union {
> > bool cfg_bool;
> > int cfg_int;
> > unsigned long cfg_ulong;
> > u32 cfg_u32;
> > char *cfg_str;
> > } val;
> >
> > But just dealing with the "char *" as below would also be fine with me:
> >
> > union {
> > u64 period;
> > u64 freq;
> > bool time;
> > u64 stack_user;
> > int max_stack;
> > bool inherit;
> > bool overwrite;
> > unsigned long max_events;
> > bool percore;
> > bool aux_output;
> > u32 aux_sample_size;
> > u64 cfg_chg;
> > u64 num;
> > char *str;
> > } val;
> >
> > >
> > > struct perf_evsel_config_term {
> > > struct list_head list;
> > > enum evsel_term_type type;
> > > union {
> > > u64 period;
> > > u64 freq;
> > > bool time;
> > > char *callgraph;
> > > char *drv_cfg;
> > > u64 stack_user;
> > > int max_stack;
> > > bool inherit;
> > > bool overwrite;
> > > char *branch;
> > > unsigned long max_events;
> > > bool percore;
> > > bool aux_output;
> > > u32 aux_sample_size;
> > > u64 cfg_chg;
> > > + u64 num;
> > > + char *str;
> > > } val;
> > > bool weak;
> > > };
> > >
> > > > I will let Jiri make the
> > > > final call but if we are to proceed this way I think we should have a
> > > > member per type to avoid casting issues.
> > >
> > > Yeah, let's see what's Jiri thinking.
> > >
> > > Just note, with this change, I don't see any casting warning or errors
> > > when built perf on arm64/arm32.
> >
> > At this time you may not, but they will happen and it will be very hard to
> > debug.
>
> hi,
> sry for late reply..
>
> I think ;-) we should either add all different types to the union
> or just add 'str' pointer to handle strings correctly.. which seems
> better, because it's less changes and there's no real issue that
> would need that other bigger change
Thanks for the suggestion, Jiri.
Have sent out patch v5 with following the ideas.
Thanks,
Leo Yan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
@ 2020-01-13 15:21 ` Leo Yan
0 siblings, 0 replies; 18+ messages in thread
From: Leo Yan @ 2020-01-13 15:21 UTC (permalink / raw)
To: Jiri Olsa
Cc: Mark Rutland, Ian Rogers, Andi Kleen, Mathieu Poirier,
Suzuki K Poulose, Peter Zijlstra, Adrian Hunter,
Linux Kernel Mailing List, Arnaldo Carvalho de Melo,
Alexander Shishkin, Ingo Molnar, Jiri Olsa, Namhyung Kim,
Thomas Gleixner, linux-arm-kernel, Mike Leach
On Fri, Jan 10, 2020 at 04:04:10PM +0100, Jiri Olsa wrote:
> On Thu, Jan 09, 2020 at 09:34:24AM -0700, Mathieu Poirier wrote:
>
> SNIP
>
> >
> > If we are to deal with all flields of the union, I think it should be as below:
> >
> > union {
> > bool cfg_bool;
> > int cfg_int;
> > unsigned long cfg_ulong;
> > u32 cfg_u32;
> > char *cfg_str;
> > } val;
> >
> > But just dealing with the "char *" as below would also be fine with me:
> >
> > union {
> > u64 period;
> > u64 freq;
> > bool time;
> > u64 stack_user;
> > int max_stack;
> > bool inherit;
> > bool overwrite;
> > unsigned long max_events;
> > bool percore;
> > bool aux_output;
> > u32 aux_sample_size;
> > u64 cfg_chg;
> > u64 num;
> > char *str;
> > } val;
> >
> > >
> > > struct perf_evsel_config_term {
> > > struct list_head list;
> > > enum evsel_term_type type;
> > > union {
> > > u64 period;
> > > u64 freq;
> > > bool time;
> > > char *callgraph;
> > > char *drv_cfg;
> > > u64 stack_user;
> > > int max_stack;
> > > bool inherit;
> > > bool overwrite;
> > > char *branch;
> > > unsigned long max_events;
> > > bool percore;
> > > bool aux_output;
> > > u32 aux_sample_size;
> > > u64 cfg_chg;
> > > + u64 num;
> > > + char *str;
> > > } val;
> > > bool weak;
> > > };
> > >
> > > > I will let Jiri make the
> > > > final call but if we are to proceed this way I think we should have a
> > > > member per type to avoid casting issues.
> > >
> > > Yeah, let's see what's Jiri thinking.
> > >
> > > Just note, with this change, I don't see any casting warning or errors
> > > when built perf on arm64/arm32.
> >
> > At this time you may not, but they will happen and it will be very hard to
> > debug.
>
> hi,
> sry for late reply..
>
> I think ;-) we should either add all different types to the union
> or just add 'str' pointer to handle strings correctly.. which seems
> better, because it's less changes and there's no real issue that
> would need that other bigger change
Thanks for the suggestion, Jiri.
Have sent out patch v5 with following the ideas.
Thanks,
Leo Yan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
2020-01-10 15:04 ` Jiri Olsa
@ 2020-01-13 15:22 ` Leo Yan
-1 siblings, 0 replies; 18+ messages in thread
From: Leo Yan @ 2020-01-13 15:22 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Mathieu Poirier,
Suzuki K Poulose, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Namhyung Kim, Ian Rogers, Adrian Hunter,
Thomas Gleixner, Andi Kleen, linux-arm-kernel, linux-kernel,
Mike Leach
On Fri, Jan 10, 2020 at 04:04:22PM +0100, Jiri Olsa wrote:
> On Wed, Jan 08, 2020 at 10:20:09PM +0800, Leo Yan wrote:
> > The struct perf_evsel_config_term::val is a union which contains
> > multiple variables for corresponding types. This leads the union to
> > be complex and also causes complex code logic.
> >
> > This patch refactors the structure to use two general variables in the
> > 'val' union: one is 'num' for unsigned 64-bit integer and another is
> > 'str' for string variable. This can simplify the data structure and
> > the related code, this also can benefit for possibly extension.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>
> there's some arch code that needs to be changed.. please
> change other archs as well
>
>
> CC arch/x86/util/intel-pt.o
> arch/x86/util/intel-pt.c: In function ‘intel_pt_config_sample_mode’:
> arch/x86/util/intel-pt.c:563:24: error: ‘union <anonymous>’ has no member named ‘cfg_chg’
> 563 | user_bits = term->val.cfg_chg;
This compiling error will be dismissed in patch v5, since val.cfg_chg
is kept in the structure.
Thanks,
Leo
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term
@ 2020-01-13 15:22 ` Leo Yan
0 siblings, 0 replies; 18+ messages in thread
From: Leo Yan @ 2020-01-13 15:22 UTC (permalink / raw)
To: Jiri Olsa
Cc: Mark Rutland, Ian Rogers, Andi Kleen, Mathieu Poirier,
Suzuki K Poulose, Peter Zijlstra, Adrian Hunter, linux-kernel,
Arnaldo Carvalho de Melo, Alexander Shishkin, Ingo Molnar,
Jiri Olsa, Namhyung Kim, Thomas Gleixner, linux-arm-kernel,
Mike Leach
On Fri, Jan 10, 2020 at 04:04:22PM +0100, Jiri Olsa wrote:
> On Wed, Jan 08, 2020 at 10:20:09PM +0800, Leo Yan wrote:
> > The struct perf_evsel_config_term::val is a union which contains
> > multiple variables for corresponding types. This leads the union to
> > be complex and also causes complex code logic.
> >
> > This patch refactors the structure to use two general variables in the
> > 'val' union: one is 'num' for unsigned 64-bit integer and another is
> > 'str' for string variable. This can simplify the data structure and
> > the related code, this also can benefit for possibly extension.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>
> there's some arch code that needs to be changed.. please
> change other archs as well
>
>
> CC arch/x86/util/intel-pt.o
> arch/x86/util/intel-pt.c: In function ‘intel_pt_config_sample_mode’:
> arch/x86/util/intel-pt.c:563:24: error: ‘union <anonymous>’ has no member named ‘cfg_chg’
> 563 | user_bits = term->val.cfg_chg;
This compiling error will be dismissed in patch v5, since val.cfg_chg
is kept in the structure.
Thanks,
Leo
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-01-13 15:23 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-08 14:20 [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term Leo Yan
2020-01-08 14:20 ` Leo Yan
2020-01-08 14:20 ` [PATCH v4 2/2] perf parse: Copy string to perf_evsel_config_term Leo Yan
2020-01-08 14:20 ` Leo Yan
2020-01-08 17:58 ` [PATCH v4 1/2] perf parse: Refactor struct perf_evsel_config_term Mathieu Poirier
2020-01-08 17:58 ` Mathieu Poirier
2020-01-09 5:08 ` Leo Yan
2020-01-09 5:08 ` Leo Yan
2020-01-09 16:34 ` Mathieu Poirier
2020-01-09 16:34 ` Mathieu Poirier
2020-01-10 15:04 ` Jiri Olsa
2020-01-10 15:04 ` Jiri Olsa
2020-01-13 15:21 ` Leo Yan
2020-01-13 15:21 ` Leo Yan
2020-01-10 15:04 ` Jiri Olsa
2020-01-10 15:04 ` Jiri Olsa
2020-01-13 15:22 ` Leo Yan
2020-01-13 15:22 ` Leo Yan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.