| Message ID | 1616668398-144648-3-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf arm64 metricgroup support | expand |
On Thu, Mar 25, 2021 at 06:33:14PM +0800, John Garry wrote: SNIP > +struct metric { > + struct list_head list; > + struct metric_ref metric_ref; > +}; > + > +static int resolve_metric_simple(struct expr_parse_ctx *pctx, > + struct list_head *compound_list, > + struct pmu_events_map *map, > + const char *metric_name) > +{ > + struct hashmap_entry *cur, *cur_tmp; > + struct metric *metric, *tmp; > + size_t bkt; > + bool all; > + int rc; > + > + do { > + all = true; > + hashmap__for_each_entry_safe((&pctx->ids), cur, cur_tmp, bkt) { > + struct metric_ref *ref; > + struct pmu_event *pe; > + > + pe = metrcgroup_find_metric(cur->key, map); > + if (!pe) > + continue; > + > + if (!strcmp(metric_name, (char *)cur->key)) { > + pr_warning("Recursion detected for metric %s\n", metric_name); > + rc = -1; > + goto out_err; > + } > + > + all = false; > + > + /* The metric key itself needs to go out.. */ > + expr__del_id(pctx, cur->key); > + > + metric = malloc(sizeof(*metric)); > + if (!metric) { > + rc = -ENOMEM; > + goto out_err; > + } > + > + ref = &metric->metric_ref; > + ref->metric_name = pe->metric_name; > + ref->metric_expr = pe->metric_expr; > + list_add_tail(&metric->list, compound_list); > + > + rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); so this might add new items to pctx->ids, I think you need to restart the iteration as we do it in __resolve_metric otherwise you could miss some new keys jirka > + if (rc) > + goto out_err; > + } > + } while (!all); > + > + return 0; > + > +out_err: > + list_for_each_entry_safe(metric, tmp, compound_list, list) > + free(metric); > + > + return rc; > + > +} SNIP
On 01/04/2021 14:49, Jiri Olsa wrote: > On Thu, Mar 25, 2021 at 06:33:14PM +0800, John Garry wrote: > > SNIP > >> +struct metric { >> + struct list_head list; >> + struct metric_ref metric_ref; >> +}; >> + >> +static int resolve_metric_simple(struct expr_parse_ctx *pctx, >> + struct list_head *compound_list, >> + struct pmu_events_map *map, >> + const char *metric_name) >> +{ >> + struct hashmap_entry *cur, *cur_tmp; >> + struct metric *metric, *tmp; >> + size_t bkt; >> + bool all; >> + int rc; >> + >> + do { >> + all = true; >> + hashmap__for_each_entry_safe((&pctx->ids), cur, cur_tmp, bkt) { >> + struct metric_ref *ref; >> + struct pmu_event *pe; >> + >> + pe = metrcgroup_find_metric(cur->key, map); * >> + if (!pe) >> + continue; >> + >> + if (!strcmp(metric_name, (char *)cur->key)) { >> + pr_warning("Recursion detected for metric %s\n", metric_name); >> + rc = -1; >> + goto out_err; >> + } >> + >> + all = false; >> + >> + /* The metric key itself needs to go out.. */ >> + expr__del_id(pctx, cur->key); >> + >> + metric = malloc(sizeof(*metric)); >> + if (!metric) { >> + rc = -ENOMEM; >> + goto out_err; >> + } >> + >> + ref = &metric->metric_ref; >> + ref->metric_name = pe->metric_name; >> + ref->metric_expr = pe->metric_expr; >> + list_add_tail(&metric->list, compound_list); >> + >> + rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); > Hi Jirka, > so this might add new items to pctx->ids, I think you need > to restart the iteration as we do it in __resolve_metric > otherwise you could miss some new keys I thought that I was doing this. Indeed, this code is very much like __resolve_metric() ;) So expr__find_other() may add a new item to pctx->ids, and we always iterate again, and try to lookup any pmu_events, *, above. If none exist, then we have broken down pctx into primitive events aliases and unresolvable metrics, and stop iterating. And then unresolvable metrics would be found in check_parse_cpu(). As an example, we can deal with metric test1, below, which references 2x other metrics: { "MetricExpr": "IDQ_UOPS_NOT_DELIVERED.CORE / (4 * (( ( CPU_CLK_UNHALTED.THREAD / 2 ) * ( 1 + CPU_CLK_UNHALTED.ONE_THREAD_ACTIVE / CPU_CLK_UNHALTED.REF_XCLK ) )))", "MetricName": "Frontend_Bound", }, { "MetricExpr": "( UOPS_ISSUED.ANY - UOPS_RETIRED.RETIRE_SLOTS + 4 * INT_MISC.RECOVERY_CYCLES ) / (4 * cycles)", "MetricName": "Bad_Speculation", }, { "MetricExpr": "Bad_Speculation + Frontend_Bound", "MetricName": "test1", }, Does that satisfy your concern, or have I missed something? Thanks, John > > jirka > >> + if (rc) >> + goto out_err; >> + } >> + } while (!all); >> + >> + return 0; >> + >> +out_err: >> + list_for_each_entry_safe(metric, tmp, compound_list, list) >> + free(metric); >> + >> + return rc; >> + >> +} > > SNIP > > . >
On Tue, Apr 06, 2021 at 12:00:27PM +0100, John Garry wrote: > On 01/04/2021 14:49, Jiri Olsa wrote: > > On Thu, Mar 25, 2021 at 06:33:14PM +0800, John Garry wrote: > > > > SNIP > > > > > +struct metric { > > > + struct list_head list; > > > + struct metric_ref metric_ref; > > > +}; > > > + > > > +static int resolve_metric_simple(struct expr_parse_ctx *pctx, > > > + struct list_head *compound_list, > > > + struct pmu_events_map *map, > > > + const char *metric_name) > > > +{ > > > + struct hashmap_entry *cur, *cur_tmp; > > > + struct metric *metric, *tmp; > > > + size_t bkt; > > > + bool all; > > > + int rc; > > > + > > > + do { > > > + all = true; > > > + hashmap__for_each_entry_safe((&pctx->ids), cur, cur_tmp, bkt) { > > > + struct metric_ref *ref; > > > + struct pmu_event *pe; > > > + > > > + pe = metrcgroup_find_metric(cur->key, map); > > * > > > > + if (!pe) > > > + continue; > > > + > > > + if (!strcmp(metric_name, (char *)cur->key)) { > > > + pr_warning("Recursion detected for metric %s\n", metric_name); > > > + rc = -1; > > > + goto out_err; > > > + } > > > + > > > + all = false; > > > + > > > + /* The metric key itself needs to go out.. */ > > > + expr__del_id(pctx, cur->key); > > > + > > > + metric = malloc(sizeof(*metric)); > > > + if (!metric) { > > > + rc = -ENOMEM; > > > + goto out_err; > > > + } > > > + > > > + ref = &metric->metric_ref; > > > + ref->metric_name = pe->metric_name; > > > + ref->metric_expr = pe->metric_expr; > > > + list_add_tail(&metric->list, compound_list); > > > + > > > + rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); > > > > Hi Jirka, > > > so this might add new items to pctx->ids, I think you need > > to restart the iteration as we do it in __resolve_metric > > otherwise you could miss some new keys > > I thought that I was doing this. Indeed, this code is very much like > __resolve_metric() ;) > > So expr__find_other() may add a new item to pctx->ids, and we always iterate > again, and try to lookup any pmu_events, *, above. If none exist, then we hm, I don't see that.. so, what you do is: hashmap__for_each_entry_safe((&pctx->ids) ....) { rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); } and what I think we need to do is: hashmap__for_each_entry_safe((&pctx->ids) ....) { rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); break; } each time you resolve another metric, you need to restart the pctx->ids iteration, because there will be new items, and we are in the middle of it jirka > have broken down pctx into primitive events aliases and unresolvable > metrics, and stop iterating. And then unresolvable metrics would be found in > check_parse_cpu(). > > As an example, we can deal with metric test1, below, which references 2x > other metrics: > > { > "MetricExpr": "IDQ_UOPS_NOT_DELIVERED.CORE / (4 * (( ( > CPU_CLK_UNHALTED.THREAD / 2 ) * ( 1 + CPU_CLK_UNHALTED.ONE_THREAD_ACTIVE / > CPU_CLK_UNHALTED.REF_XCLK ) )))", > "MetricName": "Frontend_Bound", > }, > { > "MetricExpr": "( UOPS_ISSUED.ANY - UOPS_RETIRED.RETIRE_SLOTS + 4 * > INT_MISC.RECOVERY_CYCLES ) / (4 * cycles)", > "MetricName": "Bad_Speculation", > }, > { > "MetricExpr": "Bad_Speculation + Frontend_Bound", > "MetricName": "test1", > }, > > Does that satisfy your concern, or have I missed something? > > Thanks, > John > > > > > jirka > > > > > + if (rc) > > > + goto out_err; > > > + } > > > + } while (!all); > > > + > > > + return 0; > > > + > > > +out_err: > > > + list_for_each_entry_safe(metric, tmp, compound_list, list) > > > + free(metric); > > > + > > > + return rc; > > > + > > > +} > > > > SNIP > > > > . > > >
On 06/04/2021 13:17, Jiri Olsa wrote: >>>> + ref = &metric->metric_ref; >>>> + ref->metric_name = pe->metric_name; >>>> + ref->metric_expr = pe->metric_expr; >>>> + list_add_tail(&metric->list, compound_list); >>>> + >>>> + rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); >> Hi Jirka, >> >>> so this might add new items to pctx->ids, I think you need >>> to restart the iteration as we do it in __resolve_metric >>> otherwise you could miss some new keys >> I thought that I was doing this. Indeed, this code is very much like >> __resolve_metric();) >> >> So expr__find_other() may add a new item to pctx->ids, and we always iterate >> again, and try to lookup any pmu_events, *, above. If none exist, then we > hm, I don't see that.. so, what you do is: > > hashmap__for_each_entry_safe((&pctx->ids) ....) { > > rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); > } > > and what I think we need to do is: > > hashmap__for_each_entry_safe((&pctx->ids) ....) { > > rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); > > break; > } > > each time you resolve another metric, you need to restart > the pctx->ids iteration, because there will be new items, > and we are in the middle of it Sure, but we will restart anyway. Regardless of this, I don't think what I am doing is safe, i.e. adding new items in the middle of the iter, so I will change in the way you suggest. Thanks, John
On Tue, Apr 06, 2021 at 01:43:09PM +0100, John Garry wrote: > On 06/04/2021 13:17, Jiri Olsa wrote: > > > > > + ref = &metric->metric_ref; > > > > > + ref->metric_name = pe->metric_name; > > > > > + ref->metric_expr = pe->metric_expr; > > > > > + list_add_tail(&metric->list, compound_list); > > > > > + > > > > > + rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); > > > Hi Jirka, > > > > > > > so this might add new items to pctx->ids, I think you need > > > > to restart the iteration as we do it in __resolve_metric > > > > otherwise you could miss some new keys > > > I thought that I was doing this. Indeed, this code is very much like > > > __resolve_metric();) > > > > > > So expr__find_other() may add a new item to pctx->ids, and we always iterate > > > again, and try to lookup any pmu_events, *, above. If none exist, then we > > hm, I don't see that.. so, what you do is: > > > > hashmap__for_each_entry_safe((&pctx->ids) ....) { > > > > rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); > > } > > > > and what I think we need to do is: > > > > hashmap__for_each_entry_safe((&pctx->ids) ....) { > > > > rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); > > > > break; > > } > > > > each time you resolve another metric, you need to restart > > the pctx->ids iteration, because there will be new items, > > and we are in the middle of it > > Sure, but we will restart anyway. hum, where? you call expr__find_other and continue to next pctx->ids item > > Regardless of this, I don't think what I am doing is safe, i.e. adding new > items in the middle of the iter, so I will change in the way you suggest. it'll always add items in the middle of the iteration jirka > > Thanks, > John >
On 06/04/2021 13:55, Jiri Olsa wrote: >>>> So expr__find_other() may add a new item to pctx->ids, and we always iterate >>>> again, and try to lookup any pmu_events, *, above. If none exist, then we >>> hm, I don't see that.. so, what you do is: >>> >>> hashmap__for_each_entry_safe((&pctx->ids) ....) { >>> >>> rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); >>> } >>> >>> and what I think we need to do is: >>> >>> hashmap__for_each_entry_safe((&pctx->ids) ....) { >>> >>> rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); >>> >>> break; >>> } >>> >>> each time you resolve another metric, you need to restart >>> the pctx->ids iteration, because there will be new items, >>> and we are in the middle of it >> Sure, but we will restart anyway. > hum, where? you call expr__find_other and continue to next > pctx->ids item We have: resolve_metric_simple() { bool all; do { all = true; hashmap__for_each_entry_safe(&pctx->ids, ...) { pe = metricgroup_find_metric(cur->key, map); if (!pe) continue; ... all = false; expr_del_id(pctx, cur->key); ... rc = expr__find_other(pe->metric_expr, pctx); if (rc) goto out_err; } } while (!all); } So once we evaluate a pmu_event in pctx->ids in @pe, @all is set false, and we would loop again in the do-while loop, regardless of what expr__find_other() does (apart from erroring), and so call hashmap__for_each_entry_safe(&pctx->ids, ) again. This is really what is done in __resolve_metric() - indeed, I would use that function directly, but it looks hard to extract that from metricgroup.c . Thanks, John > >> Regardless of this, I don't think what I am doing is safe, i.e. adding new >> items in the middle of the iter, so I will change in the way you suggest. > it'll always add items in the middle of the iteration
On Tue, Apr 06, 2021 at 02:21:11PM +0100, John Garry wrote: > On 06/04/2021 13:55, Jiri Olsa wrote: > > > > > So expr__find_other() may add a new item to pctx->ids, and we always iterate > > > > > again, and try to lookup any pmu_events, *, above. If none exist, then we > > > > hm, I don't see that.. so, what you do is: > > > > > > > > hashmap__for_each_entry_safe((&pctx->ids) ....) { > > > > > > > > rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); > > > > } > > > > > > > > and what I think we need to do is: > > > > > > > > hashmap__for_each_entry_safe((&pctx->ids) ....) { > > > > > > > > rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); > > > > > > > > break; > > > > } > > > > > > > > each time you resolve another metric, you need to restart > > > > the pctx->ids iteration, because there will be new items, > > > > and we are in the middle of it > > > Sure, but we will restart anyway. > > hum, where? you call expr__find_other and continue to next > > pctx->ids item > > We have: > > resolve_metric_simple() > { > bool all; > > do { > all = true; > > hashmap__for_each_entry_safe(&pctx->ids, ...) { > > pe = metricgroup_find_metric(cur->key, map); > if (!pe) > continue; > > ... > all = false; > > expr_del_id(pctx, cur->key); > > ... > rc = expr__find_other(pe->metric_expr, pctx); > if (rc) > goto out_err; > } > > } while (!all); > > } > > So once we evaluate a pmu_event in pctx->ids in @pe, @all is set false, and > we would loop again in the do-while loop, regardless of what > expr__find_other() does (apart from erroring), and so call > hashmap__for_each_entry_safe(&pctx->ids, ) again. ah ok, so it finishes the hash iteration first and then restarts it.. ok, I missed that, then it's fine > > This is really what is done in __resolve_metric() - indeed, I would use that > function directly, but it looks hard to extract that from metricgroup.c . yea, it's another world ;-) it's better to keep it separated thanks, jirka > > Thanks, > John > > > > > > Regardless of this, I don't think what I am doing is safe, i.e. adding new > > > items in the middle of the iter, so I will change in the way you suggest. > > it'll always add items in the middle of the iteration >
On 06/04/2021 14:34, Jiri Olsa wrote: >> >> } >> >> So once we evaluate a pmu_event in pctx->ids in @pe, @all is set false, and >> we would loop again in the do-while loop, regardless of what >> expr__find_other() does (apart from erroring), and so call >> hashmap__for_each_entry_safe(&pctx->ids, ) again. > ah ok, so it finishes the hash iteration first and > then restarts it.. ok, I missed that, then it's fine > >> This is really what is done in __resolve_metric() - indeed, I would use that >> function directly, but it looks hard to extract that from metricgroup.c . > yea, it's another world;-) it's better to keep it separated ok, so I'll still add the break statement, as suggested. I'll also wait to see if Ian or you have a strong feeling about the function naming in patch 1/6. Thanks, John
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c index 0ca6a5a53523..20b6bf14f7f7 100644 --- a/tools/perf/tests/pmu-events.c +++ b/tools/perf/tests/pmu-events.c @@ -12,6 +12,7 @@ #include "util/evlist.h" #include "util/expr.h" #include "util/parse-events.h" +#include "metricgroup.h" struct perf_pmu_test_event { /* used for matching against events from generated pmu-events.c */ @@ -471,6 +472,70 @@ static void expr_failure(const char *msg, pr_debug("On expression %s\n", pe->metric_expr); } +struct metric { + struct list_head list; + struct metric_ref metric_ref; +}; + +static int resolve_metric_simple(struct expr_parse_ctx *pctx, + struct list_head *compound_list, + struct pmu_events_map *map, + const char *metric_name) +{ + struct hashmap_entry *cur, *cur_tmp; + struct metric *metric, *tmp; + size_t bkt; + bool all; + int rc; + + do { + all = true; + hashmap__for_each_entry_safe((&pctx->ids), cur, cur_tmp, bkt) { + struct metric_ref *ref; + struct pmu_event *pe; + + pe = metrcgroup_find_metric(cur->key, map); + if (!pe) + continue; + + if (!strcmp(metric_name, (char *)cur->key)) { + pr_warning("Recursion detected for metric %s\n", metric_name); + rc = -1; + goto out_err; + } + + all = false; + + /* The metric key itself needs to go out.. */ + expr__del_id(pctx, cur->key); + + metric = malloc(sizeof(*metric)); + if (!metric) { + rc = -ENOMEM; + goto out_err; + } + + ref = &metric->metric_ref; + ref->metric_name = pe->metric_name; + ref->metric_expr = pe->metric_expr; + list_add_tail(&metric->list, compound_list); + + rc = expr__find_other(pe->metric_expr, NULL, pctx, 0); + if (rc) + goto out_err; + } + } while (!all); + + return 0; + +out_err: + list_for_each_entry_safe(metric, tmp, compound_list, list) + free(metric); + + return rc; + +} + static int test_parsing(void) { struct pmu_events_map *cpus_map = perf_pmu__find_map(NULL); @@ -488,7 +553,9 @@ static int test_parsing(void) break; j = 0; for (;;) { + struct metric *metric, *tmp; struct hashmap_entry *cur; + LIST_HEAD(compound_list); size_t bkt; pe = &map->table[j++]; @@ -504,6 +571,13 @@ static int test_parsing(void) continue; } + if (resolve_metric_simple(&ctx, &compound_list, map, + pe->metric_name)) { + expr_failure("Could not resolve metrics", map, pe); + ret++; + goto exit; /* Don't tolerate errors due to severity */ + } + /* * Add all ids with a made up value. The value may * trigger divide by zero when subtracted and so try to @@ -519,6 +593,11 @@ static int test_parsing(void) ret++; } + list_for_each_entry_safe(metric, tmp, &compound_list, list) { + expr__add_ref(&ctx, &metric->metric_ref); + free(metric); + } + if (expr__parse(&result, &ctx, pe->metric_expr, 0)) { expr_failure("Parse failed", map, pe); ret++; @@ -527,6 +606,7 @@ static int test_parsing(void) } } /* TODO: fail when not ok */ +exit: return ret == 0 ? TEST_OK : TEST_SKIP; }
The pmu-events parsing test does not handle metric reuse at all. Introduce some simple handling to resolve metrics who reference other metrics. Signed-off-by: John Garry <john.garry@huawei.com> --- tools/perf/tests/pmu-events.c | 80 +++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+)