Re: [PATCH v1 2/3] perf parse-events: Make common term list to strbuf helper

[Liang, Kan]

On 2023-08-30 3:07 a.m., Ian Rogers wrote:
> A term list is turned into a string for debug output and for the str
> value in the alias. Add a helper to do this based on existing code,
> but then fix for situations like events being identified. Use strbuf
> to manage the dynamic memory allocation and remove the 256 byte
> limit. Use in various places the string of the term list is required.
> 
> Before:
> ```
> $ sudo perf stat -vv -e inst_retired.any true
> Using CPUID GenuineIntel-6-8D-1
> intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
> Attempting to add event pmu 'cpu' with 'inst_retired.any,' that may result in non-fatal errors
> After aliases, add event pmu 'cpu' with 'event,period,' that may result in non-fatal errors
> inst_retired.any -> cpu/inst_retired.any/
> ...
> ```
> 
> After:
> ```
> $ sudo perf stat -vv -e inst_retired.any true
> Using CPUID GenuineIntel-6-8D-1
> intel_pt default config: tsc,mtc,mtc_period=3,psb_period=3,pt,branch
> Attempt to add: cpu/inst_retired.any/
> ..after resolving event: cpu/event=0xc0,period=0x1e8483/
> inst_retired.any -> cpu/event=0xc0,period=0x1e8483/
> ...
> ```
> 
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> ---
>  tools/perf/util/parse-events.c | 101 ++++++++++++++++++++++++---------
>  tools/perf/util/parse-events.h |   2 +
>  tools/perf/util/pmu.c          |  19 ++-----
>  3 files changed, 81 insertions(+), 41 deletions(-)

Which branch should I use to apply the patch?
I tried the perf-tools-next branch, but there is some conflict.
I'd like to do some tests on a hybrid machine.

Thanks,
Kan
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 4c812fbe0cf9..0b941b58bdc0 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -13,7 +13,7 @@
>  #include <subcmd/parse-options.h>
>  #include "parse-events.h"
>  #include "string2.h"
> -#include "strlist.h"
> +#include "strbuf.h"
>  #include "debug.h"
>  #include <api/fs/tracing_path.h>
>  #include <perf/cpumap.h>
> @@ -1303,19 +1303,6 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>  
>  	pmu = parse_state->fake_pmu ?: perf_pmus__find(name);
>  
> -	if (verbose > 1 && !(pmu && pmu->selectable)) {
> -		fprintf(stderr, "Attempting to add event pmu '%s' with '",
> -			name);
> -		if (head_config) {
> -			struct parse_events_term *term;
> -
> -			list_for_each_entry(term, head_config, list) {
> -				fprintf(stderr, "%s,", term->config);
> -			}
> -		}
> -		fprintf(stderr, "' that may result in non-fatal errors\n");
> -	}
> -
>  	if (!pmu) {
>  		char *err_str;
>  
> @@ -1325,6 +1312,21 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>  			parse_events_error__handle(err, loc->first_column, err_str, NULL);
>  		return -EINVAL;
>  	}
> +
> +	if (verbose > 1) {
> +		struct strbuf sb;
> +
> +		strbuf_init(&sb, /*hint=*/ 0);
> +		if (pmu->selectable && !head_config) {
> +			strbuf_addf(&sb, "%s//", name);
> +		} else {
> +			strbuf_addf(&sb, "%s/", name);
> +			parse_events_term__to_strbuf(head_config, &sb);
> +			strbuf_addch(&sb, '/');
> +		}
> +		fprintf(stderr, "Attempt to add: %s\n", sb.buf);
> +		strbuf_release(&sb);
> +	}
>  	if (head_config)
>  		fix_raw(head_config, pmu);
>  
> @@ -1349,16 +1351,12 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
>  		return -EINVAL;
>  
>  	if (verbose > 1) {
> -		fprintf(stderr, "After aliases, add event pmu '%s' with '",
> -			name);
> -		if (head_config) {
> -			struct parse_events_term *term;
> +		struct strbuf sb;
>  
> -			list_for_each_entry(term, head_config, list) {
> -				fprintf(stderr, "%s,", term->config);
> -			}
> -		}
> -		fprintf(stderr, "' that may result in non-fatal errors\n");
> +		strbuf_init(&sb, /*hint=*/ 0);
> +		parse_events_term__to_strbuf(head_config, &sb);
> +		fprintf(stderr, "..after resolving event: %s/%s/\n", name, sb.buf);
> +		strbuf_release(&sb);
>  	}
>  
>  	/*
> @@ -1460,7 +1458,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>  		parse_events_copy_term_list(head, &orig_head);
>  		if (!parse_events_add_pmu(parse_state, list, pmu->name,
>  					  orig_head, auto_merge_stats, loc)) {
> -			pr_debug("%s -> %s/%s/\n", str, pmu->name, str);
> +			struct strbuf sb;
> +
> +			strbuf_init(&sb, /*hint=*/ 0);
> +			parse_events_term__to_strbuf(orig_head, &sb);
> +			pr_debug("%s -> %s/%s/\n", str, pmu->name, sb.buf);
> +			strbuf_release(&sb);
>  			ok++;
>  		}
>  		parse_events_terms__delete(orig_head);
> @@ -1469,7 +1472,12 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
>  	if (parse_state->fake_pmu) {
>  		if (!parse_events_add_pmu(parse_state, list, str, head,
>  					  /*auto_merge_stats=*/true, loc)) {
> -			pr_debug("%s -> %s/%s/\n", str, "fake_pmu", str);
> +			struct strbuf sb;
> +
> +			strbuf_init(&sb, /*hint=*/ 0);
> +			parse_events_term__to_strbuf(head, &sb);
> +			pr_debug("%s -> %s/%s/\n", str, "fake_pmu", sb.buf);
> +			strbuf_release(&sb);
>  			ok++;
>  		}
>  	}
> @@ -2085,7 +2093,7 @@ void parse_events_error__handle(struct parse_events_error *err, int idx,
>  		break;
>  	default:
>  		pr_debug("Multiple errors dropping message: %s (%s)\n",
> -			err->str, err->help);
> +			err->str, err->help ?: "<no help>");
>  		free(err->str);
>  		err->str = str;
>  		free(err->help);
> @@ -2502,6 +2510,47 @@ void parse_events_terms__delete(struct list_head *terms)
>  	free(terms);
>  }
>  
> +int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
> +{
> +	struct parse_events_term *term;
> +	bool first = true;
> +
> +	if (!term_list)
> +		return 0;
> +
> +	list_for_each_entry(term, term_list, list) {
> +		int ret;
> +
> +		if (!first) {
> +			ret = strbuf_addch(sb, ',');
> +			if (ret < 0)
> +				return ret;
> +		}
> +		first = false;
> +
> +		if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> +			if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER && term->val.num == 1)
> +				ret = strbuf_addf(sb, "%s", term->config);
> +			else
> +				ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
> +		else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) {
> +			if (term->config) {
> +				ret = strbuf_addf(sb, "%s=", term->config);
> +				if (ret < 0)
> +					return ret;
> +			} else if (term->type_term < __PARSE_EVENTS__TERM_TYPE_NR) {
> +				ret = strbuf_addf(sb, "%s=", config_term_names[term->type_term]);
> +				if (ret < 0)
> +					return ret;
> +			}
> +			ret = strbuf_addf(sb, "%s", term->val.str);
> +		}
> +		if (ret < 0)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
>  void parse_events_evlist_error(struct parse_events_state *parse_state,
>  			       int idx, const char *str)
>  {
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 6d75d853ce00..20bdc35d6112 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -18,6 +18,7 @@ struct parse_events_error;
>  
>  struct option;
>  struct perf_pmu;
> +struct strbuf;
>  
>  const char *event_type(int type);
>  
> @@ -152,6 +153,7 @@ int parse_events_term__clone(struct parse_events_term **new,
>  void parse_events_term__delete(struct parse_events_term *term);
>  void parse_events_terms__delete(struct list_head *terms);
>  void parse_events_terms__purge(struct list_head *terms);
> +int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb);
>  int parse_events__modifier_event(struct list_head *list, char *str, bool add);
>  int parse_events__modifier_group(struct list_head *list, char *event_mod);
>  int parse_events_name(struct list_head *list, const char *name);
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index b3f8f3f1e900..8dbb7008877e 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -507,12 +507,11 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
>  				const char *desc, const char *val, FILE *val_fd,
>  				const struct pmu_event *pe)
>  {
> -	struct parse_events_term *term;
>  	struct perf_pmu_alias *alias;
>  	int ret;
> -	char newval[256];
>  	const char *long_desc = NULL, *topic = NULL, *unit = NULL, *pmu_name = NULL;
>  	bool deprecated = false, perpkg = false;
> +	struct strbuf sb;
>  
>  	if (perf_pmu__find_alias(pmu, name, /*load=*/ false)) {
>  		/* Alias was already created/loaded. */
> @@ -582,20 +581,10 @@ static int perf_pmu__new_alias(struct perf_pmu *pmu, const char *name,
>  	 *
>  	 * Rebuild string to make alias->str member comparable.
>  	 */
> -	ret = 0;
> -	list_for_each_entry(term, &alias->terms, list) {
> -		if (ret)
> -			ret += scnprintf(newval + ret, sizeof(newval) - ret,
> -					 ",");
> -		if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
> -			ret += scnprintf(newval + ret, sizeof(newval) - ret,
> -					 "%s=%#x", term->config, term->val.num);
> -		else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR)
> -			ret += scnprintf(newval + ret, sizeof(newval) - ret,
> -					 "%s=%s", term->config, term->val.str);
> -	}
>  	zfree(&alias->str);
> -	alias->str = strdup(newval);
> +	strbuf_init(&sb, /*hint=*/ 0);
> +	parse_events_term__to_strbuf(&alias->terms, &sb);
> +	alias->str = strbuf_detach(&sb, /*sz=*/ NULL);
>  	if (!pe)
>  		pmu->sysfs_aliases++;
>  	else