Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <528dd99d-6a2f-8b10-e3e3-21f682ab9101@linux.intel.com>
Date:   Thu, 27 Apr 2023 16:01:18 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v1 14/40] perf print-events: Avoid unnecessary strlist
Content-Language: en-US
To:     Ian Rogers <irogers@google.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Ahmad Yasin <ahmad.yasin@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Stephane Eranian <eranian@google.com>,
        Andi Kleen <ak@linux.intel.com>,
        Perry Taylor <perry.taylor@intel.com>,
        Samantha Alt <samantha.alt@intel.com>,
        Caleb Biggers <caleb.biggers@intel.com>,
        Weilin Wang <weilin.wang@intel.com>,
        Edward Baker <edward.baker@intel.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Florian Fischer <florian.fischer@muhq.space>,
        Rob Herring <robh@kernel.org>,
        Zhengjun Xing <zhengjun.xing@linux.intel.com>,
        John Garry <john.g.garry@oracle.com>,
        Kajol Jain <kjain@linux.ibm.com>,
        Sumanth Korikkar <sumanthk@linux.ibm.com>,
        Thomas Richter <tmricht@linux.ibm.com>,
        Tiezhu Yang <yangtiezhu@loongson.cn>,
        Ravi Bangoria <ravi.bangoria@amd.com>,
        Leo Yan <leo.yan@linaro.org>,
        Yang Jihong <yangjihong1@huawei.com>,
        James Clark <james.clark@arm.com>,
        Suzuki Poulouse <suzuki.poulose@arm.com>,
        Kang Minchul <tegongkang@gmail.com>,
        Athira Rajeev <atrajeev@linux.vnet.ibm.com>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230426070050.1315519-1-irogers@google.com>
 <20230426070050.1315519-15-irogers@google.com>
From:   "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <20230426070050.1315519-15-irogers@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2023-04-26 3:00 a.m., Ian Rogers wrote:
> The strlist in print_hwcache_events holds the event names as they are
> generated, and then it is iterated and printed. This is unnecessary
> and each event can just be printed as it is processed.
> Rename the variable i to res, to be more intention revealing and
> consistent with other code.
> 

Looks good to me.

Reviewed-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan

> Signed-off-by: Ian Rogers <irogers@google.com>> ---
>  tools/perf/util/print-events.c | 60 ++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/perf/util/print-events.c b/tools/perf/util/print-events.c
> index 386b1ab0b60e..93bbb868d400 100644
> --- a/tools/perf/util/print-events.c
> +++ b/tools/perf/util/print-events.c
> @@ -226,58 +226,60 @@ void print_sdt_events(const struct print_callbacks *print_cb, void *print_state)
>  
>  int print_hwcache_events(const struct print_callbacks *print_cb, void *print_state)
>  {
> -	struct strlist *evt_name_list = strlist__new(NULL, NULL);
> -	struct str_node *nd;
> +	const char *event_type_descriptor = event_type_descriptors[PERF_TYPE_HW_CACHE];
>  
> -	if (!evt_name_list) {
> -		pr_debug("Failed to allocate new strlist for hwcache events\n");
> -		return -ENOMEM;
> -	}
>  	for (int type = 0; type < PERF_COUNT_HW_CACHE_MAX; type++) {
>  		for (int op = 0; op < PERF_COUNT_HW_CACHE_OP_MAX; op++) {
>  			/* skip invalid cache type */
>  			if (!evsel__is_cache_op_valid(type, op))
>  				continue;
>  
> -			for (int i = 0; i < PERF_COUNT_HW_CACHE_RESULT_MAX; i++) {
> +			for (int res = 0; res < PERF_COUNT_HW_CACHE_RESULT_MAX; res++) {
>  				struct perf_pmu *pmu = NULL;
>  				char name[64];
>  
> -				__evsel__hw_cache_type_op_res_name(type, op, i, name, sizeof(name));
> +				__evsel__hw_cache_type_op_res_name(type, op, res,
> +								   name, sizeof(name));
>  				if (!perf_pmu__has_hybrid()) {
>  					if (is_event_supported(PERF_TYPE_HW_CACHE,
> -							       type | (op << 8) | (i << 16)))
> -						strlist__add(evt_name_list, name);
> +								type | (op << 8) | (res << 16))) {
> +						print_cb->print_event(print_state,
> +								"cache",
> +								/*pmu_name=*/NULL,
> +								name,
> +								/*event_alias=*/NULL,
> +								/*scale_unit=*/NULL,
> +								/*deprecated=*/false,
> +								event_type_descriptor,
> +								/*desc=*/NULL,
> +								/*long_desc=*/NULL,
> +								/*encoding_desc=*/NULL);
> +					}
>  					continue;
>  				}
>  				perf_pmu__for_each_hybrid_pmu(pmu) {
>  					if (is_event_supported(PERF_TYPE_HW_CACHE,
> -					    type | (op << 8) | (i << 16) |
> +					    type | (op << 8) | (res << 16) |
>  					    ((__u64)pmu->type << PERF_PMU_TYPE_SHIFT))) {
>  						char new_name[128];
> -							snprintf(new_name, sizeof(new_name),
> -								 "%s/%s/", pmu->name, name);
> -							strlist__add(evt_name_list, new_name);
> +						snprintf(new_name, sizeof(new_name),
> +							"%s/%s/", pmu->name, name);
> +						print_cb->print_event(print_state,
> +								"cache",
> +								pmu->name,
> +								name,
> +								new_name,
> +								/*scale_unit=*/NULL,
> +								/*deprecated=*/false,
> +								event_type_descriptor,
> +								/*desc=*/NULL,
> +								/*long_desc=*/NULL,
> +								/*encoding_desc=*/NULL);
>  					}
>  				}
>  			}
>  		}
>  	}
> -
> -	strlist__for_each_entry(nd, evt_name_list) {
> -		print_cb->print_event(print_state,
> -				"cache",
> -				/*pmu_name=*/NULL,
> -				nd->s,
> -				/*event_alias=*/NULL,
> -				/*scale_unit=*/NULL,
> -				/*deprecated=*/false,
> -				event_type_descriptors[PERF_TYPE_HW_CACHE],
> -				/*desc=*/NULL,
> -				/*long_desc=*/NULL,
> -				/*encoding_desc=*/NULL);
> -	}
> -	strlist__delete(evt_name_list);
>  	return 0;
>  }
>