Messages in this thread | | | From | Namhyung Kim <> | Date | Thu, 2 Mar 2023 13:15:46 -0800 | Subject | Re: [PATCH] perf: fix perf_event_context->time |
| |
On Wed, Mar 1, 2023 at 3:16 PM Song Liu <songliubraving@meta.com> wrote: > > > > > On Mar 1, 2023, at 2:29 PM, Namhyung Kim <namhyung@kernel.org> wrote: > > > > Hi Song, > > > > On Tue, Feb 28, 2023 at 11:22 AM Song Liu <song@kernel.org> wrote: > >> > >> Time readers rely on perf_event_context->[time|timestamp|timeoffset] to get > >> accurate time_enabled and time_running for an event. The difference between > >> ctx->timestamp and ctx->time is the among of time when the context is not > >> enabled. For cpuctx.ctx, time and timestamp should stay the same, however, > > > > I'm not sure if it's correct. The timestamp can go when the context is disabled > > for example, in ctx_resched() even if the NMI watchdog is enabled, right? > > I think we do not disable EVENT_TIME for per cpu ctx?
I can see ctx_sched_out(ctx, EVENT_TIME) in some places. Also it'd reset EVENT_TIME if both _PINNED and _FLEXIBLE is cleared.
Thanks, Namhyung
> > > > >> it is not the case at the moment. To show this with drgn [1]: > >> > >> drgn 0.0.22 (using Python 3.8.6, elfutils 0.185, with libkdumpfile) > >> For help, type help(drgn). > >> ... > >>>>> ctx = per_cpu_ptr(prog['pmu'].pmu_cpu_context, 0).ctx > >>>>> ctx.timestamp * 1.0 / ctx.time > >> (double)1.0385869134111765 > >>>>> > >> > >> ctx->timestamp is about 4% larger than ctx.time. This issue causes time > >> read by perf_event_read_local() goes back in some cases. > > > > I don't think the difference between the ctx.time and the timestamp > > itself is a problem. I think the problem is it CAN update the timestamp > > (and the timeoffset) while the context is enabled. Then depending on > > the timing, event times can return smaller values than before. > > For per cpu ctx, I think timeoffset should stay the same (may not be zero > though). > > Thanks, > Song > >
| |