Re: [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii@kernel.org>,
	bpf@vger.kernel.org, ast@kernel.org,  daniel@iogearbox.net,
	martin.lau@kernel.org, kernel-team@meta.com
Subject: Re: [PATCH RFC bpf-next 1/3] bpf: revamp bpf_attr and name each command's field and substruct
Date: Thu, 25 May 2023 10:19:31 -0700	[thread overview]
Message-ID: <CAEf4Bzbe-D1PwWB7T4SCzNG3RKTMko_0h1TOiEmUrR22NPjfXg@mail.gmail.com> (raw)
In-Reply-To: <20230525031810.g42tmdk7ykjrkrcr@MacBook-Pro-8.local>

On Wed, May 24, 2023 at 8:18 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, May 24, 2023 at 02:02:41PM -0700, Andrii Nakryiko wrote:
> >
> > And there were a bunch of other similar changes. Please take a thorough
> > look and suggest more changes or which changes to drop. I'm not married
> > to any of them, it just felt like a good improvement.
>
> Agree that current layout sucks, but ...
>
> >  include/uapi/linux/bpf.h       | 235 +++++++++++++++++++++++++++------
> >  kernel/bpf/syscall.c           |  40 +++---
> >  tools/include/uapi/linux/bpf.h | 235 +++++++++++++++++++++++++++------
> >  3 files changed, 405 insertions(+), 105 deletions(-)
>
> ... the diff makes it worse. The diffstat for "nop" change is a red flag.

Only 100 lines are a real "nop" change to copy/paste existing fields
that are in unnamed fields. The rest is a value add.

I don't think the deal is in stats, though, right?

>
> > +     /*
> > +      * LEGACY anonymous substructs, for backwards compatibility.
> > +      * Each of the below anonymous substructs are ABI compatible with one
> > +      * of the above named substructs. Please use named substructs.
> > +      */
> > +
>
> All of them cannot be removed. This bagage will be a forever eyesore.
> Currently it's not pretty. The diffs make uapi file just ugly.
> Especially considering how 'named' and 'legacy' will start diverging.

We have to allow "divergence" (only in the sense that new fields only
go into named substructs, but the existing fields stay fixed, of
course), to avoid more naming conflicts. If that wasn't the case,
using struct_group() macro could have been used to avoid a copy/paste
of those anonymous field/struct copies.

So I'm not happy about those 100 lines copy paste of fixed fields
either, but at least that would get us out of the current global
naming namespace for PROG_LOAD, MAP_CREATE, BTF_LOAD, etc.

> New commands are thankfully named. We've learned the lesson,

Unfortunately, the problem is that unnamed commands are the ones that
are most likely to keep evolving.

> but prior mistake is unfixable. We have to live with it.

Ok, too bad, but it's fine. It was worth a try.

I tried to come up with something like struct_group() approach to
minimize code changes in UAPI header, but we have a more complicated
situation where part of struct has to be both anonymous and named,
while another part (newly added fields) should go only to named parts.
And that doesn't seem to be possible to support with a macro,
unfortunately.