2024-07-15 12:37 UTC+0100 ~ Tao Chen <chen.dylane@xxxxxxxxx> > Now, attach/detach tcx prog supported in libbpf, so we can add new > command 'bpftool attach/detach tcx' to attach tcx prog with bpftool > for user. > > # bpftool prog load tc_prog.bpf.o /sys/fs/bpf/tc_prog > # bpftool prog show > ... > 192: sched_cls name tc_prog tag 187aeb611ad00cfc gpl > loaded_at 2024-07-11T15:58:16+0800 uid 0 > xlated 152B jited 97B memlock 4096B map_ids 100,99,97 > btf_id 260 > # bpftool net attach tcxingress name tc_prog dev lo > # bpftool net > ... > tc: > lo(1) tcx/ingress tc_prog prog_id 29 > > # bpftool net detach tcxingress dev lo > # bpftool net > ... > tc: > # bpftool net attach tcxingress name tc_prog dev lo > # bpftool net > tc: > lo(1) tcx/ingress tc_prog prog_id 29 > > Test environment: ubuntu_22_04, 6.7.0-060700-generic > > Signed-off-by: Tao Chen <chen.dylane@xxxxxxxxx> > --- > tools/bpf/bpftool/net.c | 52 ++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c > index 968714b4c3d4..be7fd76202f1 100644 > --- a/tools/bpf/bpftool/net.c > +++ b/tools/bpf/bpftool/net.c > @@ -67,6 +67,8 @@ enum net_attach_type { > NET_ATTACH_TYPE_XDP_GENERIC, > NET_ATTACH_TYPE_XDP_DRIVER, > NET_ATTACH_TYPE_XDP_OFFLOAD, > + NET_ATTACH_TYPE_TCX_INGRESS, > + NET_ATTACH_TYPE_TCX_EGRESS, > }; > > static const char * const attach_type_strings[] = { > @@ -74,6 +76,8 @@ static const char * const attach_type_strings[] = { > [NET_ATTACH_TYPE_XDP_GENERIC] = "xdpgeneric", > [NET_ATTACH_TYPE_XDP_DRIVER] = "xdpdrv", > [NET_ATTACH_TYPE_XDP_OFFLOAD] = "xdpoffload", > + [NET_ATTACH_TYPE_TCX_INGRESS] = "tcxingress", > + [NET_ATTACH_TYPE_TCX_EGRESS] = "tcxegress", Hi, thanks for this work! I wonder whether "tcx_ingress" and "tcx_egress" might be more readable? I know we don't have underscores for XDP types but I'd be tempted to add it for the tcx types, what do you think? > }; > > static const char * const attach_loc_strings[] = { > @@ -647,6 +651,32 @@ static int do_attach_detach_xdp(int progfd, enum net_attach_type attach_type, > return bpf_xdp_attach(ifindex, progfd, flags, NULL); > } > > +static int get_tcx_type(enum net_attach_type attach_type) > +{ > + int type = 0; > + > + if (attach_type == NET_ATTACH_TYPE_TCX_INGRESS) > + type |= BPF_TCX_INGRESS; > + else if (attach_type == NET_ATTACH_TYPE_TCX_EGRESS) > + type |= BPF_TCX_EGRESS; Why the logical OR in this function? This seems to be copied from the XDP code, where we need to set flags. Here we just need the type, if I remember correctly, so we could have: switch (attach_type) { case (NET_ATTACH_TYPE_TCX_INGRESS): return BPF_TXC_INGRESS; case (NET_ATTACH_TYPE_TCX_EGRESS): return BPF_TCX_EGRESS; } (or if/else, works as well) which would be easier to understand in my opinion. > + > + return type; > +} > + > +static int do_attach_tcx(int progfd, enum net_attach_type attach_type, int ifindex) > +{ > + int type = get_tcx_type(attach_type); > + > + return bpf_prog_attach(progfd, ifindex, type, 0); > +} > + > +static int do_detach_tcx(int targetfd, enum net_attach_type attach_type) > +{ > + int type = get_tcx_type(attach_type); > + > + return bpf_prog_detach(targetfd, type); > +} > + > static int do_attach(int argc, char **argv) > { > enum net_attach_type attach_type; > @@ -694,6 +724,15 @@ static int do_attach(int argc, char **argv) > goto cleanup; > } > > + /* attach tcx prog */ > + if (is_prefix("tcx", attach_type_strings[attach_type])) > + err = do_attach_tcx(progfd, attach_type, ifindex); > + if (err) { > + p_err("interface %s attach failed: %s", > + attach_type_strings[attach_type], strerror(-err)); > + goto cleanup; > + } This introduces a second check on "err" in the function: if we attach an XDP program we'll try to attach then check "err" twice. Same for a TCX program, we'll check "err" before even trying to attach. I understand this replicates what we do for XDP, but I'm not sure the sequential calls to 'is_prefix("...")' is the cleanest approach. We should probably change the XDP case a bit and integrate with TCX better. Expanding the different attach types is more verbose, but remains the most straightforward way in my opinion. switch (attach_type) { case NET_ATTACH_TYPE_XDP: case NET_ATTACH_TYPE_XDP_GENERIC: case NET_ATTACH_TYPE_XDP_DRIVER: case NET_ATTACH_TYPE_XDP_OFFLOAD: err = do_attach_xdp(...); break; case NET_ATTACH_TYPE_TCX_INGRESS: case NET_ATTACH_TYPE_TCX_EGRESS: err = do_attach_tcx(...); break; } // Single check on "err" for both XDP and TCX here; // Or moving it to the switch statement if checks/error messages // needed to be different, but that's not the case in your patch if (err) { p_err(...); goto cleanup; } > + > if (json_output) > jsonw_null(json_wtr); > cleanup: > @@ -732,6 +771,16 @@ static int do_detach(int argc, char **argv) > return err; > } > > + /* detach tcx prog */ > + if (is_prefix("tcx", attach_type_strings[attach_type])) > + err = do_detach_tcx(ifindex, attach_type); > + > + if (err < 0) { > + p_err("interface %s detach failed: %s", > + attach_type_strings[attach_type], strerror(-err)); > + return err; > + } Same here. > + > if (json_output) > jsonw_null(json_wtr); > > @@ -928,7 +977,8 @@ static int do_help(int argc, char **argv) > " %1$s %2$s help\n" > "\n" > " " HELP_SPEC_PROGRAM "\n" > - " ATTACH_TYPE := { xdp | xdpgeneric | xdpdrv | xdpoffload }\n" > + " ATTACH_TYPE := { xdp | xdpgeneric | xdpdrv | xdpoffload | tcxingress\n" > + " | tcxegress}\n" Please use spaces only for indent inside of the string, and add a space before the ending '}'. > " " HELP_SPEC_OPTIONS " }\n" > "\n" > "Note: Only xdp, tcx, tc, netkit, flow_dissector and netfilter attachments\n"