Re: [PATCH v5 1/4] perf util: Move block_pair_cmp to block-info

["Jin, Yao" <yao.jin@linux.intel.com>]

On 1/31/2020 4:32 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 28, 2020 at 08:55:53PM +0800, Jin Yao escreveu:
>> block_pair_cmp() is a function which is used to compare
>> two blocks. Moving it from builtin-diff.c to block-info.c
>> to let it can be used by other builtins.
>>
>>   v4/v5:
>>   ------
>>   No change.
>>
>>   v3:
>>   ---
>>   Separate it from original patch for good tracking.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>   tools/perf/builtin-diff.c    | 17 -----------------
>>   tools/perf/util/block-info.c | 17 +++++++++++++++++
>>   tools/perf/util/block-info.h |  2 ++
>>   3 files changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
>> index f8b6ae557d8b..5ff1e21082cb 100644
>> --- a/tools/perf/builtin-diff.c
>> +++ b/tools/perf/builtin-diff.c
>> @@ -572,23 +572,6 @@ static void init_block_hist(struct block_hist *bh)
>>   	bh->valid = true;
>>   }
>>   
>> -static int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
>> -{
>> -	struct block_info *bi_a = a->block_info;
>> -	struct block_info *bi_b = b->block_info;
>> -	int cmp;
>> -
>> -	if (!bi_a->sym || !bi_b->sym)
>> -		return -1;
>> -
>> -	cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
>> -
>> -	if ((!cmp) && (bi_a->start == bi_b->start) && (bi_a->end == bi_b->end))
>> -		return 0;
>> -
>> -	return -1;
>> -}
>> -
>>   static struct hist_entry *get_block_pair(struct hist_entry *he,
>>   					 struct hists *hists_pair)
>>   {
>> diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
>> index c4b030bf6ec2..f0f38bdd496a 100644
>> --- a/tools/perf/util/block-info.c
>> +++ b/tools/perf/util/block-info.c
>> @@ -475,3 +475,20 @@ float block_info__total_cycles_percent(struct hist_entry *he)
>>   
>>   	return 0.0;
>>   }
>> +
>> +int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
> 
> First thing that came to mind was that hist_entry comparision functions
> had been changed to return int64_t recently, when I went to look at it I
> found this:
> 
> tools/perf/util/block-info.c
> 
> int64_t block_info__cmp(struct perf_hpp_fmt *fmt __maybe_unused,
>                          struct hist_entry *left, struct hist_entry *right)
> {
>          struct block_info *bi_l = left->block_info;
>          struct block_info *bi_r = right->block_info;
>          int cmp;
> .
> .
> .
> 
> Which look a bit more complete, can you check if that can be used
> instead or explain why my quick analysis of this is b0rken?
> 
> Perhaps we can have a __block_info__cmp() that doesn't receive the
> perf_hpp_fmt (that isn't even used above) so that the previous use of
> block_pair_cmp() can be replaced with __block_info__cmp() instead?
> 
> Thanks,
> 
> - Arnaldo
> 

Hi Arnaldo,

I think it's a good idea to use __block_info__cmp() to replace the 
block_pair_cmp(). The block_info__cmp() is more comprehensive than 
block_pair_cmp() and we only need a bit change such as,

--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -74,7 +74,7 @@ int64_t block_info__cmp(struct perf_hpp_fmt *fmt 
__maybe_unused,

         if (!bi_l->sym || !bi_r->sym) {
                 if (!bi_l->sym && !bi_r->sym)
-                       return 0;
+                       return -1;
                 else if (!bi_l->sym)
                         return -1;
                 else

It returns -1 if both syms are NULL.

I will update the patchset.

Thanks
Jin Yao

>> +{
>> +	struct block_info *bi_a = a->block_info;
>> +	struct block_info *bi_b = b->block_info;
>> +	int cmp;
>> +
>> +	if (!bi_a->sym || !bi_b->sym)
>> +		return -1;
>> +
>> +	cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
>> +
>> +	if ((!cmp) && (bi_a->start == bi_b->start) && (bi_a->end == bi_b->end))
>> +		return 0;
>> +
>> +	return -1;
>> +}
>> diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
>> index bef0d75e9819..4fa91eeae92e 100644
>> --- a/tools/perf/util/block-info.h
>> +++ b/tools/perf/util/block-info.h
>> @@ -76,4 +76,6 @@ int report__browse_block_hists(struct block_hist *bh, float min_percent,
>>   
>>   float block_info__total_cycles_percent(struct hist_entry *he);
>>   
>> +int block_pair_cmp(struct hist_entry *a, struct hist_entry *b);
>> +
>>   #endif /* __PERF_BLOCK_H */
>> -- 
>> 2.17.1
>>
>