lkml.org 
[lkml]   [2021]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3] perf/core: Set event shadow time for inactive events too
On Fri, Dec 10, 2021 at 2:33 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Dec 09, 2021 at 01:35:11PM -0800, Namhyung Kim wrote:
> > On Thu, Dec 9, 2021 at 3:26 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Sun, Dec 05, 2021 at 02:48:43PM -0800, Namhyung Kim wrote:
> > >
> > > > Actually 18446744069443110306 is 0xffffffff01b345a2 so it seems to
> > > > have a negative enabled time. In fact, bperf keeps values returned by
> > > > bpf_perf_event_read_value() which calls perf_event_read_local(), and
> > > > accumulates delta between two calls. When event->shadow_ctx_time is
> > > > not set, it'd return invalid enabled time which is bigger than normal.
> > >
> > > *that*, how does it happen that shadow_time isn't set? It should be last
> > > set when the event switches to INACTIVE, no?
> >
> > As you can see, perf_event_set_state() doesn't set the shadow time.
> > It's called from event_sched_in() which might result in ACTIVE or
> > INACTIVE. But the problem is that there's a case that event_sched_in
> > was not called at all - when group_can_go_on() returns false.
> >
> > > At which point the logic in
> > > perf_event_read_local() should make @enabled move forward while @running
> > > stays put.
> >
> > It's not about updating event->total_time_enabled, it only
> > afftects the returned value of @enabled.
> >
> > I'd say the time calculation is broken so it'd break @running
> > as well. But this case can only happen on INACTIVE -
> > otherwise it'd call event_sched_in() and update the shadow
> > time properly, so no issue there. And then we can see
> > the broken value of enabled time only.
>
> I'm thinking this is a cgroup specific thing. Normally the shadow_time
> thing is simply a relative displacement between event-time and the
> global clock. That displacement never changes, except when you do
> IOC_DISABLE/IOC_ENABLE.

I think it changes when the events are scheduled in and out.
The global clock (ctx->timestamp) is constantly changing
when any event in the context is scheduled while event-
time might not change if the event is not scheduled, no?

Anyway, as I told you this is not a cgroup event.
The point of the BPF work was not to use cgroup events
and my example in the commit message was not about
cgroups at all.

The cgroup event has its own set of problems.. sigh.
I'll post one that I hit recently.

>
> However, for cgroup things are different, since the cgroup events aren't
> unconditionally runnable, that is, the enabled time should only count
> when the cgroup is active, right?

Yeah, that's my understanding.

>
> So perhaps perf_event_read_local() should use a cgroup clock instead of
> perf_clock() for cgroup events.
>
> Let me think about that some more...

Thanks,
Namhyung

\
 
 \ /
  Last update: 2021-12-11 00:20    [W:0.105 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site