MIME-Version: 1.0
References: <20230228192145.2120675-1-song@kernel.org> <CAM9d7chLaXDU4mMkD1U6YuOGZXcu7PFWGOuEkFKSkbrCLS+zWg@mail.gmail.com>
 <BBB1A458-25CA-4C0F-AF04-18534D092142@fb.com>
In-Reply-To: <BBB1A458-25CA-4C0F-AF04-18534D092142@fb.com>
From:   Namhyung Kim <namhyung@kernel.org>
Date:   Thu, 2 Mar 2023 13:15:46 -0800
Message-ID: <CAM9d7chCcz=o+XX_ruZ3+zfp2Z2sPDG43dpTZH_mf6zXYjTJ7w@mail.gmail.com>
Subject: Re: [PATCH] perf: fix perf_event_context->time
To:     Song Liu <songliubraving@meta.com>
Cc:     Song Liu <song@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        Kernel Team <kernel-team@meta.com>,
        Peter Zijlstra <peterz@infradead.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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