在 2024/7/17 01:23, Quentin Monnet 写道:
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?
Hi，Quentin，thanks for your reply!
You are right, tcx_* looks more readable, i will change it in v2.
};
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.
It seems more reasonable, i will change it in v2.
+
+ 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;
}
My bad, i will add another patch to refactor this as you say.
+
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.
Got it.
+
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 '}'.
ok, i will fix this.
" " HELP_SPEC_OPTIONS " }\n"
"\n"
"Note: Only xdp, tcx, tc, netkit, flow_dissector and netfilter attachments\n"
--
Best Regards
Dylane Chen
