[bpf-next PATCH 0/2] xdp/bpf: extend XDP samples/bpf xdp_rxq_info

[bpf-next PATCH 0/2] xdp/bpf: extend XDP samples/bpf xdp_rxq_info

[Jesper Dangaard Brouer]

While writing an article about XDP, the samples/bpf xdp_rxq_info
program were extended to cover some more use-cases.

---

Jesper Dangaard Brouer (2):
      samples/bpf: extend xdp_rxq_info to read packet payload
      samples/bpf: xdp_rxq_info action XDP_TX must adjust MAC-addrs


 samples/bpf/xdp_rxq_info_kern.c |   43 +++++++++++++++++++++++++++++++++++++
 samples/bpf/xdp_rxq_info_user.c |   45 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 82 insertions(+), 6 deletions(-)

[bpf-next PATCH 1/2] samples/bpf: extend xdp_rxq_info to read packet payload

[Jesper Dangaard Brouer]

There is a cost associated with reading the packet data payload
that this test ignored.  Add option --read to allow enabling
reading part of the payload.

This sample/tool helps us analyse an issue observed with a NIC
mlx5 (ConnectX-5 Ex) and an Intel(R) Xeon(R) CPU E5-1650 v4.

With no_touch of data:

Running XDP on dev:mlx5p1 (ifindex:8) action:XDP_DROP options:no_touch
XDP stats       CPU     pps         issue-pps
XDP-RX CPU      0       14,465,157  0
XDP-RX CPU      1       14,464,728  0
XDP-RX CPU      2       14,465,283  0
XDP-RX CPU      3       14,465,282  0
XDP-RX CPU      4       14,464,159  0
XDP-RX CPU      5       14,465,379  0
XDP-RX CPU      total   86,789,992

When not touching data, we observe that the CPUs have idle cycles.
When reading data the CPUs are 100% busy in softirq.

With reading data:

Running XDP on dev:mlx5p1 (ifindex:8) action:XDP_DROP options:read
XDP stats       CPU     pps         issue-pps
XDP-RX CPU      0       9,620,639   0
XDP-RX CPU      1       9,489,843   0
XDP-RX CPU      2       9,407,854   0
XDP-RX CPU      3       9,422,289   0
XDP-RX CPU      4       9,321,959   0
XDP-RX CPU      5       9,395,242   0
XDP-RX CPU      total   56,657,828

The effect seen above is a result of cache-misses occuring when
more RXQs are being used.  Based on perf-event observations, our
conclusion is that the CPUs DDIO (Direct Data I/O) choose to
deliver packet into main memory, instead of L3-cache.  We also
found, that this can be mitigated by either using less RXQs or by
reducing NICs the RX-ring size.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 samples/bpf/xdp_rxq_info_kern.c |   19 +++++++++++++++++++
 samples/bpf/xdp_rxq_info_user.c |   34 ++++++++++++++++++++++++++++------
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/samples/bpf/xdp_rxq_info_kern.c b/samples/bpf/xdp_rxq_info_kern.c
index 3fd209291653..61af6210df2f 100644
--- a/samples/bpf/xdp_rxq_info_kern.c
+++ b/samples/bpf/xdp_rxq_info_kern.c
@@ -4,6 +4,8 @@
  *  Example howto extract XDP RX-queue info
  */
 #include <uapi/linux/bpf.h>
+#include <uapi/linux/if_ether.h>
+#include <uapi/linux/in.h>
 #include "bpf_helpers.h"
 
 /* Config setup from with userspace
@@ -14,6 +16,11 @@
 struct config {
 	__u32 action;
 	int ifindex;
+	__u32 options;
+};
+enum cfg_options_flags {
+	NO_TOUCH = 0x0U,
+	READ_MEM = 0x1U,
 };
 struct bpf_map_def SEC("maps") config_map = {
 	.type		= BPF_MAP_TYPE_ARRAY,
@@ -90,6 +97,18 @@ int  xdp_prognum0(struct xdp_md *ctx)
 	if (key == MAX_RXQs)
 		rxq_rec->issue++;
 
+	/* Default: Don't touch packet data, only count packets */
+	if (unlikely(config->options & READ_MEM)) {
+		struct ethhdr *eth = data;
+
+		if (eth + 1 > data_end)
+			return XDP_ABORTED;
+
+		/* Avoid compiler removing this: Drop non 802.3 Ethertypes */
+		if (ntohs(eth->h_proto) < ETH_P_802_3_MIN)
+			return XDP_ABORTED;
+	}
+
 	return config->action;
 }
 
diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
index e4e9ba52bff0..435485d4f49e 100644
--- a/samples/bpf/xdp_rxq_info_user.c
+++ b/samples/bpf/xdp_rxq_info_user.c
@@ -50,6 +50,7 @@ static const struct option long_options[] = {
 	{"sec",		required_argument,	NULL, 's' },
 	{"no-separators", no_argument,		NULL, 'z' },
 	{"action",	required_argument,	NULL, 'a' },
+	{"readmem", 	no_argument,		NULL, 'r' },
 	{0, 0, NULL,  0 }
 };
 
@@ -66,6 +67,11 @@ static void int_exit(int sig)
 struct config {
 	__u32 action;
 	int ifindex;
+	__u32 options;
+};
+enum cfg_options_flags {
+	NO_TOUCH = 0x0U,
+	READ_MEM = 0x1U,
 };
 #define XDP_ACTION_MAX (XDP_TX + 1)
 #define XDP_ACTION_MAX_STRLEN 11
@@ -109,6 +115,16 @@ static void list_xdp_actions(void)
 	printf("\n");
 }
 
+static char* options2str(enum cfg_options_flags flag)
+{
+	if (flag == NO_TOUCH)
+		return "no_touch";
+	if (flag & READ_MEM)
+		return "read";
+	fprintf(stderr, "ERR: Unknown config option flags");
+	exit(EXIT_FAIL);
+}
+
 static void usage(char *argv[])
 {
 	int i;
@@ -305,7 +321,7 @@ static __u64 calc_errs_pps(struct datarec *r,
 
 static void stats_print(struct stats_record *stats_rec,
 			struct stats_record *stats_prev,
-			int action)
+			int action, __u32 cfg_opt)
 {
 	unsigned int nr_rxqs = bpf_map__def(rx_queue_index_map)->max_entries;
 	unsigned int nr_cpus = bpf_num_possible_cpus();
@@ -316,8 +332,8 @@ static void stats_print(struct stats_record *stats_rec,
 	int i;
 
 	/* Header */
-	printf("\nRunning XDP on dev:%s (ifindex:%d) action:%s\n",
-	       ifname, ifindex, action2str(action));
+	printf("\nRunning XDP on dev:%s (ifindex:%d) action:%s options:%s\n",
+	       ifname, ifindex, action2str(action), options2str(cfg_opt));
 
 	/* stats_global_map */
 	{
@@ -399,7 +415,7 @@ static inline void swap(struct stats_record **a, struct stats_record **b)
 	*b = tmp;
 }
 
-static void stats_poll(int interval, int action)
+static void stats_poll(int interval, int action, __u32 cfg_opt)
 {
 	struct stats_record *record, *prev;
 
@@ -410,7 +426,7 @@ static void stats_poll(int interval, int action)
 	while (1) {
 		swap(&prev, &record);
 		stats_collect(record);
-		stats_print(record, prev, action);
+		stats_print(record, prev, action, cfg_opt);
 		sleep(interval);
 	}
 
@@ -421,6 +437,7 @@ static void stats_poll(int interval, int action)
 
 int main(int argc, char **argv)
 {
+	__u32 cfg_options= NO_TOUCH ; /* Default: Don't touch packet memory */
 	struct rlimit r = {10 * 1024 * 1024, RLIM_INFINITY};
 	struct bpf_prog_load_attr prog_load_attr = {
 		.prog_type	= BPF_PROG_TYPE_XDP,
@@ -435,6 +452,7 @@ int main(int argc, char **argv)
 	int interval = 2;
 	__u32 key = 0;
 
+
 	char action_str_buf[XDP_ACTION_MAX_STRLEN + 1 /* for \0 */] = { 0 };
 	int action = XDP_PASS; /* Default action */
 	char *action_str = NULL;
@@ -496,6 +514,9 @@ int main(int argc, char **argv)
 			action_str = (char *)&action_str_buf;
 			strncpy(action_str, optarg, XDP_ACTION_MAX_STRLEN);
 			break;
+		case 'r':
+			cfg_options |= READ_MEM;
+			break;
 		case 'h':
 		error:
 		default:
@@ -522,6 +543,7 @@ int main(int argc, char **argv)
 		}
 	}
 	cfg.action = action;
+	cfg.options = cfg_options;
 
 	/* Trick to pretty printf with thousands separators use %' */
 	if (use_separators)
@@ -542,6 +564,6 @@ int main(int argc, char **argv)
 		return EXIT_FAIL_XDP;
 	}
 
-	stats_poll(interval, action);
+	stats_poll(interval, action, cfg_options);
 	return EXIT_OK;
 }

Re: [bpf-next PATCH 1/2] samples/bpf: extend xdp_rxq_info to read packet payload

[Song Liu]

On Mon, Jun 25, 2018 at 7:27 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> There is a cost associated with reading the packet data payload
> that this test ignored.  Add option --read to allow enabling
> reading part of the payload.
>
> This sample/tool helps us analyse an issue observed with a NIC
> mlx5 (ConnectX-5 Ex) and an Intel(R) Xeon(R) CPU E5-1650 v4.
>
> With no_touch of data:
>
> Running XDP on dev:mlx5p1 (ifindex:8) action:XDP_DROP options:no_touch
> XDP stats       CPU     pps         issue-pps
> XDP-RX CPU      0       14,465,157  0
> XDP-RX CPU      1       14,464,728  0
> XDP-RX CPU      2       14,465,283  0
> XDP-RX CPU      3       14,465,282  0
> XDP-RX CPU      4       14,464,159  0
> XDP-RX CPU      5       14,465,379  0
> XDP-RX CPU      total   86,789,992
>
> When not touching data, we observe that the CPUs have idle cycles.
> When reading data the CPUs are 100% busy in softirq.
>
> With reading data:
>
> Running XDP on dev:mlx5p1 (ifindex:8) action:XDP_DROP options:read
> XDP stats       CPU     pps         issue-pps
> XDP-RX CPU      0       9,620,639   0
> XDP-RX CPU      1       9,489,843   0
> XDP-RX CPU      2       9,407,854   0
> XDP-RX CPU      3       9,422,289   0
> XDP-RX CPU      4       9,321,959   0
> XDP-RX CPU      5       9,395,242   0
> XDP-RX CPU      total   56,657,828
>
> The effect seen above is a result of cache-misses occuring when
> more RXQs are being used.  Based on perf-event observations, our
> conclusion is that the CPUs DDIO (Direct Data I/O) choose to
> deliver packet into main memory, instead of L3-cache.  We also
> found, that this can be mitigated by either using less RXQs or by
> reducing NICs the RX-ring size.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> ---
>  samples/bpf/xdp_rxq_info_kern.c |   19 +++++++++++++++++++
>  samples/bpf/xdp_rxq_info_user.c |   34 ++++++++++++++++++++++++++++------
>  2 files changed, 47 insertions(+), 6 deletions(-)
>
> diff --git a/samples/bpf/xdp_rxq_info_kern.c b/samples/bpf/xdp_rxq_info_kern.c
> index 3fd209291653..61af6210df2f 100644
> --- a/samples/bpf/xdp_rxq_info_kern.c
> +++ b/samples/bpf/xdp_rxq_info_kern.c
> @@ -4,6 +4,8 @@
>   *  Example howto extract XDP RX-queue info
>   */
>  #include <uapi/linux/bpf.h>
> +#include <uapi/linux/if_ether.h>
> +#include <uapi/linux/in.h>
>  #include "bpf_helpers.h"
>
>  /* Config setup from with userspace
> @@ -14,6 +16,11 @@
>  struct config {
>         __u32 action;
>         int ifindex;
> +       __u32 options;
> +};
> +enum cfg_options_flags {
> +       NO_TOUCH = 0x0U,
> +       READ_MEM = 0x1U,
>  };
>  struct bpf_map_def SEC("maps") config_map = {
>         .type           = BPF_MAP_TYPE_ARRAY,
> @@ -90,6 +97,18 @@ int  xdp_prognum0(struct xdp_md *ctx)
>         if (key == MAX_RXQs)
>                 rxq_rec->issue++;
>
> +       /* Default: Don't touch packet data, only count packets */
> +       if (unlikely(config->options & READ_MEM)) {
> +               struct ethhdr *eth = data;
> +
> +               if (eth + 1 > data_end)
> +                       return XDP_ABORTED;
> +
> +               /* Avoid compiler removing this: Drop non 802.3 Ethertypes */
> +               if (ntohs(eth->h_proto) < ETH_P_802_3_MIN)
> +                       return XDP_ABORTED;
> +       }
> +
>         return config->action;
>  }
>
> diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
> index e4e9ba52bff0..435485d4f49e 100644
> --- a/samples/bpf/xdp_rxq_info_user.c
> +++ b/samples/bpf/xdp_rxq_info_user.c
> @@ -50,6 +50,7 @@ static const struct option long_options[] = {
>         {"sec",         required_argument,      NULL, 's' },
>         {"no-separators", no_argument,          NULL, 'z' },
>         {"action",      required_argument,      NULL, 'a' },
> +       {"readmem",     no_argument,            NULL, 'r' },
>         {0, 0, NULL,  0 }
>  };
>
> @@ -66,6 +67,11 @@ static void int_exit(int sig)
>  struct config {
>         __u32 action;
>         int ifindex;
> +       __u32 options;
> +};
> +enum cfg_options_flags {
> +       NO_TOUCH = 0x0U,
> +       READ_MEM = 0x1U,
>  };
>  #define XDP_ACTION_MAX (XDP_TX + 1)
>  #define XDP_ACTION_MAX_STRLEN 11
> @@ -109,6 +115,16 @@ static void list_xdp_actions(void)
>         printf("\n");
>  }
>
> +static char* options2str(enum cfg_options_flags flag)
> +{
> +       if (flag == NO_TOUCH)
> +               return "no_touch";
> +       if (flag & READ_MEM)
> +               return "read";
> +       fprintf(stderr, "ERR: Unknown config option flags");
> +       exit(EXIT_FAIL);
> +}
> +

enum cfg_options_flags is used as a bitmap in other parts of the sample.
So this function is a little weird (with more flags added).

Thanks,
Song

>  static void usage(char *argv[])
>  {
>         int i;
> @@ -305,7 +321,7 @@ static __u64 calc_errs_pps(struct datarec *r,
>
>  static void stats_print(struct stats_record *stats_rec,
>                         struct stats_record *stats_prev,
> -                       int action)
> +                       int action, __u32 cfg_opt)
>  {
>         unsigned int nr_rxqs = bpf_map__def(rx_queue_index_map)->max_entries;
>         unsigned int nr_cpus = bpf_num_possible_cpus();
> @@ -316,8 +332,8 @@ static void stats_print(struct stats_record *stats_rec,
>         int i;
>
>         /* Header */
> -       printf("\nRunning XDP on dev:%s (ifindex:%d) action:%s\n",
> -              ifname, ifindex, action2str(action));
> +       printf("\nRunning XDP on dev:%s (ifindex:%d) action:%s options:%s\n",
> +              ifname, ifindex, action2str(action), options2str(cfg_opt));
>
>         /* stats_global_map */
>         {
> @@ -399,7 +415,7 @@ static inline void swap(struct stats_record **a, struct stats_record **b)
>         *b = tmp;
>  }
>
> -static void stats_poll(int interval, int action)
> +static void stats_poll(int interval, int action, __u32 cfg_opt)
>  {
>         struct stats_record *record, *prev;
>
> @@ -410,7 +426,7 @@ static void stats_poll(int interval, int action)
>         while (1) {
>                 swap(&prev, &record);
>                 stats_collect(record);
> -               stats_print(record, prev, action);
> +               stats_print(record, prev, action, cfg_opt);
>                 sleep(interval);
>         }
>
> @@ -421,6 +437,7 @@ static void stats_poll(int interval, int action)
>
>  int main(int argc, char **argv)
>  {
> +       __u32 cfg_options= NO_TOUCH ; /* Default: Don't touch packet memory */
>         struct rlimit r = {10 * 1024 * 1024, RLIM_INFINITY};
>         struct bpf_prog_load_attr prog_load_attr = {
>                 .prog_type      = BPF_PROG_TYPE_XDP,
> @@ -435,6 +452,7 @@ int main(int argc, char **argv)
>         int interval = 2;
>         __u32 key = 0;
>
> +
>         char action_str_buf[XDP_ACTION_MAX_STRLEN + 1 /* for \0 */] = { 0 };
>         int action = XDP_PASS; /* Default action */
>         char *action_str = NULL;
> @@ -496,6 +514,9 @@ int main(int argc, char **argv)
>                         action_str = (char *)&action_str_buf;
>                         strncpy(action_str, optarg, XDP_ACTION_MAX_STRLEN);
>                         break;
> +               case 'r':
> +                       cfg_options |= READ_MEM;
> +                       break;
>                 case 'h':
>                 error:
>                 default:
> @@ -522,6 +543,7 @@ int main(int argc, char **argv)
>                 }
>         }
>         cfg.action = action;
> +       cfg.options = cfg_options;
>
>         /* Trick to pretty printf with thousands separators use %' */
>         if (use_separators)
> @@ -542,6 +564,6 @@ int main(int argc, char **argv)
>                 return EXIT_FAIL_XDP;
>         }
>
> -       stats_poll(interval, action);
> +       stats_poll(interval, action, cfg_options);
>         return EXIT_OK;
>  }
>

Re: [bpf-next PATCH 1/2] samples/bpf: extend xdp_rxq_info to read packet payload

[Jesper Dangaard Brouer]

On Tue, 26 Jun 2018 16:53:15 -0700
Song Liu <liu.song.a23@gmail.com> wrote:

> > +static char* options2str(enum cfg_options_flags flag)
> > +{
> > +       if (flag == NO_TOUCH)
> > +               return "no_touch";
> > +       if (flag & READ_MEM)
> > +               return "read";
> > +       fprintf(stderr, "ERR: Unknown config option flags");
> > +       exit(EXIT_FAIL);
> > +}
> > +  
> 
> enum cfg_options_flags is used as a bitmap in other parts of the sample.
> So this function is a little weird (with more flags added).

Sure, and do I handle this correctly in the next patch.

I'm uncertain what you want me to change?
Do you want me to drop the enum, and use #define instead?

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

Re: [bpf-next PATCH 1/2] samples/bpf: extend xdp_rxq_info to read packet payload

[Song Liu]

On Wed, Jun 27, 2018 at 4:23 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> On Tue, 26 Jun 2018 16:53:15 -0700
> Song Liu <liu.song.a23@gmail.com> wrote:
>
>> > +static char* options2str(enum cfg_options_flags flag)
>> > +{
>> > +       if (flag == NO_TOUCH)
>> > +               return "no_touch";
>> > +       if (flag & READ_MEM)
>> > +               return "read";
>> > +       fprintf(stderr, "ERR: Unknown config option flags");
>> > +       exit(EXIT_FAIL);
>> > +}
>> > +
>>
>> enum cfg_options_flags is used as a bitmap in other parts of the sample.
>> So this function is a little weird (with more flags added).
>
> Sure, and do I handle this correctly in the next patch.
>
> I'm uncertain what you want me to change?
> Do you want me to drop the enum, and use #define instead?

I think  it is good  as-is for sample code.

Acked-by: Song Liu <songliubraving@fb.com>

[bpf-next PATCH 2/2] samples/bpf: xdp_rxq_info action XDP_TX must adjust MAC-addrs

[Jesper Dangaard Brouer]

XDP_TX requires also changing the MAC-addrs, else some hardware
may drop the TX packet before reaching the wire.  This was
observed with driver mlx5.

If xdp_rxq_info select --action XDP_TX the swapmac functionality
is activated.  It is also possible to manually enable via cmdline
option --swapmac.  This is practical if wanting to measure the
overhead of writing/updating payload for other action types.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 samples/bpf/xdp_rxq_info_kern.c |   26 +++++++++++++++++++++++++-
 samples/bpf/xdp_rxq_info_user.c |   11 +++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/samples/bpf/xdp_rxq_info_kern.c b/samples/bpf/xdp_rxq_info_kern.c
index 61af6210df2f..222a83eed1cb 100644
--- a/samples/bpf/xdp_rxq_info_kern.c
+++ b/samples/bpf/xdp_rxq_info_kern.c
@@ -21,6 +21,7 @@ struct config {
 enum cfg_options_flags {
 	NO_TOUCH = 0x0U,
 	READ_MEM = 0x1U,
+	SWAP_MAC = 0x2U,
 };
 struct bpf_map_def SEC("maps") config_map = {
 	.type		= BPF_MAP_TYPE_ARRAY,
@@ -52,6 +53,23 @@ struct bpf_map_def SEC("maps") rx_queue_index_map = {
 	.max_entries	= MAX_RXQs + 1,
 };
 
+static __always_inline
+void swap_src_dst_mac(void *data)
+{
+	unsigned short *p = data;
+	unsigned short dst[3];
+
+	dst[0] = p[0];
+	dst[1] = p[1];
+	dst[2] = p[2];
+	p[0] = p[3];
+	p[1] = p[4];
+	p[2] = p[5];
+	p[3] = dst[0];
+	p[4] = dst[1];
+	p[5] = dst[2];
+}
+
 SEC("xdp_prog0")
 int  xdp_prognum0(struct xdp_md *ctx)
 {
@@ -98,7 +116,7 @@ int  xdp_prognum0(struct xdp_md *ctx)
 		rxq_rec->issue++;
 
 	/* Default: Don't touch packet data, only count packets */
-	if (unlikely(config->options & READ_MEM)) {
+	if (unlikely(config->options & (READ_MEM|SWAP_MAC))) {
 		struct ethhdr *eth = data;
 
 		if (eth + 1 > data_end)
@@ -107,6 +125,12 @@ int  xdp_prognum0(struct xdp_md *ctx)
 		/* Avoid compiler removing this: Drop non 802.3 Ethertypes */
 		if (ntohs(eth->h_proto) < ETH_P_802_3_MIN)
 			return XDP_ABORTED;
+
+		/* XDP_TX requires changing MAC-addrs, else HW may drop.
+		 * Can also be enabled with --swapmac (for test purposes)
+		 */
+		if (unlikely(config->options & SWAP_MAC))
+			swap_src_dst_mac(data);
 	}
 
 	return config->action;
diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
index 435485d4f49e..248a7eab9531 100644
--- a/samples/bpf/xdp_rxq_info_user.c
+++ b/samples/bpf/xdp_rxq_info_user.c
@@ -51,6 +51,7 @@ static const struct option long_options[] = {
 	{"no-separators", no_argument,		NULL, 'z' },
 	{"action",	required_argument,	NULL, 'a' },
 	{"readmem", 	no_argument,		NULL, 'r' },
+	{"swapmac", 	no_argument,		NULL, 'm' },
 	{0, 0, NULL,  0 }
 };
 
@@ -72,6 +73,7 @@ struct config {
 enum cfg_options_flags {
 	NO_TOUCH = 0x0U,
 	READ_MEM = 0x1U,
+	SWAP_MAC = 0x2U,
 };
 #define XDP_ACTION_MAX (XDP_TX + 1)
 #define XDP_ACTION_MAX_STRLEN 11
@@ -119,6 +121,8 @@ static char* options2str(enum cfg_options_flags flag)
 {
 	if (flag == NO_TOUCH)
 		return "no_touch";
+	if (flag & SWAP_MAC)
+		return "swapmac";
 	if (flag & READ_MEM)
 		return "read";
 	fprintf(stderr, "ERR: Unknown config option flags");
@@ -517,6 +521,9 @@ int main(int argc, char **argv)
 		case 'r':
 			cfg_options |= READ_MEM;
 			break;
+		case 'm':
+			cfg_options |= SWAP_MAC;
+			break;
 		case 'h':
 		error:
 		default:
@@ -543,6 +550,10 @@ int main(int argc, char **argv)
 		}
 	}
 	cfg.action = action;
+
+	/* XDP_TX requires changing MAC-addrs, else HW may drop */
+	if (action == XDP_TX)
+		cfg_options |= SWAP_MAC;
 	cfg.options = cfg_options;
 
 	/* Trick to pretty printf with thousands separators use %' */

Re: [bpf-next PATCH 2/2] samples/bpf: xdp_rxq_info action XDP_TX must adjust MAC-addrs

[Song Liu]

On Mon, Jun 25, 2018 at 7:27 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> XDP_TX requires also changing the MAC-addrs, else some hardware
> may drop the TX packet before reaching the wire.  This was
> observed with driver mlx5.
>
> If xdp_rxq_info select --action XDP_TX the swapmac functionality
> is activated.  It is also possible to manually enable via cmdline
> option --swapmac.  This is practical if wanting to measure the
> overhead of writing/updating payload for other action types.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> ---
>  samples/bpf/xdp_rxq_info_kern.c |   26 +++++++++++++++++++++++++-
>  samples/bpf/xdp_rxq_info_user.c |   11 +++++++++++
>  2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/samples/bpf/xdp_rxq_info_kern.c b/samples/bpf/xdp_rxq_info_kern.c
> index 61af6210df2f..222a83eed1cb 100644
> --- a/samples/bpf/xdp_rxq_info_kern.c
> +++ b/samples/bpf/xdp_rxq_info_kern.c
> @@ -21,6 +21,7 @@ struct config {
>  enum cfg_options_flags {
>         NO_TOUCH = 0x0U,
>         READ_MEM = 0x1U,
> +       SWAP_MAC = 0x2U,
>  };
>  struct bpf_map_def SEC("maps") config_map = {
>         .type           = BPF_MAP_TYPE_ARRAY,
> @@ -52,6 +53,23 @@ struct bpf_map_def SEC("maps") rx_queue_index_map = {
>         .max_entries    = MAX_RXQs + 1,
>  };
>
> +static __always_inline
> +void swap_src_dst_mac(void *data)
> +{
> +       unsigned short *p = data;
> +       unsigned short dst[3];
> +
> +       dst[0] = p[0];
> +       dst[1] = p[1];
> +       dst[2] = p[2];
> +       p[0] = p[3];
> +       p[1] = p[4];
> +       p[2] = p[5];
> +       p[3] = dst[0];
> +       p[4] = dst[1];
> +       p[5] = dst[2];
> +}
> +
>  SEC("xdp_prog0")
>  int  xdp_prognum0(struct xdp_md *ctx)
>  {
> @@ -98,7 +116,7 @@ int  xdp_prognum0(struct xdp_md *ctx)
>                 rxq_rec->issue++;
>
>         /* Default: Don't touch packet data, only count packets */
> -       if (unlikely(config->options & READ_MEM)) {
> +       if (unlikely(config->options & (READ_MEM|SWAP_MAC))) {
>                 struct ethhdr *eth = data;
>
>                 if (eth + 1 > data_end)
> @@ -107,6 +125,12 @@ int  xdp_prognum0(struct xdp_md *ctx)
>                 /* Avoid compiler removing this: Drop non 802.3 Ethertypes */
>                 if (ntohs(eth->h_proto) < ETH_P_802_3_MIN)
>                         return XDP_ABORTED;
> +
> +               /* XDP_TX requires changing MAC-addrs, else HW may drop.
> +                * Can also be enabled with --swapmac (for test purposes)
> +                */
> +               if (unlikely(config->options & SWAP_MAC))
> +                       swap_src_dst_mac(data);
>         }
>
>         return config->action;
> diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
> index 435485d4f49e..248a7eab9531 100644
> --- a/samples/bpf/xdp_rxq_info_user.c
> +++ b/samples/bpf/xdp_rxq_info_user.c
> @@ -51,6 +51,7 @@ static const struct option long_options[] = {
>         {"no-separators", no_argument,          NULL, 'z' },
>         {"action",      required_argument,      NULL, 'a' },
>         {"readmem",     no_argument,            NULL, 'r' },
> +       {"swapmac",     no_argument,            NULL, 'm' },
>         {0, 0, NULL,  0 }
>  };
>
> @@ -72,6 +73,7 @@ struct config {
>  enum cfg_options_flags {
>         NO_TOUCH = 0x0U,
>         READ_MEM = 0x1U,
> +       SWAP_MAC = 0x2U,
>  };
>  #define XDP_ACTION_MAX (XDP_TX + 1)
>  #define XDP_ACTION_MAX_STRLEN 11
> @@ -119,6 +121,8 @@ static char* options2str(enum cfg_options_flags flag)
>  {
>         if (flag == NO_TOUCH)
>                 return "no_touch";
> +       if (flag & SWAP_MAC)
> +               return "swapmac";
>         if (flag & READ_MEM)
>                 return "read";

I guess SWAP_MAC also reads the memory, so it "includes" READ_MEM?
It is OK for now. We may need to refactor this part when adding other
flags in the
future.

Thanks,
Song


>         fprintf(stderr, "ERR: Unknown config option flags");
> @@ -517,6 +521,9 @@ int main(int argc, char **argv)
>                 case 'r':
>                         cfg_options |= READ_MEM;
>                         break;
> +               case 'm':
> +                       cfg_options |= SWAP_MAC;
> +                       break;
>                 case 'h':
>                 error:
>                 default:
> @@ -543,6 +550,10 @@ int main(int argc, char **argv)
>                 }
>         }
>         cfg.action = action;
> +
> +       /* XDP_TX requires changing MAC-addrs, else HW may drop */
> +       if (action == XDP_TX)
> +               cfg_options |= SWAP_MAC;
>         cfg.options = cfg_options;
>
>         /* Trick to pretty printf with thousands separators use %' */
>

Re: [bpf-next PATCH 2/2] samples/bpf: xdp_rxq_info action XDP_TX must adjust MAC-addrs

[Jesper Dangaard Brouer]

On Tue, 26 Jun 2018 17:09:01 -0700
Song Liu <liu.song.a23@gmail.com> wrote:

> On Mon, Jun 25, 2018 at 7:27 AM, Jesper Dangaard Brouer
> <brouer@redhat.com> wrote:
> > XDP_TX requires also changing the MAC-addrs, else some hardware
> > may drop the TX packet before reaching the wire.  This was
> > observed with driver mlx5.
> >
> > If xdp_rxq_info select --action XDP_TX the swapmac functionality
> > is activated.  It is also possible to manually enable via cmdline
> > option --swapmac.  This is practical if wanting to measure the
> > overhead of writing/updating payload for other action types.
> >
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> > ---
[...]
> >
> > diff --git a/samples/bpf/xdp_rxq_info_kern.c b/samples/bpf/xdp_rxq_info_kern.c
> > index 61af6210df2f..222a83eed1cb 100644
> > --- a/samples/bpf/xdp_rxq_info_kern.c
> > +++ b/samples/bpf/xdp_rxq_info_kern.c
> > @@ -21,6 +21,7 @@ struct config {
> >  enum cfg_options_flags {
> >         NO_TOUCH = 0x0U,
> >         READ_MEM = 0x1U,
> > +       SWAP_MAC = 0x2U,
> >  };
[...]
> > @@ -98,7 +116,7 @@ int  xdp_prognum0(struct xdp_md *ctx)
> >                 rxq_rec->issue++;
> >
> >         /* Default: Don't touch packet data, only count packets */
> > -       if (unlikely(config->options & READ_MEM)) {
> > +       if (unlikely(config->options & (READ_MEM|SWAP_MAC))) {
> >                 struct ethhdr *eth = data;
> >
> >                 if (eth + 1 > data_end)
[...]

> > diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
> > index 435485d4f49e..248a7eab9531 100644
[...]
> > @@ -119,6 +121,8 @@ static char* options2str(enum cfg_options_flags flag)
> >  {
> >         if (flag == NO_TOUCH)
> >                 return "no_touch";
> > +       if (flag & SWAP_MAC)
> > +               return "swapmac";
> >         if (flag & READ_MEM)
> >                 return "read";  
> 
> I guess SWAP_MAC also reads the memory, so it "includes" READ_MEM?

True (see _kern side)

> It is OK for now. We may need to refactor this part when adding other
> flags in the future.

Sure, do remember that this is only a 'sample' program.

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

Re: [bpf-next PATCH 2/2] samples/bpf: xdp_rxq_info action XDP_TX must adjust MAC-addrs

[Song Liu]

On Wed, Jun 27, 2018 at 4:20 AM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> On Tue, 26 Jun 2018 17:09:01 -0700
> Song Liu <liu.song.a23@gmail.com> wrote:
>
>> On Mon, Jun 25, 2018 at 7:27 AM, Jesper Dangaard Brouer
>> <brouer@redhat.com> wrote:
>> > XDP_TX requires also changing the MAC-addrs, else some hardware
>> > may drop the TX packet before reaching the wire.  This was
>> > observed with driver mlx5.
>> >
>> > If xdp_rxq_info select --action XDP_TX the swapmac functionality
>> > is activated.  It is also possible to manually enable via cmdline
>> > option --swapmac.  This is practical if wanting to measure the
>> > overhead of writing/updating payload for other action types.
>> >
>> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>> > Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>> > ---
> [...]
>> >
>> > diff --git a/samples/bpf/xdp_rxq_info_kern.c b/samples/bpf/xdp_rxq_info_kern.c
>> > index 61af6210df2f..222a83eed1cb 100644
>> > --- a/samples/bpf/xdp_rxq_info_kern.c
>> > +++ b/samples/bpf/xdp_rxq_info_kern.c
>> > @@ -21,6 +21,7 @@ struct config {
>> >  enum cfg_options_flags {
>> >         NO_TOUCH = 0x0U,
>> >         READ_MEM = 0x1U,
>> > +       SWAP_MAC = 0x2U,
>> >  };
> [...]
>> > @@ -98,7 +116,7 @@ int  xdp_prognum0(struct xdp_md *ctx)
>> >                 rxq_rec->issue++;
>> >
>> >         /* Default: Don't touch packet data, only count packets */
>> > -       if (unlikely(config->options & READ_MEM)) {
>> > +       if (unlikely(config->options & (READ_MEM|SWAP_MAC))) {
>> >                 struct ethhdr *eth = data;
>> >
>> >                 if (eth + 1 > data_end)
> [...]
>
>> > diff --git a/samples/bpf/xdp_rxq_info_user.c b/samples/bpf/xdp_rxq_info_user.c
>> > index 435485d4f49e..248a7eab9531 100644
> [...]
>> > @@ -119,6 +121,8 @@ static char* options2str(enum cfg_options_flags flag)
>> >  {
>> >         if (flag == NO_TOUCH)
>> >                 return "no_touch";
>> > +       if (flag & SWAP_MAC)
>> > +               return "swapmac";
>> >         if (flag & READ_MEM)
>> >                 return "read";
>>
>> I guess SWAP_MAC also reads the memory, so it "includes" READ_MEM?
>
> True (see _kern side)
>
>> It is OK for now. We may need to refactor this part when adding other
>> flags in the future.
>
> Sure, do remember that this is only a 'sample' program.

Agreed.

Acked-by: Song Liu <songliubraving@fb.com>

Re: [bpf-next PATCH 0/2] xdp/bpf: extend XDP samples/bpf xdp_rxq_info

[Daniel Borkmann]

On 06/25/2018 04:27 PM, Jesper Dangaard Brouer wrote:
> While writing an article about XDP, the samples/bpf xdp_rxq_info
> program were extended to cover some more use-cases.

Applied to bpf-next, thanks guys!