Re: [PATCH] sched/tracing: append prev_state to tp args instead

[Peter Zijlstra]

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@xxxxxx> 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@xxxxxxxxxx/

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?

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