Re: [PATCH] perf stat: Take cgroups into account for shadow stats

[Namhyung Kim]

On Sun, Nov 15, 2020 at 10:08 PM Andi Kleen  wrote:
>
> Actually thinking about it more you should probably pass around ctx/cgroup
> in a single abstract argument. Otherwise have to change all the metrics
> functions for the next filter too.

Ok, will do.

Thanks,
Namhyung

Re: [PATCH] perf stat: Take cgroups into account for shadow stats

[Namhyung Kim]

Hi Andi,

On Sun, Nov 15, 2020 at 10:05 PM Andi Kleen  wrote:
>
> > @@ -57,6 +59,9 @@ static int saved_value_cmp(struct rb_node *rb_node, const 
> > void *entry)
> >   if (a->ctx != b->ctx)
> >   return a->ctx - b->ctx;
> >
> > + if (a->cgrp != b->cgrp)
> > + return (char *)a->cgrp < (char *)b->cgrp ? -1 : +1;
>
> This means the sort order will depend on heap randomization,
> which will make it harder to debug.
>
> Better use something stable like the inode number of the cgroup.

I don't think they are used for sorting.  It's just to compare to find a
matching event value.

For heap randomization, we already used the same technique
for evsel and runtime_stat pointers.  I can make it use cgroup id
but not sure it is really worth it.

>
> Do we have the same problem with other filters?

I'm not aware of it.

>
> The rest of the patch looks good to me.

Thanks for the review!

Namhyung

Re: [PATCH] perf stat: Take cgroups into account for shadow stats

[Andi Kleen]

Actually thinking about it more you should probably pass around ctx/cgroup
in a single abstract argument. Otherwise have to change all the metrics
functions for the next filter too.

Re: [PATCH] perf stat: Take cgroups into account for shadow stats

[Andi Kleen]

> @@ -57,6 +59,9 @@ static int saved_value_cmp(struct rb_node *rb_node, const 
> void *entry)
>   if (a->ctx != b->ctx)
>   return a->ctx - b->ctx;
>  
> + if (a->cgrp != b->cgrp)
> + return (char *)a->cgrp < (char *)b->cgrp ? -1 : +1;

This means the sort order will depend on heap randomization, 
which will make it harder to debug.

Better use something stable like the inode number of the cgroup.

Do we have the same problem with other filters?

The rest of the patch looks good to me.

-Andi