linux-kernel - Re: [PATCH] perf metric: Code cleanup with map_for_each

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 17 Sep 2020 19:53:29 +0800
From:   "liwei (GF)" <liwei391@...wei.com>
To:     Namhyung Kim <namhyung@...nel.org>
CC:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        "Ian Rogers" <irogers@...gle.com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "Li Bin" <huawei.libin@...wei.com>, <guohanjun@...wei.com>
Subject: Re: [PATCH] perf metric: Code cleanup with map_for_each_event()

Hi Namhyung,

On 2020/9/17 13:19, Namhyung Kim wrote:
> Hello,
> 
> On Thu, Sep 17, 2020 at 11:45 AM Wei Li <liwei391@...wei.com> wrote:
>>
>> Since we have introduced map_for_each_event() to walk the 'pmu_events_map',
>> clean up metricgroup__print() and metricgroup__has_metric() with it.
>>
>> Signed-off-by: Wei Li <liwei391@...wei.com>
> 
> Acked-by: Namhyung Kim <namhyung@...nel.org>
> 
> A nit-pick below:
> 
> 
>> ---
>>  tools/perf/util/metricgroup.c | 33 +++++++++++++--------------------
>>  1 file changed, 13 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index 8831b964288f..3734cbb2c456 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -26,6 +26,17 @@
>>  #include "util.h"
>>  #include <asm/bug.h>
>>
>> +#define map_for_each_event(__pe, __idx, __map)                         \
>> +       for (__idx = 0, __pe = &__map->table[__idx];                    \
>> +            __pe->name || __pe->metric_group || __pe->metric_name;     \
>> +            __pe = &__map->table[++__idx])
>> +
>> +#define map_for_each_metric(__pe, __idx, __map, __metric)              \
>> +       map_for_each_event(__pe, __idx, __map)                          \
>> +               if (__pe->metric_expr &&                                \
>> +                   (match_metric(__pe->metric_group, __metric) ||      \
>> +                    match_metric(__pe->metric_name, __metric)))
>> +
> 
> You may consider adding a declaration of match_metric() here.
> Right now, all users are below the function so it's ok but
> having the macro here can enable future addition above IMHO.
> 

Thanks for spotting that.
I'd like to move them just below match_metric() for now, when we really need
to use them somewhere else, we may move them to header file then.

Thanks,
Wei