Messages in this thread | | | From | Ian Rogers <> | Date | Wed, 20 Jul 2022 08:18:04 -0700 | Subject | Re: [PATCH v3 1/3] perf: Align user space counter reading with code |
| |
On Wed, Jul 20, 2022 at 8:06 AM Vince Weaver <vincent.weaver@maine.edu> wrote: > > On Tue, 19 Jul 2022, Ian Rogers wrote: > > > Align the user space counter reading documentation with the code in > > perf_mmap__read_self. Previously the documentation was based on the perf > > rdpmc test, but now general purpose code is provided by libperf. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > include/uapi/linux/perf_event.h | 35 +++++++++++++++++---------- > > tools/include/uapi/linux/perf_event.h | 35 +++++++++++++++++---------- > > 2 files changed, 44 insertions(+), 26 deletions(-) > > > > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h > > index d37629dbad72..6826dabb7e03 100644 > > --- a/include/uapi/linux/perf_event.h > > +++ b/include/uapi/linux/perf_event.h > > @@ -538,9 +538,13 @@ struct perf_event_mmap_page { > > * > > * if (pc->cap_usr_time && enabled != running) { > > * cyc = rdtsc(); > > - * time_offset = pc->time_offset; > > * time_mult = pc->time_mult; > > * time_shift = pc->time_shift; > > + * time_offset = pc->time_offset; > > + * if (pc->cap_user_time_short) { > > + * time_cycles = pc->time_cycles; > > + * time_mask = pc->time_mask; > > + * } > > From what I've been told, and from what perf_mmap__read_self() actually > does, many of these MMAP fields need to be accessed by READ_ONCE() > (a GPLv2 only interface) to be correct. > > Should we update perf_event.h to reflect this? Otherwise it's confusing > when the actual code and the documentation in the header don't match like > this. As an example, see the actual code snippets from > perf_mmap__read_self() > > 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); > > ...
Thanks! So I think what this patch proposes is an improvement, specifically it aligns it better with the code and deals with the divide by zero. I think what you're asking for makes sense but as you point out READ_ONCE probably isn't the correct API for something outside the kernel. Given the kernel is now C11: https://lwn.net/Articles/885941/ This opens the possibility of using stdatomic.h, so perhaps we can move these variables to more correct atomic types. So, I think we can land this and worry about the atomic API in a follow up.
Thanks, Ian
> Vince
| |