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

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Delyan Kratunov <delyank@fb.com>,
	"valentin.schneider@arm.com" <valentin.schneider@arm.com>,
	"bigeasy@linutronix.de" <bigeasy@linutronix.de>,
	"dietmar.eggemann@arm.com" <dietmar.eggemann@arm.com>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"andrii@kernel.org" <andrii@kernel.org>,
	"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>,
	"adharmap@quicinc.com" <adharmap@quicinc.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>,
	"ed.tsai@mediatek.com" <ed.tsai@mediatek.com>,
	"u.kleine-koenig@pengutronix.de" <u.kleine-koenig@pengutronix.de>,
	"juri.lelli@redhat.com" <juri.lelli@redhat.com>,
	"x86@kernel.org" <x86@kernel.org>
Subject: Re: [PATCH] sched/tracing: append prev_state to tp args instead
Date: Fri, 22 Apr 2022 09:37:54 -0700	[thread overview]
Message-ID: <CAEf4BzaEcjOC9gG6rCs95JK==ONV3aYhbQay9o9Liw8apE1HXQ@mail.gmail.com> (raw)
In-Reply-To: <20220422110903.GW2731@worktop.programming.kicks-ass.net>

On Fri, Apr 22, 2022 at 4:09 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Apr 21, 2022 at 10:12:25PM +0000, Delyan Kratunov wrote:
> > Hi folks,
> >
> > While working on bpf tooling, we noticed that the sched_switch tracepoint
> > signature recently changed in an incompatible manner. This affects the
> > runqslower tools in the kernel tree, as well as multiple libbpf tools in iovisor/bcc.
> >
> > It would be a fair amount of churn to fix all these tools, not to mention any
> > non-public tools people may be using.
>
> So on the one hand, too bad, deal with it. None of this is ABI.
>
> > If you are open to it, here's a
> > description and a patch that moves the new argument to the end,
> > so existing tools can continue working without change (the new argument
> > just won't be extracted in existing programs):
>
> And on the other hand; those users need to be fixed anyway, right?
> Accessing prev->__state is equally broken.
>
> Yes, the proposed hack is cute, but I have very little sympathy for any
> of this. These are in-kernel interfaces, they change, there must not be
> any impediment to change.
>
> If bpf wants to ride on them, it needs to suffer the pain of doing so.

Right, none of this is ABI and BPF users community is prepared to deal
with this, no one is claiming otherwise. And we did deal with __state
field rename already, see [0] for one example.

This function argument reordering is much harder to deal with in a
"contained" way like we did it for __state rename, unfortunately. So
given it doesn't have to require so much work for multiple people to
work around and is just a simple reordering of arguments to add a new
argument at the end of a function prototype, is there a big downside
to doing this?

  [0] https://github.com/iovisor/bcc/commit/8b350fd51b004d4eddd7caa410e274e2807904f9