Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 06/21] perf metric: Add documentation and rename a
 variable.
To:     Ian Rogers <irogers@google.com>, Andi Kleen <ak@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Jin Yao <yao.jin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        John Garry <john.garry@huawei.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>,
        Kees Cook <keescook@chromium.org>,
        Sami Tolvanen <samitolvanen@google.com>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Jacob Keller <jacob.e.keller@intel.com>,
        Zhen Lei <thunder.leizhen@huawei.com>,
        ToastC <mrtoastcheng@gmail.com>,
        Joakim Zhang <qiangqing.zhang@nxp.com>,
        Felix Fietkau <nbd@nbd.name>,
        Jiapeng Chong <jiapeng.chong@linux.alibaba.com>,
        Song Liu <songliubraving@fb.com>, Fabian Hemmer <copy@copy.sh>,
        Alexander Antonov <alexander.antonov@linux.intel.com>,
        Nicholas Fraser <nfraser@codeweavers.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Denys Zagorui <dzagorui@cisco.com>,
        Wan Jiabing <wanjiabing@vivo.com>,
        Thomas Richter <tmricht@linux.ibm.com>,
        Sumanth Korikkar <sumanthk@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Changbin Du <changbin.du@intel.com>,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        Andrew Kilroy <andrew.kilroy@arm.com>
Cc:     Stephane Eranian <eranian@google.com>
References: <20211015172132.1162559-1-irogers@google.com>
 <20211015172132.1162559-7-irogers@google.com>
From:   kajoljain <kjain@linux.ibm.com>
Message-ID: <38906a72-d974-73c1-636e-54a3d87f0fa5@linux.ibm.com>
Date:   Tue, 26 Oct 2021 13:48:00 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <20211015172132.1162559-7-irogers@google.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/15/21 10:51 PM, Ian Rogers wrote:
> Documentation to make current functionality clearer. Rename a variable
> called 'metric' to 'metric_name' as it can be ambiguous as to whether a
> string is the name of a metric or the expression.
> 
> Acked-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---

Patch looks good to me.

Reviewed-by: Kajol Jain<kjain@linux.ibm.com>

Thanks,
Kajol Jain

>  tools/perf/util/metricgroup.c | 59 ++++++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 139f4a793f92..3e5f02938452 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -776,13 +776,27 @@ int __weak arch_get_runtimeparam(const struct pmu_event *pe __maybe_unused)
>  
>  struct metricgroup_add_iter_data {
>  	struct list_head *metric_list;
> -	const char *metric;
> +	const char *metric_name;
>  	struct expr_ids *ids;
>  	int *ret;
>  	bool *has_match;
>  	bool metric_no_group;
>  };
>  
> +/**
> + * __add_metric - Add a metric to metric_list.
> + * @metric_list: The list the metric is added to.
> + * @pe: The pmu_event containing the metric to be added.
> + * @metric_no_group: Should events written to events be grouped "{}" or
> + *                   global. Grouping is the default but due to multiplexing the
> + *                   user may override.
> + * @runtime: A special argument for the parser only known at runtime.
> + * @mp: The pointer to a location holding the first metric added to metric
> + *      list. It is initialized here if this is the first metric.
> + * @parent: The last entry in a linked list of metrics being
> + *          added/resolved. This is maintained to detect recursion.
> + * @ids: Storage for parent list.
> + */
>  static int __add_metric(struct list_head *metric_list,
>  			const struct pmu_event *pe,
>  			bool metric_no_group,
> @@ -1076,7 +1090,7 @@ static int metricgroup__add_metric_sys_event_iter(const struct pmu_event *pe,
>  	struct metric *m = NULL;
>  	int ret;
>  
> -	if (!match_pe_metric(pe, d->metric))
> +	if (!match_pe_metric(pe, d->metric_name))
>  		return 0;
>  
>  	ret = add_metric(d->metric_list, pe, d->metric_no_group, &m, NULL, d->ids);
> @@ -1095,7 +1109,22 @@ static int metricgroup__add_metric_sys_event_iter(const struct pmu_event *pe,
>  	return ret;
>  }
>  
> -static int metricgroup__add_metric(const char *metric, bool metric_no_group,
> +/**
> + * metricgroup__add_metric - Find and add a metric, or a metric group.
> + * @metric_name: The name of the metric or metric group. For example, "IPC"
> + *               could be the name of a metric and "TopDownL1" the name of a
> + *               metric group.
> + * @metric_no_group: Should events written to events be grouped "{}" or
> + *                   global. Grouping is the default but due to multiplexing the
> + *                   user may override.
> + * @events: an out argument string of events that need to be parsed and
> + *          associated with the metric. For example, the metric "IPC" would
> + *          create an events string like "{instructions,cycles}:W".
> + * @metric_list: The list that the metric or metric group are added to.
> + * @map: The map that is searched for metrics, most commonly the table for the
> + *       architecture perf is running upon.
> + */
> +static int metricgroup__add_metric(const char *metric_name, bool metric_no_group,
>  				   struct strbuf *events,
>  				   struct list_head *metric_list,
>  				   const struct pmu_events_map *map)
> @@ -1107,7 +1136,11 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>  	int i, ret;
>  	bool has_match = false;
>  
> -	map_for_each_metric(pe, i, map, metric) {
> +	/*
> +	 * Iterate over all metrics seeing if metric matches either the name or
> +	 * group. When it does add the metric to the list.
> +	 */
> +	map_for_each_metric(pe, i, map, metric_name) {
>  		has_match = true;
>  		m = NULL;
>  
> @@ -1130,7 +1163,7 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>  			.fn = metricgroup__add_metric_sys_event_iter,
>  			.data = (void *) &(struct metricgroup_add_iter_data) {
>  				.metric_list = &list,
> -				.metric = metric,
> +				.metric_name = metric_name,
>  				.metric_no_group = metric_no_group,
>  				.ids = &ids,
>  				.has_match = &has_match,
> @@ -1169,6 +1202,22 @@ static int metricgroup__add_metric(const char *metric, bool metric_no_group,
>  	return ret;
>  }
>  
> +/**
> + * metricgroup__add_metric_list - Find and add metrics, or metric groups,
> + *                                specified in a list.
> + * @list: the list of metrics or metric groups. For example, "IPC,CPI,TopDownL1"
> + *        would match the IPC and CPI metrics, and TopDownL1 would match all
> + *        the metrics in the TopDownL1 group.
> + * @metric_no_group: Should events written to events be grouped "{}" or
> + *                   global. Grouping is the default but due to multiplexing the
> + *                   user may override.
> + * @events: an out argument string of events that need to be parsed and
> + *          associated with the metric. For example, the metric "IPC" would
> + *          create an events string like "{instructions,cycles}:W".
> + * @metric_list: The list that metrics are added to.
> + * @map: The map that is searched for metrics, most commonly the table for the
> + *       architecture perf is running upon.
> + */
>  static int metricgroup__add_metric_list(const char *list, bool metric_no_group,
>  					struct strbuf *events,
>  					struct list_head *metric_list,
>