Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220120162520.570782-1-valentin.schneider@arm.com>
 <93a20759600c05b6d9e4359a1517c88e06b44834.camel@fb.com> <20220422110903.GW2731@worktop.programming.kicks-ass.net>
 <056e9bb0d0e3fc20572d42db7386face1d0665d6.camel@fb.com> <CAADnVQKsah4aka-LJ+X+5XHHESKbbw36D8fXTLqcYp2io3PN_w@mail.gmail.com>
 <YmflVPQlwpiBuxRc@hirez.programming.kicks-ass.net> <20220426140959.op6u5m7id57aq7yc@wubuntu>
In-Reply-To: <20220426140959.op6u5m7id57aq7yc@wubuntu>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Tue, 26 Apr 2022 08:54:17 -0700
Message-ID: <CAEf4BzaoL5HVc8U16kz7m--RiPhBwuLt8ZGZppwfxV85AXXrcw@mail.gmail.com>
Subject: Re: [PATCH] sched/tracing: append prev_state to tp args instead
To:     Qais Yousef <qais.yousef@arm.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        Delyan Kratunov <delyank@fb.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        "bigeasy@linutronix.de" <bigeasy@linutronix.de>,
        "dietmar.eggemann@arm.com" <dietmar.eggemann@arm.com>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "x86@kernel.org" <x86@kernel.org>,
        "andrii@kernel.org" <andrii@kernel.org>,
        "u.kleine-koenig@pengutronix.de" <u.kleine-koenig@pengutronix.de>,
        "vincent.guittot@linaro.org" <vincent.guittot@linaro.org>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "mingo@kernel.org" <mingo@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "rdunlap@infradead.org" <rdunlap@infradead.org>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "Kenta.Tada@sony.com" <Kenta.Tada@sony.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "bristot@redhat.com" <bristot@redhat.com>,
        "ebiederm@xmission.com" <ebiederm@xmission.com>,
        "ast@kernel.org" <ast@kernel.org>,
        "legion@kernel.org" <legion@kernel.org>,
        "adharmap@quicinc.com" <adharmap@quicinc.com>,
        "valentin.schneider@arm.com" <valentin.schneider@arm.com>,
        "ed.tsai@mediatek.com" <ed.tsai@mediatek.com>,
        "juri.lelli@redhat.com" <juri.lelli@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Apr 26, 2022 at 7:10 AM Qais Yousef <qais.yousef@arm.com> wrote:
>
> 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@fb.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.

See my reply to Peter. libbpf can't know user's intent to fail this
automatically, in general. In some cases when it can it does
accommodate this automatically. In other cases it provides instruments
for user to handle this (bpf_core_field_size(),
BPF_CORE_READ_BITFIELD(), etc).

But in the end no one eliminated the need for testing your application
for correctness. Tracing programs do break on kernel changes and BPF
users do adapt to them. Sometimes adapting is easy (like state ->
__state transition), sometimes it's much more involved (like this
argument order change).

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