Re: [PATCH v4 bpf-next 15/22] libbpf: Use fd_array only with gen_loader.

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <ast@fb.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	john fastabend <john.fastabend@gmail.com>,
	bpf <bpf@vger.kernel.org>, Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v4 bpf-next 15/22] libbpf: Use fd_array only with gen_loader.
Date: Wed, 12 May 2021 11:11:23 -0700	[thread overview]
Message-ID: <CAEf4BzZyFYT8=e-g5U8VRXt_h9pwxaUWzpjNtj9My7GEm4ozuQ@mail.gmail.com> (raw)
In-Reply-To: <8122c5c0-1429-9f2d-c73a-9d8ada4f318e@fb.com>

On Tue, May 11, 2021 at 9:17 PM Alexei Starovoitov <ast@fb.com> wrote:
>
> On 5/11/21 4:24 PM, Andrii Nakryiko wrote:
> > On Fri, May 7, 2021 at 8:49 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> >>
> >> From: Alexei Starovoitov <ast@kernel.org>
> >>
> >> Rely on fd_array kernel feature only to generate loader program,
> >> since it's mandatory for it.
> >> Avoid using fd_array by default to preserve test coverage
> >> for old style map_fd patching.
> >>
> >> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> >> ---
> >
> > As mentioned in the previous patch, this is almost a complete undo of
> > one of the earlier patches, but it also leaves FEAT_FD_IDX and
> > probe_kern_fd_idx() around. Can you please try to combine them?
>
> I cannot combine 9 and this 15, because then 14 will be broken.
> I'd have to combine all 3, but then the fd_idx won't get its own
> test coverage.

If you want to keep them separate, then adding `void *gen_loader` and
using that instead of kernel_supports() check would minimize code
churn. It will be auto-initialized to NULL and then in patch #14
you'll just define gen_loader with a proper type and initialization.

> With patches 1 through 9 I've tested the whole test_progs
> and that gives the confidence that kernel and libbpf side
> are doing it correctly.
> Then with the rest of patches and without 15 I test everything again.
> Such testing approach covers lskel and fd_idx together.
> I think this patch 15 is rather unnecessary. It's here only because
> you didn't like patch 9.
> I think patch 9 is a good default for libbpf to take.
> Eventually llvm can emit .o with fd_idx style.

When it does and it provides some benefits, we can switch. As I
mentioned previously, the current mechanism covers all the cases and
works fine without requiring extra code, so there is little reason to
switch the default to it.

> The libbpf would just call probe_kern_fd_idx() and won't need
> to massage the .text after llvm if kernel supports it.
> It would need to sanitize back to BPF_PSEUDO_MAP_FD only on older kernels.
> That's the reason I'm not deleting probe_kern_fd_idx() in this patch,
> because I think it will be used fairly soon.
> But I can delete it too if you insist.

Yes, please. We can always add it back when necessary. I'd like to
avoid dead code lying around.

> But combining 9,14,15 into one I'd like to avoid.

Sure, I agree, see above for a simple way to keep them separate
without code churn.

> I think there is a value to have this in git to easily go back later
> to test everything in fd_idx mode by reverting this patch 15.