On Tue, May 13, 2025 at 3:12 PM Ian Rogers [off-list ref] wrote:

On Tue, May 13, 2025 at 2:13 PM Arnaldo Carvalho de Melo
[off-list ref] wrote:

quoted

On Fri, May 02, 2025 at 01:14:32PM +0530, Mukesh Kumar Chaurasiya wrote:

quoted

On Fri, Apr 25, 2025 at 02:46:43PM -0300, Arnaldo Carvalho de Melo wrote:

quoted

Maybe that max() call in perf_cpu_map__intersect() somehow makes the
compiler happy.

quoted

quoted

And in perf_cpu_map__alloc() all calls seems to validate it.

quoted

quoted

Like:

quoted

quoted

+++ b/tools/lib/perf/cpumap.c

@@ -411,7 +411,7 @@ int perf_cpu_map__merge(struct perf_cpu_map **orig, struct perf_cpu_map *other)
        }

        tmp_len = __perf_cpu_map__nr(*orig) + __perf_cpu_map__nr(other);
-       tmp_cpus = malloc(tmp_len * sizeof(struct perf_cpu));
+       tmp_cpus = calloc(tmp_len, sizeof(struct perf_cpu));
        if (!tmp_cpus)
                return -ENOMEM;

quoted

quoted

⬢ [acme@toolbx perf-tools-next]$

quoted

quoted

And better, do the max size that the compiler is trying to help us
catch?

quoted

Isn't it better to use perf_cpu_map__nr. That should fix this problem.

Maybe, have you tried it?

quoted

One question I have, in perf_cpu_map__nr, the function is returning
1 in case *cpus is NULL. Is it ok to do that? wouldn't it cause problems?

Indeed this better be documented, as by just looking at:

int perf_cpu_map__nr(const struct perf_cpu_map *cpus)
{
        return cpus ? __perf_cpu_map__nr(cpus) : 1;
}

It really doesn't make much sense to say that a NULL map has one entry.

But the next functions are:

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;
}

So it seems that a NULL cpu map means "any/all CPU) and a map with just
one entry would have as its content "-1" that would mean "any/all CPU".

Ian did work on trying to simplify/clarify this, so maybe he can chime
in :-)

So I've tried to improve the naming but not vary the implementation
greatly - initially I was in the code fixing reference count checking
issues. There is an important distinction between "all" meaning a
range of CPUs like 0-15 on a 16 core/hyperthread system, and "any"
meaning the special "-1" value. It is possible to have a perf_cpu_map
to both be "all" and "any", iterating an empty perf_cpu_map has
strangely also meant the "any" and so the code isn't specific but
relies on these odd properties.

I just remembered, there was also a discussion with Adrian IIRC. I
wanted "all" to mean all possible CPUs on a system, and "online" to be
either the same or a subset of that, in case CPUs were offline. There
was a different view that the distinction between "all" and "online"
wasn't useful. So there is some confusion in the code in this regard
and some errors may exist when CPU cores are offline. There's
definitely work to make the API better.

Thanks,
Ian

Anyway, I'm not sure on the implication of this with
malloc/calloc/unsigned... It would seem reasonable to me for
__perf_cpu_map__nr to return an unsigned number and to propagate that
to fix the new GCC issue.

Thanks,
Ian

quoted

- Arnaldo