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

From: Thomas Gleixner <tglx@linutronix.de>
To: Delyan Kratunov <delyank@fb.com>,
	"peterz@infradead.org" <peterz@infradead.org>
Cc: "dietmar.eggemann@arm.com" <dietmar.eggemann@arm.com>,
	"bigeasy@linutronix.de" <bigeasy@linutronix.de>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"qais.yousef@arm.com" <qais.yousef@arm.com>,
	Delyan Kratunov <delyank@fb.com>,
	"andrii@kernel.org" <andrii@kernel.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"vincent.guittot@linaro.org" <vincent.guittot@linaro.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"adharmap@quicinc.com" <adharmap@quicinc.com>,
	"alexei.starovoitov@gmail.com" <alexei.starovoitov@gmail.com>,
	"andrii.nakryiko@gmail.com" <andrii.nakryiko@gmail.com>,
	"ast@kernel.org" <ast@kernel.org>,
	"Kenta.Tada@sony.com" <Kenta.Tada@sony.com>,
	"rdunlap@infradead.org" <rdunlap@infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"bristot@redhat.com" <bristot@redhat.com>,
	"ebiederm@xmission.com" <ebiederm@xmission.com>,
	"mingo@kernel.org" <mingo@kernel.org>,
	"rostedt@goodmis.org" <rostedt@goodmis.org>,
	"ed.tsai@mediatek.com" <ed.tsai@mediatek.com>,
	"legion@kernel.org" <legion@kernel.org>,
	"acme@kernel.org" <acme@kernel.org>,
	"u.kleine-koenig@pengutronix.de" <u.kleine-koenig@pengutronix.de>,
	"valentin.schneider@arm.com" <valentin.schneider@arm.com>,
	"juri.lelli@redhat.com" <juri.lelli@redhat.com>,
	"namhyung@kernel.org" <namhyung@kernel.org>
Subject: Re: [PATCH v2] sched/tracing: append prev_state to tp args instead
Date: Thu, 12 May 2022 01:40:57 +0200	[thread overview]
Message-ID: <8735hfy6wm.ffs@tglx> (raw)
In-Reply-To: <c8a6930dfdd58a4a5755fc01732675472979732b.camel@fb.com>

On Wed, May 11 2022 at 18:28, Delyan Kratunov wrote:
> Commit fa2c3254d7cf (sched/tracing: Don't re-read p->state when emitting
> sched_switch event, 2022-01-20) added a new prev_state argument to the
> sched_switch tracepoint, before the prev task_struct pointer.
>
> This reordering of arguments broke BPF programs that use the raw
> tracepoint (e.g. tp_btf programs). The type of the second argument has
> changed and existing programs that assume a task_struct* argument
> (e.g. for bpf_task_storage access) will now fail to verify.
>
> If we instead append the new argument to the end, all existing programs
> would continue to work and can conditionally extract the prev_state
> argument on supported kernel versions.

If we instead? ... would continue to work?

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
  https://www.kernel.org/doc/html/latest/process/submitting-patches.html#submittingpatches

It's not too hard to actually follow documented rules. Something like
this:

  "Undo the reordering and move the new argument last, which ensures
   that all existing programs continue to work."

Hmm?

What's worse is that the changelog is missing a clear statement that
this is a one-time change which cannot be abused to turn tracepoints
into unmodifiable ABI as you stated in:

 https://lore.kernel.org/lkml/CAEf4BzaEo+dxSRJZHQiXYrj-a3_B-eODZUxGh3HrnPjquMYFXQ@mail.gmail.com

Thanks,

        tglx