[PATCH 0/2] tools: bpftool: add net (un)load command to load XDP

[PATCH 0/2] tools: bpftool: add net (un)load command to load XDP

[Daniel T. Lee]

Currently, bpftool net only supports dumping progs loaded on the
interface. To load XDP prog on interface, user must use other tool
(eg. iproute2). By this patch, with `bpftool net (un)load`, user can
(un)load XDP prog on interface.

    $ ./bpftool prog
    ...
    208: xdp  name xdp_prog1  tag ad822e38b629553f  gpl
      loaded_at 2019-07-28T18:03:11+0900  uid 0
    ...
    $ ./bpftool net load id 208 xdpdrv enp6s0np1
    $ ./bpftool net
    xdp:
    enp6s0np1(5) driver id 208
    ...
    $ ./bpftool net unload xdpdrv enp6s0np1
    $ ./bpftool net
    xdp:
    ...

The word 'load' is used instead of 'attach', since XDP program is not
considered as 'bpf_attach_type' and can't be attached with
'BPF_PROG_ATTACH'. In this context, the meaning of 'load' is, prog will
be loaded on interface.

While this patch only contains support for XDP, through `net (un)load`,
bpftool can further support other prog attach types.

XDP (un)load tested on Netronome Agilio.

Daniel T. Lee (2):
  tools: bpftool: add net load command to load XDP on interface
  tools: bpftool: add net unload command to unload XDP on interface

 tools/bpf/bpftool/net.c | 160 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 159 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH 1/2] tools: bpftool: add net load command to load XDP on interface

[Daniel T. Lee]

By this commit, using `bpftool net load`, user can load XDP prog on
interface. New type of enum 'net_load_type' has been made, as stated at
cover-letter, the meaning of 'load' is, prog will be loaded on interface.

BPF prog will be loaded through libbpf 'bpf_set_link_xdp_fd'.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 tools/bpf/bpftool/net.c | 107 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
index 67e99c56bc88..d3a4f18b5b95 100644
--- a/tools/bpf/bpftool/net.c
+++ b/tools/bpf/bpftool/net.c
@@ -55,6 +55,35 @@ struct bpf_attach_info {
 	__u32 flow_dissector_id;
 };
 
+enum net_load_type {
+	NET_LOAD_TYPE_XDP,
+	NET_LOAD_TYPE_XDP_GENERIC,
+	NET_LOAD_TYPE_XDP_DRIVE,
+	NET_LOAD_TYPE_XDP_OFFLOAD,
+	__MAX_NET_LOAD_TYPE
+};
+
+static const char * const load_type_strings[] = {
+	[NET_LOAD_TYPE_XDP] = "xdp",
+	[NET_LOAD_TYPE_XDP_GENERIC] = "xdpgeneric",
+	[NET_LOAD_TYPE_XDP_DRIVE] = "xdpdrv",
+	[NET_LOAD_TYPE_XDP_OFFLOAD] = "xdpoffload",
+	[__MAX_NET_LOAD_TYPE] = NULL,
+};
+
+static enum net_load_type parse_load_type(const char *str)
+{
+	enum net_load_type type;
+
+	for (type = 0; type < __MAX_NET_LOAD_TYPE; type++) {
+		if (load_type_strings[type] &&
+		   is_prefix(str, load_type_strings[type]))
+			return type;
+	}
+
+	return __MAX_NET_LOAD_TYPE;
+}
+
 static int dump_link_nlmsg(void *cookie, void *msg, struct nlattr **tb)
 {
 	struct bpf_netdev_t *netinfo = cookie;
@@ -223,6 +252,77 @@ static int query_flow_dissector(struct bpf_attach_info *attach_info)
 	return 0;
 }
 
+static int parse_load_args(int argc, char **argv, int *progfd,
+			   enum net_load_type *load_type, int *ifindex)
+{
+	if (!REQ_ARGS(3))
+		return -EINVAL;
+
+	*progfd = prog_parse_fd(&argc, &argv);
+	if (*progfd < 0)
+		return *progfd;
+
+	*load_type = parse_load_type(*argv);
+	if (*load_type == __MAX_NET_LOAD_TYPE) {
+		p_err("invalid net load/unload type");
+		return -EINVAL;
+	}
+
+	NEXT_ARG();
+	if (!REQ_ARGS(1))
+		return -EINVAL;
+
+	*ifindex = if_nametoindex(*argv);
+	if (!*ifindex) {
+		p_err("Invalid ifname");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int do_load_unload_xdp(int *progfd, enum net_load_type *load_type,
+			      int *ifindex)
+{
+	__u32 flags;
+	int err;
+
+	flags = XDP_FLAGS_UPDATE_IF_NOEXIST;
+	if (*load_type == NET_LOAD_TYPE_XDP_GENERIC)
+		flags |= XDP_FLAGS_SKB_MODE;
+	if (*load_type == NET_LOAD_TYPE_XDP_DRIVE)
+		flags |= XDP_FLAGS_DRV_MODE;
+	if (*load_type == NET_LOAD_TYPE_XDP_OFFLOAD)
+		flags |= XDP_FLAGS_HW_MODE;
+
+	err = bpf_set_link_xdp_fd(*ifindex, *progfd, flags);
+
+	return err;
+}
+
+static int do_load(int argc, char **argv)
+{
+	enum net_load_type load_type;
+	int err, progfd, ifindex;
+
+	err = parse_load_args(argc, argv, &progfd, &load_type, &ifindex);
+	if (err)
+		return err;
+
+	if (is_prefix("xdp", load_type_strings[load_type]))
+		err = do_load_unload_xdp(&progfd, &load_type, &ifindex);
+
+	if (err < 0) {
+		p_err("link set %s failed", load_type_strings[load_type]);
+		return -1;
+	}
+
+	if (json_output)
+		jsonw_null(json_wtr);
+
+	return 0;
+}
+
 static int do_show(int argc, char **argv)
 {
 	struct bpf_attach_info attach_info = {};
@@ -305,13 +405,17 @@ static int do_help(int argc, char **argv)
 
 	fprintf(stderr,
 		"Usage: %s %s { show | list } [dev <devname>]\n"
+		"       %s %s load PROG LOAD_TYPE <devname>\n"
 		"       %s %s help\n"
+		"\n"
+		"       " HELP_SPEC_PROGRAM "\n"
+		"       LOAD_TYPE := { xdp | xdpgeneric | xdpdrv | xdpoffload }\n"
 		"Note: Only xdp and tc attachments are supported now.\n"
 		"      For progs attached to cgroups, use \"bpftool cgroup\"\n"
 		"      to dump program attachments. For program types\n"
 		"      sk_{filter,skb,msg,reuseport} and lwt/seg6, please\n"
 		"      consult iproute2.\n",
-		bin_name, argv[-2], bin_name, argv[-2]);
+		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
 
 	return 0;
 }
@@ -319,6 +423,7 @@ static int do_help(int argc, char **argv)
 static const struct cmd cmds[] = {
 	{ "show",	do_show },
 	{ "list",	do_show },
+	{ "load",	do_load },
 	{ "help",	do_help },
 	{ 0 }
 };
-- 
2.20.1

[PATCH 2/2] tools: bpftool: add net unload command to unload XDP on interface

[Daniel T. Lee]

By this commit, using `bpftool net unload`, the loaded XDP prog can
be unloaded. Unloading the BPF prog will be done through libbpf
'bpf_set_link_xdp_fd' with the progfd set to -1.

Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
---
 tools/bpf/bpftool/net.c | 55 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
index d3a4f18b5b95..9d353b6e7d6d 100644
--- a/tools/bpf/bpftool/net.c
+++ b/tools/bpf/bpftool/net.c
@@ -281,6 +281,31 @@ static int parse_load_args(int argc, char **argv, int *progfd,
 	return 0;
 }
 
+static int parse_unload_args(int argc, char **argv,
+			     enum net_load_type *load_type, int *ifindex)
+{
+	if (!REQ_ARGS(2))
+		return -EINVAL;
+
+	*load_type = parse_load_type(*argv);
+	if (*load_type == __MAX_NET_LOAD_TYPE) {
+		p_err("invalid net load/unload type");
+		return -EINVAL;
+	}
+
+	NEXT_ARG();
+	if (!REQ_ARGS(1))
+		return -EINVAL;
+
+	*ifindex = if_nametoindex(*argv);
+	if (!*ifindex) {
+		p_err("Invalid ifname");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int do_load_unload_xdp(int *progfd, enum net_load_type *load_type,
 			      int *ifindex)
 {
@@ -323,6 +348,31 @@ static int do_load(int argc, char **argv)
 	return 0;
 }
 
+static int do_unload(int argc, char **argv)
+{
+	enum net_load_type load_type;
+	int err, progfd, ifindex;
+
+	err = parse_unload_args(argc, argv, &load_type, &ifindex);
+	if (err)
+		return err;
+
+	/* to unload xdp prog */
+	progfd = -1;
+	if (is_prefix("xdp", load_type_strings[load_type]))
+		err = do_load_unload_xdp(&progfd, &load_type, &ifindex);
+
+	if (err < 0) {
+		p_err("link set %s failed", load_type_strings[load_type]);
+		return -1;
+	}
+
+	if (json_output)
+		jsonw_null(json_wtr);
+
+	return 0;
+}
+
 static int do_show(int argc, char **argv)
 {
 	struct bpf_attach_info attach_info = {};
@@ -406,6 +456,7 @@ static int do_help(int argc, char **argv)
 	fprintf(stderr,
 		"Usage: %s %s { show | list } [dev <devname>]\n"
 		"       %s %s load PROG LOAD_TYPE <devname>\n"
+		"       %s %s unload LOAD_TYPE <devname>\n"
 		"       %s %s help\n"
 		"\n"
 		"       " HELP_SPEC_PROGRAM "\n"
@@ -415,7 +466,8 @@ static int do_help(int argc, char **argv)
 		"      to dump program attachments. For program types\n"
 		"      sk_{filter,skb,msg,reuseport} and lwt/seg6, please\n"
 		"      consult iproute2.\n",
-		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2]);
+		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
+		bin_name, argv[-2]);
 
 	return 0;
 }
@@ -424,6 +476,7 @@ static const struct cmd cmds[] = {
 	{ "show",	do_show },
 	{ "list",	do_show },
 	{ "load",	do_load },
+	{ "unload",	do_unload },
 	{ "help",	do_help },
 	{ 0 }
 };
-- 
2.20.1

Re: [PATCH 0/2] tools: bpftool: add net (un)load command to load XDP

[Andrii Nakryiko]

On Tue, Jul 30, 2019 at 11:48 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
>
> Currently, bpftool net only supports dumping progs loaded on the
> interface. To load XDP prog on interface, user must use other tool
> (eg. iproute2). By this patch, with `bpftool net (un)load`, user can
> (un)load XDP prog on interface.
>
>     $ ./bpftool prog
>     ...
>     208: xdp  name xdp_prog1  tag ad822e38b629553f  gpl
>       loaded_at 2019-07-28T18:03:11+0900  uid 0
>     ...
>     $ ./bpftool net load id 208 xdpdrv enp6s0np1
>     $ ./bpftool net
>     xdp:
>     enp6s0np1(5) driver id 208
>     ...
>     $ ./bpftool net unload xdpdrv enp6s0np1
>     $ ./bpftool net
>     xdp:
>     ...
>
> The word 'load' is used instead of 'attach', since XDP program is not

Let's be consistent and "attach" everything. Tracing BPF programs
(kprobe, tracepoints) are also not attached with BPF_PROG_ATTACH, but
it's still a process of attaching BPF program to a BPF hook (a point
in the system which can execute BPF program).

Even better, if you can check what we recently did for tracing APIs
with struct bpf_link and can add similar APIs for XDP programs, it
would be a nice step towards consistency of APIs (and terminology as
well). Please consider doing that. Thanks!

> considered as 'bpf_attach_type' and can't be attached with
> 'BPF_PROG_ATTACH'. In this context, the meaning of 'load' is, prog will
> be loaded on interface.
>
> While this patch only contains support for XDP, through `net (un)load`,
> bpftool can further support other prog attach types.
>
> XDP (un)load tested on Netronome Agilio.
>
> Daniel T. Lee (2):
>   tools: bpftool: add net load command to load XDP on interface
>   tools: bpftool: add net unload command to unload XDP on interface
>
>  tools/bpf/bpftool/net.c | 160 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 159 insertions(+), 1 deletion(-)
>
> --
> 2.20.1
>

Re: [PATCH 0/2] tools: bpftool: add net (un)load command to load XDP

[Jakub Kicinski]

On Wed, 31 Jul 2019 03:48:19 +0900, Daniel T. Lee wrote:
> Currently, bpftool net only supports dumping progs loaded on the
> interface. To load XDP prog on interface, user must use other tool
> (eg. iproute2). By this patch, with `bpftool net (un)load`, user can
> (un)load XDP prog on interface.

I don't understand why using another tool is a bad thing :(
What happened to the Unix philosophy?

I remain opposed to duplicating iproute2's functionality under 
bpftool net :( The way to attach bpf programs in the networking
subsystem is through the iproute2 commends - ip and tc.. 

It seems easy enough to add a feature to bpftool but from 
a perspective of someone adding a new feature to the kernel, 
and wanting to update user space components it's quite painful :(

So could you describe to me in more detail why this is a good idea?
Perhaps others can chime in?

>     $ ./bpftool prog
>     ...
>     208: xdp  name xdp_prog1  tag ad822e38b629553f  gpl
>       loaded_at 2019-07-28T18:03:11+0900  uid 0
>     ...
>     $ ./bpftool net load id 208 xdpdrv enp6s0np1
>     $ ./bpftool net
>     xdp:
>     enp6s0np1(5) driver id 208
>     ...
>     $ ./bpftool net unload xdpdrv enp6s0np1
>     $ ./bpftool net
>     xdp:
>     ...
> 
> The word 'load' is used instead of 'attach', since XDP program is not
> considered as 'bpf_attach_type' and can't be attached with
> 'BPF_PROG_ATTACH'. In this context, the meaning of 'load' is, prog will
> be loaded on interface.
> 
> While this patch only contains support for XDP, through `net (un)load`,
> bpftool can further support other prog attach types.
> 
> XDP (un)load tested on Netronome Agilio.

Re: [PATCH 0/2] tools: bpftool: add net (un)load command to load XDP

[Alexei Starovoitov]

On Tue, Jul 30, 2019 at 03:59:15PM -0700, Jakub Kicinski wrote:
> On Wed, 31 Jul 2019 03:48:19 +0900, Daniel T. Lee wrote:
> > Currently, bpftool net only supports dumping progs loaded on the
> > interface. To load XDP prog on interface, user must use other tool
> > (eg. iproute2). By this patch, with `bpftool net (un)load`, user can
> > (un)load XDP prog on interface.
> 
> I don't understand why using another tool is a bad thing :(
> What happened to the Unix philosophy?
> 
> I remain opposed to duplicating iproute2's functionality under 
> bpftool net :( The way to attach bpf programs in the networking
> subsystem is through the iproute2 commends - ip and tc.. 
> 
> It seems easy enough to add a feature to bpftool but from 
> a perspective of someone adding a new feature to the kernel, 
> and wanting to update user space components it's quite painful :(
> 
> So could you describe to me in more detail why this is a good idea?
> Perhaps others can chime in?

I don't think it has anything to do with 'unix philosophy'.
Here the proposal to teach bpftool to attach xdp progs.
I see nothing wrong with that.
Another reason is iproute2 is still far away from adopting libbpf.
So all the latest goodness like BTF, introspection, etc will not
be available to iproute2 users for some time.
Even when iproute2 is ready it would be convenient for folks like me
(who need to debug stuff in production) to remember cmd line of
bpftool only to introspect the server. Debugging often includes
detaching/attaching progs. Not only doing 'bpftool p s'.

If bpftool was taught to do equivalent of 'ip link' that would be
very different story and I would be opposed to that.

Re: [PATCH 0/2] tools: bpftool: add net (un)load command to load XDP

[Jakub Kicinski]

On Tue, 30 Jul 2019 16:17:56 -0700, Alexei Starovoitov wrote:
> On Tue, Jul 30, 2019 at 03:59:15PM -0700, Jakub Kicinski wrote:
> > On Wed, 31 Jul 2019 03:48:19 +0900, Daniel T. Lee wrote:  
> > > Currently, bpftool net only supports dumping progs loaded on the
> > > interface. To load XDP prog on interface, user must use other tool
> > > (eg. iproute2). By this patch, with `bpftool net (un)load`, user can
> > > (un)load XDP prog on interface.  
> > 
> > I don't understand why using another tool is a bad thing :(
> > What happened to the Unix philosophy?
> > 
> > I remain opposed to duplicating iproute2's functionality under 
> > bpftool net :( The way to attach bpf programs in the networking
> > subsystem is through the iproute2 commends - ip and tc.. 
> > 
> > It seems easy enough to add a feature to bpftool but from 
> > a perspective of someone adding a new feature to the kernel, 
> > and wanting to update user space components it's quite painful :(
> > 
> > So could you describe to me in more detail why this is a good idea?
> > Perhaps others can chime in?  
> 
> I don't think it has anything to do with 'unix philosophy'.
> Here the proposal to teach bpftool to attach xdp progs.
> I see nothing wrong with that.

Nothing meaning you disagree it's duplicated effort and unnecessary 
LoC the community has to maintain, review, test..?

> Another reason is iproute2 is still far away from adopting libbpf.
> So all the latest goodness like BTF, introspection, etc will not
> be available to iproute2 users for some time.

Duplicating the same features in bpftool will only diminish the
incentive for moving iproute2 to libbpf. And for folks who deal
with a wide variety of customers, often novices maintaining two
ways of doing the same thing is a hassle :(

> Even when iproute2 is ready it would be convenient for folks like me
> (who need to debug stuff in production) to remember cmd line of
> bpftool only to introspect the server. Debugging often includes
> detaching/attaching progs. Not only doing 'bpftool p s'.

Let's just put the two commands next to each other:

       ip link set xdp $PROG dev $DEV

bpftool net attach xdp $PROG dev $DEV

Are they that different?

> If bpftool was taught to do equivalent of 'ip link' that would be
> very different story and I would be opposed to that.

Yes, that'd be pretty clear cut, only the XDP stuff is a bit more 
of a judgement call.

Re: [PATCH 0/2] tools: bpftool: add net (un)load command to load XDP

[Alexei Starovoitov]

On Tue, Jul 30, 2019 at 05:07:25PM -0700, Jakub Kicinski wrote:
> On Tue, 30 Jul 2019 16:17:56 -0700, Alexei Starovoitov wrote:
> > On Tue, Jul 30, 2019 at 03:59:15PM -0700, Jakub Kicinski wrote:
> > > On Wed, 31 Jul 2019 03:48:19 +0900, Daniel T. Lee wrote:  
> > > > Currently, bpftool net only supports dumping progs loaded on the
> > > > interface. To load XDP prog on interface, user must use other tool
> > > > (eg. iproute2). By this patch, with `bpftool net (un)load`, user can
> > > > (un)load XDP prog on interface.  
> > > 
> > > I don't understand why using another tool is a bad thing :(
> > > What happened to the Unix philosophy?
> > > 
> > > I remain opposed to duplicating iproute2's functionality under 
> > > bpftool net :( The way to attach bpf programs in the networking
> > > subsystem is through the iproute2 commends - ip and tc.. 
> > > 
> > > It seems easy enough to add a feature to bpftool but from 
> > > a perspective of someone adding a new feature to the kernel, 
> > > and wanting to update user space components it's quite painful :(
> > > 
> > > So could you describe to me in more detail why this is a good idea?
> > > Perhaps others can chime in?  
> > 
> > I don't think it has anything to do with 'unix philosophy'.
> > Here the proposal to teach bpftool to attach xdp progs.
> > I see nothing wrong with that.
> 
> Nothing meaning you disagree it's duplicated effort and unnecessary 
> LoC the community has to maintain, review, test..?

I don't see duplicated effort.

> > Another reason is iproute2 is still far away from adopting libbpf.
> > So all the latest goodness like BTF, introspection, etc will not
> > be available to iproute2 users for some time.
> 
> Duplicating the same features in bpftool will only diminish the
> incentive for moving iproute2 to libbpf. 

not at all. why do you think so?

> And for folks who deal
> with a wide variety of customers, often novices maintaining two
> ways of doing the same thing is a hassle :(

It's not the same thing.
I'm talking about my experience dealing with 'wide variety of bpf customers'.
They only have a fraction of their time to learn one tool.
Making every bpf customer learn ten tools is not an option.

> > Even when iproute2 is ready it would be convenient for folks like me
> > (who need to debug stuff in production) to remember cmd line of
> > bpftool only to introspect the server. Debugging often includes
> > detaching/attaching progs. Not only doing 'bpftool p s'.
> 
> Let's just put the two commands next to each other:
> 
>        ip link set xdp $PROG dev $DEV
> 
> bpftool net attach xdp $PROG dev $DEV
> 
> Are they that different?

yes.
they're different tools with they own upgrade/rollout cycle

> 
> > If bpftool was taught to do equivalent of 'ip link' that would be
> > very different story and I would be opposed to that.
> 
> Yes, that'd be pretty clear cut, only the XDP stuff is a bit more 
> of a judgement call.

bpftool must be able to introspect every aspect of bpf programming.
That includes detaching and attaching anywhere.
Anyone doing 'bpftool p s' should be able to switch off particular
prog id without learning ten different other tools.
iproute2 is a small bit of it. There is cgroup and tracing too.
bpftool should be one tool to do everything directly related to bpf.

Re: [PATCH 0/2] tools: bpftool: add net (un)load command to load XDP

[Jakub Kicinski]

On Tue, 30 Jul 2019 17:23:39 -0700, Alexei Starovoitov wrote:
> On Tue, Jul 30, 2019 at 05:07:25PM -0700, Jakub Kicinski wrote:
> > Nothing meaning you disagree it's duplicated effort and unnecessary 
> > LoC the community has to maintain, review, test..?  
> 
> I don't see duplicated effort.

Could you walk me through a scenario where, say, a new xdp attachment
flag is added? How can there be support in both bpftool and iproute2 
for specifying it without modifying both?

How can we say we want to make iproute2 use libbpf instead of it's own
library to avoid duplicated effort on the back end, and at the same time
claim having two tools do the same thing is no duplication 🤯

> > Duplicating the same features in bpftool will only diminish the
> > incentive for moving iproute2 to libbpf.   
> 
> not at all. why do you think so?

Because iproute2's BPF has fallen behind so the simplest thing is to
just contribute to bpftool. But iproute2 is the tool set for Linux
networking, we can't let it bit rot :(

Admittedly that's just me reading the tea leaves.

> > And for folks who deal
> > with a wide variety of customers, often novices maintaining two
> > ways of doing the same thing is a hassle :(  
> 
> It's not the same thing.
> I'm talking about my experience dealing with 'wide variety of bpf customers'.
> They only have a fraction of their time to learn one tool.
> Making every bpf customer learn ten tools is not an option.

IMHO vaguely competent users of Linux networking will know ip link.
If they are not vaguely competent, they should not attach XDP programs
to interfaces by hand...

> > Let's just put the two commands next to each other:
> > 
> >        ip link set xdp $PROG dev $DEV
> > 
> > bpftool net attach xdp $PROG dev $DEV
> > 
> > Are they that different?  
> 
> yes.
> they're different tools with they own upgrade/rollout cycle

I think this came up when bpftool net was added and I never really
understood it.

> > > If bpftool was taught to do equivalent of 'ip link' that would be
> > > very different story and I would be opposed to that.  
> > 
> > Yes, that'd be pretty clear cut, only the XDP stuff is a bit more 
> > of a judgement call.  
> 
> bpftool must be able to introspect every aspect of bpf programming.
> That includes detaching and attaching anywhere.
> Anyone doing 'bpftool p s' should be able to switch off particular
> prog id without learning ten different other tools.

I think the fact that we already have an implementation in iproute2,
which is at the risk of bit rot is more important to me that the
hypothetical scenario where everyone knows to just use bpftool (for
XDP, for TC it's still iproute2 unless there's someone crazy enough 
to reimplement the TC functionality :))

I'm not sure we can settle our differences over email :)
I have tremendous respect for all the maintainers I CCed here, 
if nobody steps up to agree with me I'll concede the bpftool net
battle entirely :)

Re: [PATCH 0/2] tools: bpftool: add net (un)load command to load XDP

[Alexei Starovoitov]

On Tue, Jul 30, 2019 at 06:21:44PM -0700, Jakub Kicinski wrote:
> > > Duplicating the same features in bpftool will only diminish the
> > > incentive for moving iproute2 to libbpf.   
> > 
> > not at all. why do you think so?
> 
> Because iproute2's BPF has fallen behind so the simplest thing is to
> just contribute to bpftool. But iproute2 is the tool set for Linux
> networking, we can't let it bit rot :(

where were you when a lot of libbpf was copy pasted into iproute2 ?!
Now it diverged a lot and it's difficult to move iproute2 back to the main
train which is libbpf.
Same thing with at least 5 copy-pastes of samples/bpf/bpf_load.c
that spawned a bunch of different bpf loaders.

> IMHO vaguely competent users of Linux networking will know ip link.
> If they are not vaguely competent, they should not attach XDP programs
> to interfaces by hand...

I'm a prime example of moderately competent linux user who
doesn't know iproute2. I'm not joking.
I don't know tc syntax and always use my cheat sheet of ip/tc commands
to do anything.

> > 
> > bpftool must be able to introspect every aspect of bpf programming.
> > That includes detaching and attaching anywhere.
> > Anyone doing 'bpftool p s' should be able to switch off particular
> > prog id without learning ten different other tools.
> 
> I think the fact that we already have an implementation in iproute2,
> which is at the risk of bit rot is more important to me that the
> hypothetical scenario where everyone knows to just use bpftool (for
> XDP, for TC it's still iproute2 unless there's someone crazy enough 
> to reimplement the TC functionality :))

I think you're missing the point that iproute2 is still necessary
to configure it.
bpftool should be able to attach/detach from anything.
But configuring that thing (whether it's cgroup or tc/xdp) is
a job of corresponding apis and tools.

> I'm not sure we can settle our differences over email :)
> I have tremendous respect for all the maintainers I CCed here, 
> if nobody steps up to agree with me I'll concede the bpftool net
> battle entirely :)

we can keep arguing forever. Respect to ease-of-use only comes
from the pain of operational experience. I don't think I can
convey that pain in the email either.

Re: [PATCH 0/2] tools: bpftool: add net (un)load command to load XDP

[Jesper Dangaard Brouer]

On Tue, 30 Jul 2019 18:52:40 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Tue, Jul 30, 2019 at 06:21:44PM -0700, Jakub Kicinski wrote:
> > > > Duplicating the same features in bpftool will only diminish the
> > > > incentive for moving iproute2 to libbpf.     
> > > 
> > > not at all. why do you think so?  
> > 
> > Because iproute2's BPF has fallen behind so the simplest thing is to
> > just contribute to bpftool. But iproute2 is the tool set for Linux
> > networking, we can't let it bit rot :(  
> 
> where were you when a lot of libbpf was copy pasted into iproute2 ?!
> Now it diverged a lot and it's difficult to move iproute2 back to the main
> train which is libbpf.

I hope we all agree that libbpf it the way forward.  I really hope we
can convert iproute2 into using libbpf.  It is challenging because
iproute2 ELF files use another map-layout, and I guess we need to stay
backward compatible.

> Same thing with at least 5 copy-pastes of samples/bpf/bpf_load.c
> that spawned a bunch of different bpf loaders.

I'm also to blame here... as I ran with bpf_load.c and based my
prototype-kernel github project on it.  And it seems that it have
gotten enough Google entropy, that people find that first. E.g. I have
pull requests that add new tools, which I have not merged, as I want to
switch to libbpf first.

Recently I've added a README[1] that warn about this issue, and point
people to xdp-tutorial[2] instead.

[1] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/README.rst
[2] https://github.com/xdp-project/xdp-tutorial

 Quote[1]:
   "WARNING: This directory contains its own out-of-date BPF
    ELF-loader (in bpf_load.c). Instead people should use libbpf."

> > IMHO vaguely competent users of Linux networking will know ip link.
> > If they are not vaguely competent, they should not attach XDP programs
> > to interfaces by hand...  
> 
> I'm a prime example of moderately competent linux user who
> doesn't know iproute2. I'm not joking.
> I don't know tc syntax and always use my cheat sheet of ip/tc commands
> to do anything.

I agree... I also need to lookup the TC commands used for attaching BPF
programs every time.  It is a pain, and I've often made mistakes that
cause several tc filter's to get attached, which leads to strange
behavior.  (AFAIK you have to remember to use both prio and handle when
removing the old, before adding the new).

 
> > > 
> > > bpftool must be able to introspect every aspect of bpf programming.
> > > That includes detaching and attaching anywhere.
> > > Anyone doing 'bpftool p s' should be able to switch off particular
> > > prog id without learning ten different other tools.  
> > 
> > I think the fact that we already have an implementation in iproute2,
> > which is at the risk of bit rot is more important to me that the
> > hypothetical scenario where everyone knows to just use bpftool (for
> > XDP, for TC it's still iproute2 unless there's someone crazy enough 
> > to reimplement the TC functionality :))  
> 
> I think you're missing the point that iproute2 is still necessary
> to configure it.
> bpftool should be able to attach/detach from anything.
> But configuring that thing (whether it's cgroup or tc/xdp) is
> a job of corresponding apis and tools.
> 
> > I'm not sure we can settle our differences over email :)
> > I have tremendous respect for all the maintainers I CCed here, 
> > if nobody steps up to agree with me I'll concede the bpftool net
> > battle entirely :)  
> 
> we can keep arguing forever. Respect to ease-of-use only comes
> from the pain of operational experience. I don't think I can
> convey that pain in the email either.

Let me try to convey some pain...

Regarding operational experience.  I have a customer using this:
 https://github.com/xdp-project/xdp-cpumap-tc

The project combines TC and XDP (with CPUMAP redirect).  It has been a
*HUGE* pain that we needed to use iproute2 tc commands to attach and
load TC BPF-programs.

(Issue#1)
The iproute2 tc BPF loader uses another ELF-layout for maps.  This was
worked around, by keeping XDP and TC BPF-progs in different C-file
using different struct's for maps.  And avoiding to use map features,
that iproute2 doesn't know about... although we found a workaround for
using more advanced map features via load-order and pinning see issue#3
sub-problem(2).

(Issue#2)
As this is a C-prog I would really prefer using a library, instead of
having to call cmdline utilities, see C-code doing this[3].  This lead
to several issues.  E.g. they had installed too old version of iproute2
on their systems, after installing newer version (in /usr/local) it was
not first in $PATH for root.  Load order also matters see issue#3.

[3] https://github.com/xdp-project/xdp-cpumap-tc/blob/master/src/common_user.c#L381-L493

(Issue#3)
Next to get TC and XDP programs to cooperate, pinned maps via bpffs is
used.  And iproute2 dictates that pinned maps are located in directory
/sys/fs/bpf/tc/globals/.  What could go wrong, it's only a static dir path.

Sub-problem(1): If XDP loads _before_ any tc-bpf cmd, then the subdirs
are not created, leading to replicating mkdir creation in C[4].  Else
the XDP load will fail.  (Troubleshooting this was complicated by 

[4] https://github.com/xdp-project/xdp-cpumap-tc/commit/25e7e56699cd75a4a

Sub-problem(2): We really want to load XDP first, because libbpf
creates maps in a better way, e.g. with "name" (and BTF info).
The "name" part was needed by libbpf to find a given map (to avoid
depending on the order maps are defined in C/ELF file) via helper
bpf_object__find_map_fd_by_name().  To handle TC noname case, I had to
code up this workaround[5], which depend on extracting the name of the
pinned file name.

[5] https://github.com/xdp-project/xdp-cpumap-tc/blob/master/src/xdp_iphash_to_cpu_user.c#L117-L131


Experience/conclusion: Getting XDP and TC to cooperate suck, primarily
because iproute2 tc is based on a separate BPF-ELF loader, which is
features are not in sync / lacking behind.  Calling cmdline binaries
from C also sucks, and I would prefer some libbpf TC attach function,
but most pain comes from the slight differences between ELF-loaders.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH 1/2] tools: bpftool: add net load command to load XDP on interface

[Jesper Dangaard Brouer]

On Wed, 31 Jul 2019 03:48:20 +0900
"Daniel T. Lee" <danieltimlee@gmail.com> wrote:

> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
> index 67e99c56bc88..d3a4f18b5b95 100644
> --- a/tools/bpf/bpftool/net.c
> +++ b/tools/bpf/bpftool/net.c
> @@ -55,6 +55,35 @@ struct bpf_attach_info {
>  	__u32 flow_dissector_id;
>  };
>  
> +enum net_load_type {
> +	NET_LOAD_TYPE_XDP,
> +	NET_LOAD_TYPE_XDP_GENERIC,
> +	NET_LOAD_TYPE_XDP_DRIVE,

Why "DRIVE" ?
Why not "DRIVER" ?

> +	NET_LOAD_TYPE_XDP_OFFLOAD,
> +	__MAX_NET_LOAD_TYPE
> +};
> +
> +static const char * const load_type_strings[] = {
> +	[NET_LOAD_TYPE_XDP] = "xdp",
> +	[NET_LOAD_TYPE_XDP_GENERIC] = "xdpgeneric",
> +	[NET_LOAD_TYPE_XDP_DRIVE] = "xdpdrv",
> +	[NET_LOAD_TYPE_XDP_OFFLOAD] = "xdpoffload",
> +	[__MAX_NET_LOAD_TYPE] = NULL,
> +};



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH 1/2] tools: bpftool: add net load command to load XDP on interface

[Jesper Dangaard Brouer]

On Wed, 31 Jul 2019 03:48:20 +0900
"Daniel T. Lee" <danieltimlee@gmail.com> wrote:

> By this commit, using `bpftool net load`, user can load XDP prog on
> interface. New type of enum 'net_load_type' has been made, as stated at
> cover-letter, the meaning of 'load' is, prog will be loaded on interface.

Why the keyword "load" ?
Why not "attach" (and "detach")?

For BPF there is a clear distinction between the "load" and "attach"
steps.  I know this is under subcommand "net", but to follow the
conversion of other subcommands e.g. "prog" there are both "load" and
"attach" commands.


> BPF prog will be loaded through libbpf 'bpf_set_link_xdp_fd'.

Again this is a "set" operation, not a "load" operation.

> Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>

[...]
>  static int do_show(int argc, char **argv)
>  {
>  	struct bpf_attach_info attach_info = {};
> @@ -305,13 +405,17 @@ static int do_help(int argc, char **argv)
>  
>  	fprintf(stderr,
>  		"Usage: %s %s { show | list } [dev <devname>]\n"
> +		"       %s %s load PROG LOAD_TYPE <devname>\n"

The "PROG" here does it correspond to the 'bpftool prog' syntax?:

 PROG := { id PROG_ID | pinned FILE | tag PROG_TAG }

>  		"       %s %s help\n"
> +		"\n"
> +		"       " HELP_SPEC_PROGRAM "\n"
> +		"       LOAD_TYPE := { xdp | xdpgeneric | xdpdrv | xdpoffload }\n"
>  		"Note: Only xdp and tc attachments are supported now.\n"
>  		"      For progs attached to cgroups, use \"bpftool cgroup\"\n"
>  		"      to dump program attachments. For program types\n"
>  		"      sk_{filter,skb,msg,reuseport} and lwt/seg6, please\n"
>  		"      consult iproute2.\n",


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH 1/2] tools: bpftool: add net load command to load XDP on interface

[Daniel T. Lee]

On Wed, Jul 31, 2019 at 7:08 PM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> On Wed, 31 Jul 2019 03:48:20 +0900
> "Daniel T. Lee" <danieltimlee@gmail.com> wrote:
>
> > By this commit, using `bpftool net load`, user can load XDP prog on
> > interface. New type of enum 'net_load_type' has been made, as stated at
> > cover-letter, the meaning of 'load' is, prog will be loaded on interface.
>
> Why the keyword "load" ?
> Why not "attach" (and "detach")?
>
> For BPF there is a clear distinction between the "load" and "attach"
> steps.  I know this is under subcommand "net", but to follow the
> conversion of other subcommands e.g. "prog" there are both "load" and
> "attach" commands.
>
>
> > BPF prog will be loaded through libbpf 'bpf_set_link_xdp_fd'.
>
> Again this is a "set" operation, not a "load" operation.

From earlier at cover-letter, I thought using the same word 'load' might give
confusion since XDP program is not considered as 'bpf_attach_type' and can't
be attached with 'BPF_PROG_ATTACH'.

But, according to the feedback from you and Andrii Nakryiko, replacing
the word 'load' as 'attach' would be more clear and more consistent.

> > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
>
> [...]
> >  static int do_show(int argc, char **argv)
> >  {
> >       struct bpf_attach_info attach_info = {};
> > @@ -305,13 +405,17 @@ static int do_help(int argc, char **argv)
> >
> >       fprintf(stderr,
> >               "Usage: %s %s { show | list } [dev <devname>]\n"
> > +             "       %s %s load PROG LOAD_TYPE <devname>\n"
>
> The "PROG" here does it correspond to the 'bpftool prog' syntax?:
>
>  PROG := { id PROG_ID | pinned FILE | tag PROG_TAG }
>

Yes. By using the same 'prog_parse_fd' from 'bpftool prog',
user can 'attach' XDP prog with id, pinned file or tag.

> >               "       %s %s help\n"
> > +             "\n"
> > +             "       " HELP_SPEC_PROGRAM "\n"
> > +             "       LOAD_TYPE := { xdp | xdpgeneric | xdpdrv | xdpoffload }\n"
> >               "Note: Only xdp and tc attachments are supported now.\n"
> >               "      For progs attached to cgroups, use \"bpftool cgroup\"\n"
> >               "      to dump program attachments. For program types\n"
> >               "      sk_{filter,skb,msg,reuseport} and lwt/seg6, please\n"
> >               "      consult iproute2.\n",
>
>
> --
> Best regards,
>   Jesper Dangaard Brouer
>   MSc.CS, Principal Kernel Engineer at Red Hat
>   LinkedIn: http://www.linkedin.com/in/brouer

And about the enum 'NET_LOAD_TYPE_XDP_DRIVE',
'DRIVER' looks more clear to understand.

Will change to it right away.

Thanks for the review.

Re: [PATCH 0/2] tools: bpftool: add net (un)load command to load XDP

[David Ahern]

On 7/30/19 7:21 PM, Jakub Kicinski wrote:
> 
>>>> If bpftool was taught to do equivalent of 'ip link' that would be
>>>> very different story and I would be opposed to that.  
>>> Yes, that'd be pretty clear cut, only the XDP stuff is a bit more 
>>> of a judgement call.  
>> bpftool must be able to introspect every aspect of bpf programming.
>> That includes detaching and attaching anywhere.
>> Anyone doing 'bpftool p s' should be able to switch off particular
>> prog id without learning ten different other tools.
> I think the fact that we already have an implementation in iproute2,
> which is at the risk of bit rot is more important to me that the
> hypothetical scenario where everyone knows to just use bpftool (for
> XDP, for TC it's still iproute2 unless there's someone crazy enough 
> to reimplement the TC functionality :))

apparently the iproute2 version has bit rot which is a shame.

> 
> I'm not sure we can settle our differences over email :)
> I have tremendous respect for all the maintainers I CCed here, 
> if nobody steps up to agree with me I'll concede the bpftool net
> battle entirely :)

bpftool started as an introspection tool and has turned into a one stop
shop for all things ebpf. I am mixed on whether that is a good thing or
a bad thing.