Re: [PATCH v3 12/27] perf parse-events: Support no alias assigned event inside hybrid PMU

[Jiri Olsa]

On Mon, Mar 29, 2021 at 03:00:31PM +0800, Jin Yao wrote:

SNIP

> ---
> v3:
>  - Rename the patch:
>    'perf parse-events: Support hardware events inside PMU' -->
>    'perf parse-events: Support no alias assigned event inside hybrid PMU'
> 
>  - Major code is moved to parse-events-hybrid.c.
>  - Refine the code.
> 
>  tools/perf/util/parse-events-hybrid.c | 18 +++++-
>  tools/perf/util/parse-events-hybrid.h |  3 +-
>  tools/perf/util/parse-events.c        | 80 +++++++++++++++++++++++++--
>  tools/perf/util/parse-events.h        |  4 +-
>  tools/perf/util/parse-events.y        |  9 ++-
>  tools/perf/util/pmu.c                 |  4 +-
>  tools/perf/util/pmu.h                 |  2 +-
>  7 files changed, 108 insertions(+), 12 deletions(-)

please move the support to pass pmu_name and filter
on it within hybrid code in to separate patch

> 
> diff --git a/tools/perf/util/parse-events-hybrid.c b/tools/perf/util/parse-events-hybrid.c
> index 8a630cbab8f3..5bf176b55573 100644
> --- a/tools/perf/util/parse-events-hybrid.c
> +++ b/tools/perf/util/parse-events-hybrid.c
> @@ -64,6 +64,11 @@ static int add_hw_hybrid(struct parse_events_state *parse_state,
>  	int ret;
>  
>  	perf_pmu__for_each_hybrid_pmu(pmu) {
> +		if (parse_state->pmu_name &&
> +		    strcmp(parse_state->pmu_name, pmu->name)) {
> +			continue;

please add this check to separate function

	if (pmu_cmp(parse_stat))
		continue;

SNIP

> +	if (!parse_state->fake_pmu && head_config && !found &&
> +	    perf_pmu__is_hybrid(name)) {
> +		struct parse_events_term *term;
> +		int ret;
> +
> +		list_for_each_entry(term, head_config, list) {
> +			if (!term->config)
> +				continue;
> +
> +			ret = parse_events__with_hybrid_pmu(parse_state,
> +							    term->config,
> +							    name, &found,
> +							    list);
> +			if (found)
> +				return ret;

what if there are more terms in head_config?
should we make sure there's just one term and fail if there's more?

also we already know the perf_pmu__is_hybrid(name) is true,
so can't we just call:

  return parse_events__with_hybrid_pmu(....)


> +		}
> +	}
>  
>  	if (verbose > 1) {
>  		fprintf(stderr, "After aliases, add event pmu '%s' with '",
> @@ -1605,6 +1630,15 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>  	struct perf_pmu *pmu = NULL;
>  	int ok = 0;
>  
> +	if (parse_state->pmu_name) {
> +		list = malloc(sizeof(struct list_head));
> +		if (!list)
> +			return -1;
> +		INIT_LIST_HEAD(list);
> +		*listp = list;
> +		return 0;
> +	}

hum, why is this needed?

> +
>  	*listp = NULL;
>  	/* Add it for all PMUs that support the alias */
>  	list = malloc(sizeof(struct list_head));
> @@ -2176,6 +2210,44 @@ int parse_events_terms(struct list_head *terms, const char *str)
>  	return ret;
>  }
>  
> +static int list_entries_nr(struct list_head *list)
> +{
> +	struct list_head *pos;
> +	int n = 0;
> +
> +	list_for_each(pos, list)
> +		n++;
> +
> +	return n;
> +}
> +
> +static int parse_events__with_hybrid_pmu(struct parse_events_state *parse_state,
> +					 const char *str, char *pmu_name,
> +					 bool *found, struct list_head *list)
> +{
> +	struct parse_events_state ps = {
> +		.list           = LIST_HEAD_INIT(ps.list),
> +		.stoken         = PE_START_EVENTS,
> +		.pmu_name       = pmu_name,
> +		.idx            = parse_state->idx,
> +	};

could we add this pmu_name directly to __parse_events?

it duplicates the code plus there are some extra checks
you don't do in here and which might be needed, like
last->cmdline_group_boundary setup

> +	int ret;
> +
> +	*found = false;
> +	ret = parse_events__scanner(str, &ps);
> +	perf_pmu__parse_cleanup();
> +
> +	if (!ret) {
> +		if (!list_empty(&ps.list)) {
> +			*found = true;
> +			list_splice(&ps.list, list);
> +			parse_state->idx = list_entries_nr(list);

could you just use ps.idx instead of list_entries_nr ?

> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  int __parse_events(struct evlist *evlist, const char *str,
>  		   struct parse_events_error *err, struct perf_pmu *fake_pmu)
>  {
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index c4f2f96304ce..f9d8e8e41c38 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -138,6 +138,7 @@ struct parse_events_state {
>  	struct list_head	  *terms;
>  	int			   stoken;
>  	struct perf_pmu		  *fake_pmu;
> +	char			  *pmu_name;

so it's hybrid specific, we should name it like hybrid_pmu_name or such

thanks,
jirka