Messages in this thread | | | Date | Tue, 26 Apr 2022 15:09:59 +0100 | From | Qais Yousef <> | 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@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.
> > > 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
| |