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

[Ian Rogers]

On Sun, Nov 27, 2022 at 11:02 PM Ravi Bangoria <ravi.bangoria@xxxxxxx> wrote:
>
> 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).

uncore_ works for Intel, but... I agree having pmus.c for 1 variable
is overkill. I'd put an iterator in pmus.h like:

#define perf_pmu__for_each_pmu(pmu) list_for_each_entry(pmu, &pmus, list)

and also not have pmus be static in pmu.c. That way in the test you
can iterate the pmus and find the uncore ones.

> >> +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

I'm fine with the other changes. I've done too much Java to be
allergic to long variable names ;-)

Thanks,
Ian