lkml.org 
[lkml]   [2022]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 2/4] perf: Align user space counter reading with code
On Wed, Jun 8, 2022 at 4:44 PM Ian Rogers <irogers@google.com> 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.

IMO, this copy of not quite right code should just be removed perhaps
with a pointer to perf_mmap__read_self(). It will just get out of date
again. For example, the read loop might get rewritten with restartable
sequences.

> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> include/uapi/linux/perf_event.h | 32 ++++++++++++++++-----------
> tools/include/uapi/linux/perf_event.h | 32 ++++++++++++++++-----------
> 2 files changed, 38 insertions(+), 26 deletions(-)
>
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index d37629dbad72..3b84e0ad0723 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();

Kind of x86 specific.

> - * 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;
> + * }

This still misses the need for READ_ONCE().

> * }
> *
> * index = pc->index;
> @@ -548,6 +552,9 @@ struct perf_event_mmap_page {
> * if (pc->cap_user_rdpmc && index) {
> * width = pc->pmc_width;
> * pmc = rdpmc(index - 1);
> + * pmc <<= 64 - width;
> + * pmc >>= 64 - width;
> + * count += pmc;
> * }
> *
> * barrier();
> @@ -590,25 +597,24 @@ struct perf_event_mmap_page {
> * If cap_usr_time the below fields can be used to compute the time
> * delta since time_enabled (in ns) using rdtsc or similar.
> *
> - * u64 quot, rem;
> - * u64 delta;
> - *
> - * quot = (cyc >> time_shift);
> - * rem = cyc & (((u64)1 << time_shift) - 1);
> - * delta = time_offset + quot * time_mult +
> - * ((rem * time_mult) >> time_shift);
> + * cyc = time_cycles + ((cyc - time_cycles) & time_mask);
> + * delta = time_offset + mul_u64_u32_shr(cyc, time_mult, time_shift);

For documentation purposes, the original code was easier to read and
this is just an optimization. What does mul_u64_u32_shr() do exactly?
It's not documented.

> *
> * Where time_offset,time_mult,time_shift and cyc are read in the
> * seqcount loop described above. This delta can then be added to
> - * enabled and possible running (if index), improving the scaling:
> + * enabled and possible running (if index) to improve the scaling. Due
> + * to event multiplexing, running maybe zero and so care is needed to
> + * avoid division by zero.
> *
> * enabled += delta;
> - * if (index)
> + * if (idx)
> * running += delta;
> *
> - * quot = count / running;
> - * rem = count % running;
> - * count = quot * enabled + (rem * enabled) / running;
> + * if (running != 0) {
> + * quot = count / running;
> + * rem = count % running;
> + * count = quot * enabled + (rem * enabled) / running;
> + * }
> */
> __u16 time_shift;
> __u32 time_mult;

\
 
 \ /
  Last update: 2022-06-09 01:26    [W:0.078 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site