[PATCH iproute2-next] iplink: add support for reporting multiple XDP programs

[PATCH iproute2-next] iplink: add support for reporting multiple XDP programs

[Jakub Kicinski]

Kernel now supports attaching XDP programs in the driver
and hardware at the same time.  Print that information
correctly.

In case there are multiple programs attached kernel will
not provide IFLA_XDP_PROG_ID, so don't expect it to be
there (this also improves the printing for very old kernels
slightly, as it avoids unnecessary "prog/xdp" line).

In short mode preserve the current outputs but don't print
IDs if there are multiple.
[...]
6: netdevsim0: <BROADCAST,NOARP> mtu 1500 xdpoffload/id:11 qdisc [...]
[...]
[...]
6: netdevsim0: <BROADCAST,NOARP> mtu 1500 xdpmulti qdisc [...]
[...]

ip link output will keep using prog/xdp prefix if only one program
is attached, but can also print multiple program lines:

    prog/xdp id 8 tag fc7a51d1a693a99e jited

vs:

    prog/xdpdrv id 8 tag fc7a51d1a693a99e jited
    prog/xdpoffload id 9 tag fc7a51d1a693a99e

JSON output gains a new array called "attached" which will
contain the full list of attached programs along with their
attachment modes:

        "xdp": {
            "mode": 3,
            "prog": {
                "id": 11,
                "tag": "fc7a51d1a693a99e",
                "jited": 0
            },
            "attached": [ {
                    "mode": 3,
                    "prog": {
                        "id": 11,
                        "tag": "fc7a51d1a693a99e",
                        "jited": 0
                    }
                } ]
        },

In case there are multiple programs attached the general "xdp"
section will not contain program information:

        "xdp": {
            "mode": 4,
            "attached": [ {
                    "mode": 1,
                    "prog": {
                        "id": 10,
                        "tag": "fc7a51d1a693a99e",
                        "jited": 1
                    }
                },{
                    "mode": 3,
                    "prog": {
                        "id": 11,
                        "tag": "fc7a51d1a693a99e",
                        "jited": 0
                    }
                } ]
        },

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 ip/iplink_xdp.c | 65 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 12 deletions(-)

diff --git a/ip/iplink_xdp.c b/ip/iplink_xdp.c
index dd4fd1fd3a3b..0328bc01a981 100644
--- a/ip/iplink_xdp.c
+++ b/ip/iplink_xdp.c
@@ -91,6 +91,18 @@ int xdp_parse(int *argc, char ***argv, struct iplink_req *req,
 	return 0;
 }
 
+static void xdp_dump_json_one(struct rtattr *tb[IFLA_XDP_MAX + 1], __u32 attr,
+			      __u8 mode)
+{
+	if (!tb[attr])
+		return;
+
+	open_json_object(NULL);
+	print_uint(PRINT_JSON, "mode", NULL, mode);
+	bpf_dump_prog_info(NULL, rta_getattr_u32(tb[attr]));
+	close_json_object();
+}
+
 static void xdp_dump_json(struct rtattr *tb[IFLA_XDP_MAX + 1])
 {
 	__u32 prog_id = 0;
@@ -104,13 +116,40 @@ static void xdp_dump_json(struct rtattr *tb[IFLA_XDP_MAX + 1])
 	print_uint(PRINT_JSON, "mode", NULL, mode);
 	if (prog_id)
 		bpf_dump_prog_info(NULL, prog_id);
+
+	open_json_array(PRINT_JSON, "attached");
+	xdp_dump_json_one(tb, IFLA_XDP_SKB_PROG_ID, XDP_ATTACHED_SKB);
+	xdp_dump_json_one(tb, IFLA_XDP_DRV_PROG_ID, XDP_ATTACHED_DRV);
+	xdp_dump_json_one(tb, IFLA_XDP_HW_PROG_ID, XDP_ATTACHED_HW);
+	close_json_array(PRINT_JSON, NULL);
+
 	close_json_object();
 }
 
+static void xdp_dump_prog_one(FILE *fp, struct rtattr *tb[IFLA_XDP_MAX + 1],
+			      __u32 attr, bool link, bool details, char *pfx)
+{
+	__u32 prog_id;
+
+	if (!tb[attr])
+		return;
+
+	prog_id = rta_getattr_u32(tb[attr]);
+	if (!details) {
+		if (prog_id && !link && attr == IFLA_XDP_PROG_ID)
+			fprintf(fp, "/id:%u", prog_id);
+		return;
+	}
+
+	if (prog_id) {
+		fprintf(fp, "%s    prog/xdp%s ", _SL_, pfx);
+		bpf_dump_prog_info(fp, prog_id);
+	}
+}
+
 void xdp_dump(FILE *fp, struct rtattr *xdp, bool link, bool details)
 {
 	struct rtattr *tb[IFLA_XDP_MAX + 1];
-	__u32 prog_id = 0;
 	__u8 mode;
 
 	parse_rtattr_nested(tb, IFLA_XDP_MAX, xdp);
@@ -124,27 +163,29 @@ void xdp_dump(FILE *fp, struct rtattr *xdp, bool link, bool details)
 	else if (is_json_context())
 		return details ? (void)0 : xdp_dump_json(tb);
 	else if (details && link)
-		fprintf(fp, "%s    prog/xdp", _SL_);
+		/* don't print mode */;
 	else if (mode == XDP_ATTACHED_DRV)
 		fprintf(fp, "xdp");
 	else if (mode == XDP_ATTACHED_SKB)
 		fprintf(fp, "xdpgeneric");
 	else if (mode == XDP_ATTACHED_HW)
 		fprintf(fp, "xdpoffload");
+	else if (mode == XDP_ATTACHED_MULTI)
+		fprintf(fp, "xdpmulti");
 	else
 		fprintf(fp, "xdp[%u]", mode);
 
-	if (tb[IFLA_XDP_PROG_ID])
-		prog_id = rta_getattr_u32(tb[IFLA_XDP_PROG_ID]);
-	if (!details) {
-		if (prog_id && !link)
-			fprintf(fp, "/id:%u", prog_id);
-		fprintf(fp, " ");
-		return;
+	xdp_dump_prog_one(fp, tb, IFLA_XDP_PROG_ID, link, details, "");
+
+	if (mode == XDP_ATTACHED_MULTI) {
+		xdp_dump_prog_one(fp, tb, IFLA_XDP_SKB_PROG_ID, link, details,
+				  "generic");
+		xdp_dump_prog_one(fp, tb, IFLA_XDP_DRV_PROG_ID, link, details,
+				  "drv");
+		xdp_dump_prog_one(fp, tb, IFLA_XDP_HW_PROG_ID, link, details,
+				  "offload");
 	}
 
-	if (prog_id) {
+	if (!details || !link)
 		fprintf(fp, " ");
-		bpf_dump_prog_info(fp, prog_id);
-	}
 }
-- 
2.17.1

Re: [PATCH iproute2-next] iplink: add support for reporting multiple XDP programs

[Stephen Hemminger]

On Fri, 13 Jul 2018 13:43:59 -0700
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

>  
> +static void xdp_dump_prog_one(FILE *fp, struct rtattr *tb[IFLA_XDP_MAX + 1],
> +			      __u32 attr, bool link, bool details, char *pfx)
> +{
> +	__u32 prog_id;
> +
> +	if (!tb[attr])
> +		return;
> +
> +	prog_id = rta_getattr_u32(tb[attr]);
> +	if (!details) {
> +		if (prog_id && !link && attr == IFLA_XDP_PROG_ID)
> +			fprintf(fp, "/id:%u", prog_id);
> +		return;
> +	}
> +
> +	if (prog_id) {
> +		fprintf(fp, "%s    prog/xdp%s ", _SL_, pfx);
> +		bpf_dump_prog_info(fp, prog_id);
> +	}

Maybe const char *pfx.

I prefer to not use "printf(fp," and use print_string(PRINT_FP, NULL, "%s", ...)
because otherwise you end up mixing strings and json format output in the
same result.

You should be able to do
	tc -j ... 
and always get valid JSON output.

One quick way to test json validation is to pipe it into python:
	tc -j ... | python -mjson.tool

Re: [PATCH iproute2-next] iplink: add support for reporting multiple XDP programs

[Jakub Kicinski]

On Fri, 13 Jul 2018 13:59:41 -0700, Stephen Hemminger wrote:
> On Fri, 13 Jul 2018 13:43:59 -0700
> Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> 
> >  
> > +static void xdp_dump_prog_one(FILE *fp, struct rtattr *tb[IFLA_XDP_MAX + 1],
> > +			      __u32 attr, bool link, bool details, char *pfx)
> > +{
> > +	__u32 prog_id;
> > +
> > +	if (!tb[attr])
> > +		return;
> > +
> > +	prog_id = rta_getattr_u32(tb[attr]);
> > +	if (!details) {
> > +		if (prog_id && !link && attr == IFLA_XDP_PROG_ID)
> > +			fprintf(fp, "/id:%u", prog_id);
> > +		return;
> > +	}
> > +
> > +	if (prog_id) {
> > +		fprintf(fp, "%s    prog/xdp%s ", _SL_, pfx);
> > +		bpf_dump_prog_info(fp, prog_id);
> > +	}  
> 
> Maybe const char *pfx.

Will do!  Looking again at this code, I think I will also do this:

diff --git a/ip/iplink_xdp.c b/ip/iplink_xdp.c
index 0328bc01a981..68629834cc00 100644
--- a/ip/iplink_xdp.c
+++ b/ip/iplink_xdp.c
@@ -121,6 +121,12 @@ static void xdp_dump_json(struct rtattr *tb[IFLA_XDP_MAX + 1])
        xdp_dump_json_one(tb, IFLA_XDP_SKB_PROG_ID, XDP_ATTACHED_SKB);
        xdp_dump_json_one(tb, IFLA_XDP_DRV_PROG_ID, XDP_ATTACHED_DRV);
        xdp_dump_json_one(tb, IFLA_XDP_HW_PROG_ID, XDP_ATTACHED_HW);
+       /* Older kernel - use IFLA_XDP_PROG_ID */
+       if (tb[IFLA_XDP_PROG_ID] &&
+           !(tb[IFLA_XDP_ATTACHED_SKB] ||
+             tb[IFLA_XDP_ATTACHED_DRV] ||
+             tb[IFLA_XDP_ATTACHED_HW]))
+               xdp_dump_json_one(tb, IFLA_XDP_PROG_ID, mode);
        close_json_array(PRINT_JSON, NULL);
 
        close_json_object();

So that on older kernels we will still be able to depend on the
contents of the "attached" array, even if kernel does not know to
report program per-mode, yet.

> I prefer to not use "printf(fp," and use print_string(PRINT_FP, NULL, "%s", ...)
> because otherwise you end up mixing strings and json format output in the
> same result.
> 
> You should be able to do
> 	tc -j ... 
> and always get valid JSON output.
> 
> One quick way to test json validation is to pipe it into python:
> 	tc -j ... | python -mjson.tool

Note that XDP has separate print functions for plain text and JSON, and
the flow gets separated early on:

	mode = rta_getattr_u8(tb[IFLA_XDP_ATTACHED]);
	if (mode == XDP_ATTACHED_NONE)
		return;
	else if (is_json_context())
		return details ? (void)0 : xdp_dump_json(tb);

	... non-JSON handling follows...

The use of fprintfs is therefore okay.  Do you have a preference for
using the wrapper, even if fprintf is safe?  It's brevity vs
consistency, I guess.  We'd need a separate patch for that, 'cause I'm
not touching all the fprintfs in the file, anyway.

Re: [PATCH iproute2-next] iplink: add support for reporting multiple XDP programs

[Stephen Hemminger]

On Fri, 13 Jul 2018 14:20:37 -0700
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

> On Fri, 13 Jul 2018 13:59:41 -0700, Stephen Hemminger wrote:
> > On Fri, 13 Jul 2018 13:43:59 -0700
> > Jakub Kicinski <jakub.kicinski@netronome.com> wrote:
> >   
> > >  
> > > +static void xdp_dump_prog_one(FILE *fp, struct rtattr *tb[IFLA_XDP_MAX + 1],
> > > +			      __u32 attr, bool link, bool details, char *pfx)
> > > +{
> > > +	__u32 prog_id;
> > > +
> > > +	if (!tb[attr])
> > > +		return;
> > > +
> > > +	prog_id = rta_getattr_u32(tb[attr]);
> > > +	if (!details) {
> > > +		if (prog_id && !link && attr == IFLA_XDP_PROG_ID)
> > > +			fprintf(fp, "/id:%u", prog_id);
> > > +		return;
> > > +	}
> > > +
> > > +	if (prog_id) {
> > > +		fprintf(fp, "%s    prog/xdp%s ", _SL_, pfx);
> > > +		bpf_dump_prog_info(fp, prog_id);
> > > +	}    
> > 
> > Maybe const char *pfx.  
> 
> Will do!  Looking again at this code, I think I will also do this:
> 
> diff --git a/ip/iplink_xdp.c b/ip/iplink_xdp.c
> index 0328bc01a981..68629834cc00 100644
> --- a/ip/iplink_xdp.c
> +++ b/ip/iplink_xdp.c
> @@ -121,6 +121,12 @@ static void xdp_dump_json(struct rtattr *tb[IFLA_XDP_MAX + 1])
>         xdp_dump_json_one(tb, IFLA_XDP_SKB_PROG_ID, XDP_ATTACHED_SKB);
>         xdp_dump_json_one(tb, IFLA_XDP_DRV_PROG_ID, XDP_ATTACHED_DRV);
>         xdp_dump_json_one(tb, IFLA_XDP_HW_PROG_ID, XDP_ATTACHED_HW);
> +       /* Older kernel - use IFLA_XDP_PROG_ID */
> +       if (tb[IFLA_XDP_PROG_ID] &&
> +           !(tb[IFLA_XDP_ATTACHED_SKB] ||
> +             tb[IFLA_XDP_ATTACHED_DRV] ||
> +             tb[IFLA_XDP_ATTACHED_HW]))
> +               xdp_dump_json_one(tb, IFLA_XDP_PROG_ID, mode);
>         close_json_array(PRINT_JSON, NULL);
>  
>         close_json_object();
> 
> So that on older kernels we will still be able to depend on the
> contents of the "attached" array, even if kernel does not know to
> report program per-mode, yet.
> 
> > I prefer to not use "printf(fp," and use print_string(PRINT_FP, NULL, "%s", ...)
> > because otherwise you end up mixing strings and json format output in the
> > same result.
> > 
> > You should be able to do
> > 	tc -j ... 
> > and always get valid JSON output.
> > 
> > One quick way to test json validation is to pipe it into python:
> > 	tc -j ... | python -mjson.tool  
> 
> Note that XDP has separate print functions for plain text and JSON, and
> the flow gets separated early on:
> 
> 	mode = rta_getattr_u8(tb[IFLA_XDP_ATTACHED]);
> 	if (mode == XDP_ATTACHED_NONE)
> 		return;
> 	else if (is_json_context())
> 		return details ? (void)0 : xdp_dump_json(tb);
> 
> 	... non-JSON handling follows...
> 
> The use of fprintfs is therefore okay.  Do you have a preference for
> using the wrapper, even if fprintf is safe?  It's brevity vs
> consistency, I guess.  We'd need a separate patch for that, 'cause I'm
> not touching all the fprintfs in the file, anyway.

The only preference for the wrapper is that it is easy way to make
sure all code is JSON aware.  Since fp is always stdout in current
code, maybe just convert to printf.

Re: [PATCH iproute2-next] iplink: add support for reporting multiple XDP programs

[Jakub Kicinski]

On Fri, 13 Jul 2018 15:23:30 -0700, Stephen Hemminger wrote:
> > > I prefer to not use "printf(fp," and use print_string(PRINT_FP, NULL, "%s", ...)
> > > because otherwise you end up mixing strings and json format output in the
> > > same result.
> > > 
> > > You should be able to do
> > > 	tc -j ... 
> > > and always get valid JSON output.
> > > 
> > > One quick way to test json validation is to pipe it into python:
> > > 	tc -j ... | python -mjson.tool    
> > 
> > Note that XDP has separate print functions for plain text and JSON, and
> > the flow gets separated early on:
> > 
> > 	mode = rta_getattr_u8(tb[IFLA_XDP_ATTACHED]);
> > 	if (mode == XDP_ATTACHED_NONE)
> > 		return;
> > 	else if (is_json_context())
> > 		return details ? (void)0 : xdp_dump_json(tb);
> > 
> > 	... non-JSON handling follows...
> > 
> > The use of fprintfs is therefore okay.  Do you have a preference for
> > using the wrapper, even if fprintf is safe?  It's brevity vs
> > consistency, I guess.  We'd need a separate patch for that, 'cause I'm
> > not touching all the fprintfs in the file, anyway.  
> 
> The only preference for the wrapper is that it is easy way to make
> sure all code is JSON aware.  

Yes...

> Since fp is always stdout in current code, maybe just convert to printf.

...or maybe we could consider adding a wrapper for printf that wouldn't
take all the unnecessary parameters print_string() takes, yet clearly
indicate autor knows about JSON output concerns?