Re: [PATCH v2 5/9] libperf: Add support for user space counter access

[Rob Herring]

On Mon, Aug 31, 2020 at 3:11 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> On Fri, Aug 28, 2020 at 02:56:10PM -0600, Rob Herring wrote:
>
> SNIP


> > +int perf_mmap__read_self(struct perf_mmap *map, struct perf_counts_values *count)
> > +{
> > +     struct perf_event_mmap_page *pc = map->base;
> > +     u32 seq, idx, time_mult = 0, time_shift = 0;
> > +     u64 cnt, cyc = 0, time_offset = 0, time_cycles = 0, time_mask = ~0ULL;
> > +
> > +     BUG_ON(!pc);
> > +
> > +     if (!pc->cap_user_rdpmc)
> > +             return -1;
> > +
> > +     do {
> > +             seq = READ_ONCE(pc->lock);
> > +             barrier();
> > +
> > +             count->ena = READ_ONCE(pc->time_enabled);
> > +             count->run = READ_ONCE(pc->time_running);
> > +
> > +             if (pc->cap_user_time && count->ena != count->run) {
> > +                     cyc = read_timestamp();
> > +                     time_mult = READ_ONCE(pc->time_mult);
> > +                     time_shift = READ_ONCE(pc->time_shift);
> > +                     time_offset = READ_ONCE(pc->time_offset);
> > +
> > +                     if (pc->cap_user_time_short) {
> > +                             time_cycles = READ_ONCE(pc->time_cycles);
> > +                             time_mask = READ_ONCE(pc->time_mask);
> > +                     }
> > +             }
> > +
> > +             idx = READ_ONCE(pc->index);
> > +             cnt = READ_ONCE(pc->offset);
> > +             if (pc->cap_user_rdpmc && idx) {
>
> no need to check pc->cap_user_rdpmc again

I was thinking cap_user_rdpmc could change, but I guess idx will
always be 0 in that case.

> > +static int test_stat_user_read(int event)
> > +{
> > +     struct perf_counts_values counts = { .val = 0 };
> > +     struct perf_thread_map *threads;
> > +     struct perf_evsel *evsel;
> > +     struct perf_event_mmap_page *pc;
> > +     struct perf_event_attr attr = {
> > +             .type   = PERF_TYPE_HARDWARE,
> > +             .config = event,
> > +     };
> > +     int err, i;
> > +
> > +     threads = perf_thread_map__new_dummy();
> > +     __T("failed to create threads", threads);
> > +
> > +     perf_thread_map__set_pid(threads, 0, 0);
> > +
> > +     evsel = perf_evsel__new(&attr);
> > +     __T("failed to create evsel", evsel);
> > +
> > +     err = perf_evsel__open(evsel, NULL, threads);
> > +     __T("failed to open evsel", err == 0);
> > +
> > +     pc = perf_evsel__mmap(evsel);
> > +     __T("failed to mmap evsel", pc);
> > +
> > +#if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
> > +     __T("userspace counter access not supported", pc->cap_user_rdpmc);
> > +     __T("userspace counter access not enabled", pc->index);
> > +     __T("userspace counter width not set", pc->pmc_width >= 32);
> > +#endif
> > +
> > +     perf_evsel__read(evsel, 0, 0, &counts);
> > +     __T("failed to read value for evsel", counts.val != 0);
> > +
> > +     fputs("\n", stderr);
> > +     for (i = 0; i < 5; i++) {
> > +             volatile int count = 0x10000 << i;
> > +             __u64 start, end, last = 0;
> > +
> > +             fprintf(stderr, "\tloop = %u, ", count);
>
> we should add support to display verbose output for tests,
> because right now this breaks the output:
>
> - running test-cpumap.c...OK
> - running test-threadmap.c...OK
> - running test-evlist.c...OK
> - running test-evsel.c...
>         loop = 65536, count = 328035
>         loop = 131072, count = 655715
>         loop = 262144, count = 1311075
>         loop = 524288, count = 2627060
>         loop = 1048576, count = 5253540
>
>         loop = 65536, count = 327594
>         loop = 131072, count = 659930
>         loop = 262144, count = 1378892
>         loop = 524288, count = 2664341
>         loop = 1048576, count = 5365682
> OK
>
> but we can do it in separate change later

Would you like me to just comment this out then for now?

Rob