On Tue, May 11, 2021 at 9:17 PM Alexei Starovoitov <ast@xxxxxx> wrote: > > On 5/11/21 4:24 PM, Andrii Nakryiko wrote: > > On Fri, May 7, 2021 at 8:49 PM Alexei Starovoitov > > <alexei.starovoitov@xxxxxxxxx> wrote: > >> > >> From: Alexei Starovoitov <ast@xxxxxxxxxx> > >> > >> 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@xxxxxxxxxx> > >> --- > > > > 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.
