linux-kernel - Re: [PATCH v3] perf/core: Set event shadow time for inactive events too

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 10 Dec 2021 15:19:53 -0800
From:   Namhyung Kim <namhyung@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Ian Rogers <irogers@...gle.com>,
        Song Liu <songliubraving@...com>
Subject: Re: [PATCH v3] perf/core: Set event shadow time for inactive events too

On Fri, Dec 10, 2021 at 2:33 AM Peter Zijlstra <peterz@...radead.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@...radead.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