Message-ID: <20220426140959.op6u5m7id57aq7yc@wubuntu>
Date:   Tue, 26 Apr 2022 15:09:59 +0100
From:   Qais Yousef <qais.yousef@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Delyan Kratunov <delyank@...com>,
        Namhyung Kim <namhyung@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        "bigeasy@...utronix.de" <bigeasy@...utronix.de>,
        "dietmar.eggemann@....com" <dietmar.eggemann@....com>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "andrii@...nel.org" <andrii@...nel.org>,
        "u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>,
        "vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "Kenta.Tada@...y.com" <Kenta.Tada@...y.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "bristot@...hat.com" <bristot@...hat.com>,
        "ebiederm@...ssion.com" <ebiederm@...ssion.com>,
        "ast@...nel.org" <ast@...nel.org>,
        "legion@...nel.org" <legion@...nel.org>,
        "adharmap@...cinc.com" <adharmap@...cinc.com>,
        "valentin.schneider@....com" <valentin.schneider@....com>,
        "ed.tsai@...iatek.com" <ed.tsai@...iatek.com>,
        "juri.lelli@...hat.com" <juri.lelli@...hat.com>
Subject: Re: [PATCH] sched/tracing: append prev_state to tp args instead

On 04/26/22 14:28, Peter Zijlstra wrote:
> On Fri, Apr 22, 2022 at 11:30:12AM -0700, Alexei Starovoitov wrote:
> > On Fri, Apr 22, 2022 at 10:22 AM Delyan Kratunov <delyank@...com> wrote:
> > >
> > > On Fri, 2022-04-22 at 13:09 +0200, Peter Zijlstra wrote:
> > > > And on the other hand; those users need to be fixed anyway, right?
> > > > Accessing prev->__state is equally broken.
> > >
> > > The users that access prev->__state would most likely have to be fixed, for sure.
> > >
> > > However, not all users access prev->__state. `offcputime` for example just takes a
> > > stack trace and associates it with the switched out task. This kind of user
> > > would continue working with the proposed patch.
> > >
> > > > If bpf wants to ride on them, it needs to suffer the pain of doing so.
> > >
> > > Sure, I'm just advocating for a fairly trivial patch to avoid some of the suffering,
> > > hopefully without being a burden to development. If that's not the case, then it's a
> > > clear no-go.
> > 
> > 
> > Namhyung just sent this patch set:
> > https://patchwork.kernel.org/project/netdevbpf/patch/20220422053401.208207-3-namhyung@kernel.org/
> 
> That has:
> 
> + * recently task_struct->state renamed to __state so it made an incompatible
> + * change.
> 
> git tells me:
> 
>   2f064a59a11f ("sched: Change task_struct::state")
> 
> is almost a year old by now. That don't qualify as recently in my book.
> That says that 'old kernels used to call this...'.
> 
> > to add off-cpu profiling to perf.
> > It also hooks into sched_switch tracepoint.
> > Notice it deals with state->__state rename just fine.
> 
> So I don't speak BPF much; it always takes me more time to make bpf work
> than to just hack up the kernel, which makes it hard to get motivated.
> 
> However, it was not just a rename, state changed type too, which is why I
> did the rename, to make sure all users would get a compile fail and
> could adjust.
> 
> If you're silently making it work by frobbing the name, you loose that.
> 
> Specifically, task_struct::state used to be 'volatile long', while
> task_struct::__state is 'unsigned int'. As such, any user must now be
> very careful to use READ_ONCE(). I don't see that happening with just
> frobbing the name.
> 
> Additinoally, by shrinking the field, I suppose BE systems get to keep
> the pieces?
> 
> > But it will have a hard time without this patch
> > until we add all the extra CO-RE features to detect
> > and automatically adjust bpf progs when tracepoint
> > arguments order changed.
> 
> Could be me, but silently making it work sounds like fail :/ There's a
> reason code changes, users need to adapt, not silently pretend stuff is
> as before.
> 
> How will you know you need to fix your tool?

If libbpf doesn't fail, then yeah it's a big problem. I wonder how users of
kprobe who I suppose are more prone to this kind of problems have been coping.

> 
> > We will do it eventually, of course.
> > There will be additional work in llvm, libbpf, kernel, etc.
> > But for now I think it would be good to land Delyan's patch
> > to avoid unnecessary pain to all the users.
> > 
> > Peter, do you mind?
> 
> I suppose I can help out this time, but I really don't want to set a
> precedent for these things. Broken is broken.
> 
> The down-side for me is that the argument order no longer makes any
> sense.

I'm intending to backport fa2c3254d7cf to 5.10 and 5.15 but waiting for
a Tested-by. If you take this one, then it'll need to be backported too.

Cheers

--
Qais Yousef

Powered by blists - more mailing lists