On 5/25/23 7:19 PM, Andrii Nakryiko wrote:
On Wed, May 24, 2023 at 8:18 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> 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.
Nice idea on the struct_group()-like approach, but agree that this is
going to be tough given we need to divert anonymous and named parts as
you mention. One other wild thought ... we remove the bpf_attr entirely
from the uapi header, and have a kernel/bpf/bpf.cmd description and
generate the bpf_attr into a uapi header via script which the main header
can include. Kind of similar to the suggestion, but more flexible than
macro magic. We also have things like syscall table header generated via
script.. so it wouldn't be first. Still doesn't remove the eyesore, just
packages it differently. ;/
Thanks,
Daniel
