Re: [PATCH] perf test: Add event group test

[Ravi Bangoria]

On 27-Nov-22 8:58 AM, Ian Rogers wrote:
> On Thu, Nov 24, 2022 at 7:21 PM Ravi Bangoria <ravi.bangoria@xxxxxxx> wrote:
>>
>> Multiple events in a group can belong to one or more pmus, however
>> there are some limitations to it. Basically, perf doesn't allow
>> creating a group of events from different hw pmus. Write a simple
>> test to create various combinations of hw, sw and uncore pmu events
>> and verify group creation succeeds or fails as expected.
> 
> Awesome, thanks! Some comments below.

Thanks Ian!

>> +static int event_open(int type, unsigned long config, int g_fd)
>> +{
>> +       struct perf_event_attr attr;
>> +
>> +       memset(&attr, 0, sizeof(struct perf_event_attr));
>> +       attr.type = type;
>> +       attr.size = sizeof(struct perf_event_attr);
>> +       attr.config = config;
> 
> Could you add a comment for the line below?

Although this test exercises perf_event_open() and never enables any event,
I'm following standard practices. Snippet from man perf_event_open:

   disabled
       The disabled bit specifies whether the counter starts  out  dis‐
       abled  or  enabled.  If disabled, the event can later be enabled
       by ioctl(2), prctl(2), or enable_on_exec.

       When creating an event group, typically the group leader is ini‐
       tialized  with  disabled  set to 1 and any child events are ini‐
       tialized with disabled set to 0.  Despite disabled being 0,  the
       child events will not start until the group leader is enabled.

So it's well documented. I can probably put the same as comment.

> 
>> +       attr.disabled = g_fd == -1 ? 1 : 0;
>> +
>> +       return sys_perf_event_open(&attr, -1, 0, g_fd, 0);
>> +}
>> +
>> +/* hw: cycles, sw: context-switch, uncore: [arch dependent] */
> 
> static?

+1

> 
>> +int type[] = {0, 1, -1};
>> +unsigned long config[] = {0, 3, -1};
>> +
>> +static int setup_uncore_event(void)
>> +{
>> +       char pmu_name[25] = {0};
>> +       struct perf_pmu *pmu;
>> +
> 
> I think the below finding of an uncore PMU is clunky.

Agree.

> On my tigerlake
> Intel laptop, for example, I don't have an uncore_imc_0 but do have
> uncore_imc_free_running_0. I think the real fix here is that we should
> start a new "pmus.h" and "pmus.c", moving the pmus static variable
> from pmu.c to pmus.c. In pmus.h we should have an every PMU iterator,
> like we do with perf_pmu__for_each_hybrid_pmu.

I see only one variable that can be moved from pmu.c to pmus.c:
  LIST_HEAD(pmus)
So introducing new file pmus.c with just one list variable and a macro to
iterate over it seems overkill. Or are you suggesting to also migrate all
pmu.c functions which iterates over pmus list?

> I'd like to go further
> with a pmus.h, as the computation of the perf_pmu struct should be
> done a lot more lazily than it is now. But for now you can just
> iterate the pmus looking for one saying beginning with "uncore_" as a
> name.

Ok. I can probably introduce perf_pmu__starts_with(const char *name).

>> +static int run_test(int i, int j, int k)
>> +{
>> +       int erroneous = ((((1 << i) | (1 << j) | (1 << k)) & 5) == 5);
>> +       int fd1, fd2, fd3;
>> +
>> +       fd1 = event_open(type[i], config[i], -1);
> 
> nit: a name like "event_group_leader_fd" would be more intention
> revealing than fd1.

hmm, but that's too long :). Are you ok with:
  s/fd1/group_fd/
  s/fd2/sibling_fd1/
  s/fd3/sibling_fd2/

Thanks,
Ravi