Re: [PATCH v1 06/14] libperf cpumap: Add any, empty and min helpers

[James Clark]

On 12/12/2023 14:00, James Clark wrote:
> 
> 
> On 29/11/2023 06:02, Ian Rogers wrote:
>> Additional helpers to better replace
>> perf_cpu_map__has_any_cpu_or_is_empty.
>>
>> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
>> ---
>>  tools/lib/perf/cpumap.c              | 27 +++++++++++++++++++++++++++
>>  tools/lib/perf/include/perf/cpumap.h | 16 ++++++++++++++++
>>  tools/lib/perf/libperf.map           |  4 ++++
>>  3 files changed, 47 insertions(+)
>>
>> diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
>> index 49fc98e16514..7403819da8fd 100644
>> --- a/tools/lib/perf/cpumap.c
>> +++ b/tools/lib/perf/cpumap.c
>> @@ -316,6 +316,19 @@ bool perf_cpu_map__has_any_cpu_or_is_empty(const struct perf_cpu_map *map)
>>  	return map ? __perf_cpu_map__cpu(map, 0).cpu == -1 : true;
>>  }
>>  
>> +bool perf_cpu_map__is_any_cpu_or_is_empty(const struct perf_cpu_map *map)
>> +{
>> +	if (!map)
>> +		return true;
>> +
>> +	return __perf_cpu_map__nr(map) == 1 && __perf_cpu_map__cpu(map, 0).cpu == -1;
>> +}
>> +
>> +bool perf_cpu_map__is_empty(const struct perf_cpu_map *map)
>> +{
>> +	return map == NULL;
>> +}
>> +
> 
> Maybe it doesn't currently happen, but it seems a bit weird that the
> 'new' function can create a map of length 0 which would return empty ==
> false here.
> 
> Could we either make this check also return true for maps with length 0,
> or prevent the new function from returning a map of 0 length?
> 

By 'new' I actually meant perf_cpu_map__alloc(). I see
perf_cpu_map__new() actually takes a string, and returns all online CPUs
if the string is null, but similarly I don't see a use case where that
string is NULL.

Having something return either the parsed string, or all online CPUs if
the string was null seems like unnecessary hidden behaviour so it would
be good to remove that one too.

>>  int perf_cpu_map__idx(const struct perf_cpu_map *cpus, struct perf_cpu cpu)
>>  {
>>  	int low, high;
>> @@ -372,6 +385,20 @@ bool perf_cpu_map__has_any_cpu(const struct perf_cpu_map *map)
>>  	return map && __perf_cpu_map__cpu(map, 0).cpu == -1;
>>  }
>>  
>> +struct perf_cpu perf_cpu_map__min(const struct perf_cpu_map *map)
>> +{
>> +	struct perf_cpu cpu, result = {
>> +		.cpu = -1
>> +	};
>> +	int idx;
>> +
>> +	perf_cpu_map__for_each_cpu_skip_any(cpu, idx, map) {
>> +		result = cpu;
>> +		break;
>> +	}
>> +	return result;
>> +}
>> +
>>  struct perf_cpu perf_cpu_map__max(const struct perf_cpu_map *map)
>>  {
>>  	struct perf_cpu result = {
>> diff --git a/tools/lib/perf/include/perf/cpumap.h b/tools/lib/perf/include/perf/cpumap.h
>> index dbe0a7352b64..523e4348fc96 100644
>> --- a/tools/lib/perf/include/perf/cpumap.h
>> +++ b/tools/lib/perf/include/perf/cpumap.h
>> @@ -50,6 +50,22 @@ LIBPERF_API int perf_cpu_map__nr(const struct perf_cpu_map *cpus);
>>   * perf_cpu_map__has_any_cpu_or_is_empty - is map either empty or has the "any CPU"/dummy value.
>>   */
>>  LIBPERF_API bool perf_cpu_map__has_any_cpu_or_is_empty(const struct perf_cpu_map *map);
>> +/**
>> + * perf_cpu_map__is_any_cpu_or_is_empty - is map either empty or the "any CPU"/dummy value.
>> + */
>> +LIBPERF_API bool perf_cpu_map__is_any_cpu_or_is_empty(const struct perf_cpu_map *map);
>> +/**
>> + * perf_cpu_map__is_empty - does the map contain no values and it doesn't
>> + *                          contain the special "any CPU"/dummy value.
>> + */
>> +LIBPERF_API bool perf_cpu_map__is_empty(const struct perf_cpu_map *map);
>> +/**
>> + * perf_cpu_map__min - the minimum CPU value or -1 if empty or just the "any CPU"/dummy value.
>> + */
>> +LIBPERF_API struct perf_cpu perf_cpu_map__min(const struct perf_cpu_map *map);
>> +/**
>> + * perf_cpu_map__max - the maximum CPU value or -1 if empty or just the "any CPU"/dummy value.
>> + */
>>  LIBPERF_API struct perf_cpu perf_cpu_map__max(const struct perf_cpu_map *map);
>>  LIBPERF_API bool perf_cpu_map__has(const struct perf_cpu_map *map, struct perf_cpu cpu);
>>  LIBPERF_API bool perf_cpu_map__equal(const struct perf_cpu_map *lhs,
>> diff --git a/tools/lib/perf/libperf.map b/tools/lib/perf/libperf.map
>> index 10b3f3722642..2aa79b696032 100644
>> --- a/tools/lib/perf/libperf.map
>> +++ b/tools/lib/perf/libperf.map
>> @@ -10,6 +10,10 @@ LIBPERF_0.0.1 {
>>  		perf_cpu_map__nr;
>>  		perf_cpu_map__cpu;
>>  		perf_cpu_map__has_any_cpu_or_is_empty;
>> +		perf_cpu_map__is_any_cpu_or_is_empty;
>> +		perf_cpu_map__is_empty;
>> +		perf_cpu_map__has_any_cpu;
>> +		perf_cpu_map__min;
>>  		perf_cpu_map__max;
>>  		perf_cpu_map__has;
>>  		perf_thread_map__new_array;
>