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

Commit Message

Alexei Starovoitov May 8, 2021, 3:48 a.m. UTC

From: Alexei Starovoitov <[e-mail]>

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 <[e-mail]>
---
 tools/lib/bpf/libbpf.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

Comments

Andrii Nakryiko May 11, 2021, 11:24 p.m. UTC | #1

On Fri, May 7, 2021 at 8:49 PM Alexei Starovoitov
<[e-mail]> wrote:
>
> From: Alexei Starovoitov <[e-mail]>
>
> 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 <[e-mail]>
> ---

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?

>  tools/lib/bpf/libbpf.c | 24 ++++--------------------
>  1 file changed, 4 insertions(+), 20 deletions(-)
>

[...]

Alexei Starovoitov May 12, 2021, 4:17 a.m. UTC | #2

On 5/11/21 4:24 PM, Andrii Nakryiko wrote:
> On Fri, May 7, 2021 at 8:49 PM Alexei Starovoitov
> <[e-mail]> wrote:
>>
>> From: Alexei Starovoitov <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
> 
> 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.
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.
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.
But combining 9,14,15 into one I'd like to avoid.
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.

Andrii Nakryiko May 12, 2021, 6:11 p.m. UTC | #3

On Tue, May 11, 2021 at 9:17 PM Alexei Starovoitov <[e-mail]> wrote:
>
> On 5/11/21 4:24 PM, Andrii Nakryiko wrote:
> > On Fri, May 7, 2021 at 8:49 PM Alexei Starovoitov
> > <[e-mail]> wrote:
> >>
> >> From: Alexei Starovoitov <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >> ---
> >
> > 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.

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 0ca79712f850..24a659448782 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -291,7 +291,6 @@  struct bpf_program {
 	__u32 line_info_rec_size;
 	__u32 line_info_cnt;
 	__u32 prog_flags;
-	int *fd_array;
 };
 
 struct bpf_struct_ops {
@@ -6451,7 +6450,7 @@  bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
 
 		switch (relo->type) {
 		case RELO_LD64:
-			if (kernel_supports(obj, FEAT_FD_IDX)) {
+			if (obj->gen_loader) {
 				insn[0].src_reg = BPF_PSEUDO_MAP_IDX;
 				insn[0].imm = relo->map_idx;
 			} else {
@@ -6461,7 +6460,7 @@  bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
 			break;
 		case RELO_DATA:
 			insn[1].imm = insn[0].imm + relo->sym_off;
-			if (kernel_supports(obj, FEAT_FD_IDX)) {
+			if (obj->gen_loader) {
 				insn[0].src_reg = BPF_PSEUDO_MAP_IDX_VALUE;
 				insn[0].imm = relo->map_idx;
 			} else {
@@ -6472,7 +6471,7 @@  bpf_object__relocate_data(struct bpf_object *obj, struct bpf_program *prog)
 		case RELO_EXTERN_VAR:
 			ext = &obj->externs[relo->sym_off];
 			if (ext->type == EXT_KCFG) {
-				if (kernel_supports(obj, FEAT_FD_IDX)) {
+				if (obj->gen_loader) {
 					insn[0].src_reg = BPF_PSEUDO_MAP_IDX_VALUE;
 					insn[0].imm = obj->kconfig_map_idx;
 				} else {
@@ -7275,7 +7274,6 @@  load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
 	load_attr.attach_btf_id = prog->attach_btf_id;
 	load_attr.kern_version = kern_version;
 	load_attr.prog_ifindex = prog->prog_ifindex;
-	load_attr.fd_array = prog->fd_array;
 
 	/* specify func_info/line_info only if kernel supports them */
 	btf_fd = bpf_object__btf_fd(prog->obj);
@@ -7506,7 +7504,6 @@  static int
 bpf_object__load_progs(struct bpf_object *obj, int log_level)
 {
 	struct bpf_program *prog;
-	int *fd_array = NULL;
 	size_t i;
 	int err;
 
@@ -7517,14 +7514,6 @@  bpf_object__load_progs(struct bpf_object *obj, int log_level)
 			return err;
 	}
 
-	if (kernel_supports(obj, FEAT_FD_IDX) && obj->nr_maps) {
-		fd_array = malloc(sizeof(int) * obj->nr_maps);
-		if (!fd_array)
-			return -ENOMEM;
-		for (i = 0; i < obj->nr_maps; i++)
-			fd_array[i] = obj->maps[i].fd;
-	}
-
 	for (i = 0; i < obj->nr_programs; i++) {
 		prog = &obj->programs[i];
 		if (prog_is_subprog(obj, prog))
@@ -7534,17 +7523,12 @@  bpf_object__load_progs(struct bpf_object *obj, int log_level)
 			continue;
 		}
 		prog->log_level |= log_level;
-		prog->fd_array = fd_array;
 		err = bpf_program__load(prog, obj->license, obj->kern_version);
-		prog->fd_array = NULL;
-		if (err) {
-			free(fd_array);
+		if (err)
 			return err;
-		}
 	}
 	if (obj->gen_loader)
 		bpf_object__free_relocs(obj);
-	free(fd_array);
 	return 0;
 }