Date:   Sun, 14 Nov 2021 18:02:04 +0100
From:   Jiri Olsa <jolsa@redhat.com>
To:     Ian Rogers <irogers@google.com>
Cc:     Andi Kleen <ak@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        John Garry <john.garry@huawei.com>,
        Kajol Jain <kjain@linux.ibm.com>,
        "Paul A . Clarke" <pc@us.ibm.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Riccardo Mancini <rickyman7@gmail.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
        eranian@google.com
Subject: Re: [PATCH 2/2] perf parse-events: Architecture specific leader
 override
Message-ID: <YZFBDI6oPMidX0KO@krava>
References: <20211113071548.372572-1-irogers@google.com>
 <20211113071548.372572-2-irogers@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211113071548.372572-2-irogers@google.com>
Precedence: bulk

On Fri, Nov 12, 2021 at 11:15:48PM -0800, Ian Rogers wrote:
> Currently topdown events must appear after a slots event:
> 
> $ perf stat -e '{slots,topdown-fe-bound}' /bin/true
> 
>  Performance counter stats for '/bin/true':
> 
>          3,183,090      slots
>            986,133      topdown-fe-bound
> 
> Reversing the events yields:
> 
> $ perf stat -e '{topdown-fe-bound,slots}' /bin/true
> Error:
> The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (topdown-fe-bound).

why is this actually failing?

> 
> For metrics the order of events is determined by iterating over a
> hashmap, and so slots isn't guaranteed to be first which can yield this
> error.
> 
> Change the set_leader in parse-events, called when a group is closed, so
> that rather than always making the first event the leader, if the slots
> event exists then it is made the leader. It is then moved to the head of
> the evlist otherwise it won't be opened in the correct order.
> 
> The result is:
> 
> $ perf stat -e '{topdown-fe-bound,slots}' /bin/true
> 
>  Performance counter stats for '/bin/true':
> 
>          3,274,795      slots
>          1,001,702      topdown-fe-bound
> 
> A problem with this approach is the slots event is identified by name,
> names can be overwritten like 'cpu/slots,name=foo/' and this causes the
> leader change to fail.

would it be better then to move this shuffle to the metric code directly,
and make sure it only spits 'good' order of events?

and keep "-e '{topdown-fe-bound,slots}'" to fail if user specifies that on
the command line

> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/arch/x86/util/evlist.c | 17 +++++++++++++++++
>  tools/perf/util/evlist.h          |  1 +
>  tools/perf/util/parse-events.c    | 16 +++++++++++-----
>  tools/perf/util/parse-events.h    |  4 ++--
>  tools/perf/util/parse-events.y    | 12 ++++++++----
>  5 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/evlist.c b/tools/perf/arch/x86/util/evlist.c
> index 0b0951030a2f..1624372b2da2 100644
> --- a/tools/perf/arch/x86/util/evlist.c
> +++ b/tools/perf/arch/x86/util/evlist.c
> @@ -17,3 +17,20 @@ int arch_evlist__add_default_attrs(struct evlist *evlist)
>  	else
>  		return parse_events(evlist, TOPDOWN_L1_EVENTS, NULL);
>  }
> +
> +struct evsel *arch_evlist__leader(struct list_head *list)
> +{
> +	struct evsel *evsel, *first;
> +
> +	first = list_entry(list->next, struct evsel, core.node);
> +
> +	if (!pmu_have_event("cpu", "slots"))
> +		return first;
> +
> +	__evlist__for_each_entry(list, evsel) {
> +		if (evsel->pmu_name && !strcmp(evsel->pmu_name, "cpu") &&
> +			evsel->name && strstr(evsel->name, "slots"))
> +			return evsel;
> +	}
> +	return first;
> +}
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index 97bfb8d0be4f..993437ffe429 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -110,6 +110,7 @@ int __evlist__add_default_attrs(struct evlist *evlist,
>  	__evlist__add_default_attrs(evlist, array, ARRAY_SIZE(array))
>  
>  int arch_evlist__add_default_attrs(struct evlist *evlist);
> +struct evsel *arch_evlist__leader(struct list_head *list);
>  
>  int evlist__add_dummy(struct evlist *evlist);
>  
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 6308ba739d19..a2f4c086986f 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -1821,22 +1821,28 @@ parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list,
>  	return ret;
>  }
>  
> -void parse_events__set_leader(char *name, struct list_head *list,
> -			      struct parse_events_state *parse_state)
> +__weak struct evsel *arch_evlist__leader(struct list_head *list)
> +{
> +	return list_entry(list->next, struct evsel, core.node);
> +}
> +
> +struct list_head *parse_events__set_leader(char *name, struct list_head *list,
> +					struct parse_events_state *parse_state)
>  {
>  	struct evsel *leader;
>  
>  	if (list_empty(list)) {
>  		WARN_ONCE(true, "WARNING: failed to set leader: empty list");
> -		return;
> +		return NULL;
>  	}
>  
>  	if (parse_events__set_leader_for_uncore_aliase(name, list, parse_state))
> -		return;
> +		return NULL;
>  
> -	leader = list_entry(list->next, struct evsel, core.node);
> +	leader = arch_evlist__leader(list);
>  	__perf_evlist__set_leader(list, &leader->core);
>  	leader->group_name = name ? strdup(name) : NULL;
> +	return &leader->core.node;
>  }
>  
>  /* list_event is assumed to point to malloc'ed memory */
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index c7fc93f54577..8a6e6b4d5cbe 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -211,8 +211,8 @@ int parse_events_copy_term_list(struct list_head *old,
>  
>  enum perf_pmu_event_symbol_type
>  perf_pmu__parse_check(const char *name);
> -void parse_events__set_leader(char *name, struct list_head *list,
> -			      struct parse_events_state *parse_state);
> +struct list_head *parse_events__set_leader(char *name, struct list_head *list,
> +					struct parse_events_state *parse_state);
>  void parse_events_update_lists(struct list_head *list_event,
>  			       struct list_head *list_all);
>  void parse_events_evlist_error(struct parse_events_state *parse_state,
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 174158982fae..5358c400f938 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -199,20 +199,24 @@ group_def
>  group_def:
>  PE_NAME '{' events '}'
>  {
> -	struct list_head *list = $3;
> +	struct list_head *new_head, *list = $3;
>  
>  	inc_group_count(list, _parse_state);
> -	parse_events__set_leader($1, list, _parse_state);
> +	new_head = parse_events__set_leader($1, list, _parse_state);
> +	if (new_head)
> +		list_move(new_head, list);

why not put these last 2 lines to parse_events__set_leader as well?

>  	free($1);
>  	$$ = list;
>  }
>  |
>  '{' events '}'
>  {
> -	struct list_head *list = $2;
> +	struct list_head *new_head, *list = $2;
>  
>  	inc_group_count(list, _parse_state);
> -	parse_events__set_leader(NULL, list, _parse_state);
> +	new_head = parse_events__set_leader(NULL, list, _parse_state);
> +	if (new_head)
> +		list_move(new_head, list);

same

thanks,
jirka