[PATCH bpf-next 0/2] sample: xdp1 improvements

[PATCH bpf-next 0/2] sample: xdp1 improvements

[Matteo Croce]

Small improvements to improve the readability and easiness
to use of the xdp1 sample.

Matteo Croce (2):
  samples: bpf: improve xdp1 example
  samples: bpf: get ifindex from ifname

 samples/bpf/xdp1_user.c | 44 ++++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 12 deletions(-)

-- 
2.19.1

[PATCH bpf-next 1/2] samples: bpf: improve xdp1 example

[Matteo Croce]

Store only the total packet count for every protocol, instead of the
whole per-cpu array.
Use bpf_map_get_next_key() to iterate the map, instead of looking up
all the protocols.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 samples/bpf/xdp1_user.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/samples/bpf/xdp1_user.c b/samples/bpf/xdp1_user.c
index b02c531510ed..4f3d824fc044 100644
--- a/samples/bpf/xdp1_user.c
+++ b/samples/bpf/xdp1_user.c
@@ -34,26 +34,24 @@ static void int_exit(int sig)
 static void poll_stats(int map_fd, int interval)
 {
 	unsigned int nr_cpus = bpf_num_possible_cpus();
-	const unsigned int nr_keys = 256;
-	__u64 values[nr_cpus], prev[nr_keys][nr_cpus];
-	__u32 key;
+	__u64 values[nr_cpus], prev[UINT8_MAX] = { 0 };
 	int i;
 
-	memset(prev, 0, sizeof(prev));
-
 	while (1) {
+		__u32 key = UINT32_MAX;
+
 		sleep(interval);
 
-		for (key = 0; key < nr_keys; key++) {
+		while (bpf_map_get_next_key(map_fd, &key, &key) != -1) {
 			__u64 sum = 0;
 
 			assert(bpf_map_lookup_elem(map_fd, &key, values) == 0);
 			for (i = 0; i < nr_cpus; i++)
-				sum += (values[i] - prev[key][i]);
-			if (sum)
+				sum += values[i];
+			if (sum > prev[key])
 				printf("proto %u: %10llu pkt/s\n",
-				       key, sum / interval);
-			memcpy(prev[key], values, sizeof(values));
+				       key, (sum - prev[key]) / interval);
+			prev[key] = sum;
 		}
 	}
 }
-- 
2.19.1

[PATCH bpf-next 2/2] samples: bpf: get ifindex from ifname

[Matteo Croce]

Find the ifindex via ioctl(SIOCGIFINDEX) instead of requiring the
numeric ifindex.

Signed-off-by: Matteo Croce <mcroce@redhat.com>
---
 samples/bpf/xdp1_user.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/samples/bpf/xdp1_user.c b/samples/bpf/xdp1_user.c
index 4f3d824fc044..a1d0c5dcee9c 100644
--- a/samples/bpf/xdp1_user.c
+++ b/samples/bpf/xdp1_user.c
@@ -15,6 +15,9 @@
 #include <unistd.h>
 #include <libgen.h>
 #include <sys/resource.h>
+#include <sys/ioctl.h>
+#include <sys/socket.h>
+#include <linux/if.h>
 
 #include "bpf_util.h"
 #include "bpf/bpf.h"
@@ -59,7 +62,7 @@ static void poll_stats(int map_fd, int interval)
 static void usage(const char *prog)
 {
 	fprintf(stderr,
-		"usage: %s [OPTS] IFINDEX\n\n"
+		"usage: %s [OPTS] IFACE\n\n"
 		"OPTS:\n"
 		"    -S    use skb-mode\n"
 		"    -N    enforce native mode\n",
@@ -74,9 +77,11 @@ int main(int argc, char **argv)
 	};
 	const char *optstr = "SN";
 	int prog_fd, map_fd, opt;
+	struct ifreq ifr = { 0 };
 	struct bpf_object *obj;
 	struct bpf_map *map;
 	char filename[256];
+	int sock;
 
 	while ((opt = getopt(argc, argv, optstr)) != -1) {
 		switch (opt) {
@@ -102,7 +107,24 @@ int main(int argc, char **argv)
 		return 1;
 	}
 
-	ifindex = strtoul(argv[optind], NULL, 0);
+	sock = socket(AF_UNIX, SOCK_DGRAM, 0);
+	if (sock == -1) {
+		perror("socket");
+		return 1;
+	}
+
+	if (strlen(argv[optind]) >= IFNAMSIZ) {
+		printf("invalid ifname '%s'\n", argv[optind]);
+		return 1;
+	}
+
+	strcpy(ifr.ifr_name, argv[optind]);
+	if (ioctl(sock, SIOCGIFINDEX, &ifr) < 0) {
+		perror("SIOCGIFINDEX");
+		return 1;
+	}
+	close(sock);
+	ifindex = ifr.ifr_ifindex;
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 	prog_load_attr.file = filename;
-- 
2.19.1

Re: [PATCH bpf-next 2/2] samples: bpf: get ifindex from ifname

[John Fastabend]

On 10/18/2018 01:47 PM, Matteo Croce wrote:
> Find the ifindex via ioctl(SIOCGIFINDEX) instead of requiring the
> numeric ifindex.
> 
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> ---

I don't think there are any expectation that samples have to be
stable as far as inputs over versions. And because I consistently
run this with the ifname before realizing its the ifindex not
string name I'll Ack it.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf-next 1/2] samples: bpf: improve xdp1 example

[Y Song]

On Thu, Oct 18, 2018 at 1:48 PM Matteo Croce <mcroce@redhat.com> wrote:
>
> Store only the total packet count for every protocol, instead of the
> whole per-cpu array.
> Use bpf_map_get_next_key() to iterate the map, instead of looking up
> all the protocols.
>
> Signed-off-by: Matteo Croce <mcroce@redhat.com>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf-next 2/2] samples: bpf: get ifindex from ifname

[Y Song]

On Thu, Oct 18, 2018 at 1:48 PM Matteo Croce <mcroce@redhat.com> wrote:
>
> Find the ifindex via ioctl(SIOCGIFINDEX) instead of requiring the
> numeric ifindex.

Maybe use if_nametoindex which is simpler?

>
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
> ---
>  samples/bpf/xdp1_user.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/samples/bpf/xdp1_user.c b/samples/bpf/xdp1_user.c
> index 4f3d824fc044..a1d0c5dcee9c 100644
> --- a/samples/bpf/xdp1_user.c
> +++ b/samples/bpf/xdp1_user.c
> @@ -15,6 +15,9 @@
>  #include <unistd.h>
>  #include <libgen.h>
>  #include <sys/resource.h>
> +#include <sys/ioctl.h>
> +#include <sys/socket.h>
> +#include <linux/if.h>
>
>  #include "bpf_util.h"
>  #include "bpf/bpf.h"
> @@ -59,7 +62,7 @@ static void poll_stats(int map_fd, int interval)
>  static void usage(const char *prog)
>  {
>         fprintf(stderr,
> -               "usage: %s [OPTS] IFINDEX\n\n"
> +               "usage: %s [OPTS] IFACE\n\n"
>                 "OPTS:\n"
>                 "    -S    use skb-mode\n"
>                 "    -N    enforce native mode\n",
> @@ -74,9 +77,11 @@ int main(int argc, char **argv)
>         };
>         const char *optstr = "SN";
>         int prog_fd, map_fd, opt;
> +       struct ifreq ifr = { 0 };
>         struct bpf_object *obj;
>         struct bpf_map *map;
>         char filename[256];
> +       int sock;
>
>         while ((opt = getopt(argc, argv, optstr)) != -1) {
>                 switch (opt) {
> @@ -102,7 +107,24 @@ int main(int argc, char **argv)
>                 return 1;
>         }
>
> -       ifindex = strtoul(argv[optind], NULL, 0);
> +       sock = socket(AF_UNIX, SOCK_DGRAM, 0);
> +       if (sock == -1) {
> +               perror("socket");
> +               return 1;
> +       }
> +
> +       if (strlen(argv[optind]) >= IFNAMSIZ) {
> +               printf("invalid ifname '%s'\n", argv[optind]);
> +               return 1;
> +       }
> +
> +       strcpy(ifr.ifr_name, argv[optind]);
> +       if (ioctl(sock, SIOCGIFINDEX, &ifr) < 0) {
> +               perror("SIOCGIFINDEX");
> +               return 1;
> +       }
> +       close(sock);
> +       ifindex = ifr.ifr_ifindex;
>
>         snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
>         prog_load_attr.file = filename;
> --
> 2.19.1
>

Re: [PATCH bpf-next 2/2] samples: bpf: get ifindex from ifname

[Matteo Croce]

On Fri, Oct 19, 2018 at 5:35 AM Y Song <ys114321@gmail.com> wrote:
>
> On Thu, Oct 18, 2018 at 1:48 PM Matteo Croce <mcroce@redhat.com> wrote:
> >
> > Find the ifindex via ioctl(SIOCGIFINDEX) instead of requiring the
> > numeric ifindex.
>
> Maybe use if_nametoindex which is simpler?
>
> >
> > Signed-off-by: Matteo Croce <mcroce@redhat.com>
> > ---
> >  samples/bpf/xdp1_user.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/samples/bpf/xdp1_user.c b/samples/bpf/xdp1_user.c
> > index 4f3d824fc044..a1d0c5dcee9c 100644
> > --- a/samples/bpf/xdp1_user.c
> > +++ b/samples/bpf/xdp1_user.c
> > @@ -15,6 +15,9 @@
> >  #include <unistd.h>
> >  #include <libgen.h>
> >  #include <sys/resource.h>
> > +#include <sys/ioctl.h>
> > +#include <sys/socket.h>
> > +#include <linux/if.h>
> >
> >  #include "bpf_util.h"
> >  #include "bpf/bpf.h"
> > @@ -59,7 +62,7 @@ static void poll_stats(int map_fd, int interval)
> >  static void usage(const char *prog)
> >  {
> >         fprintf(stderr,
> > -               "usage: %s [OPTS] IFINDEX\n\n"
> > +               "usage: %s [OPTS] IFACE\n\n"
> >                 "OPTS:\n"
> >                 "    -S    use skb-mode\n"
> >                 "    -N    enforce native mode\n",
> > @@ -74,9 +77,11 @@ int main(int argc, char **argv)
> >         };
> >         const char *optstr = "SN";
> >         int prog_fd, map_fd, opt;
> > +       struct ifreq ifr = { 0 };
> >         struct bpf_object *obj;
> >         struct bpf_map *map;
> >         char filename[256];
> > +       int sock;
> >
> >         while ((opt = getopt(argc, argv, optstr)) != -1) {
> >                 switch (opt) {
> > @@ -102,7 +107,24 @@ int main(int argc, char **argv)
> >                 return 1;
> >         }
> >
> > -       ifindex = strtoul(argv[optind], NULL, 0);
> > +       sock = socket(AF_UNIX, SOCK_DGRAM, 0);
> > +       if (sock == -1) {
> > +               perror("socket");
> > +               return 1;
> > +       }
> > +
> > +       if (strlen(argv[optind]) >= IFNAMSIZ) {
> > +               printf("invalid ifname '%s'\n", argv[optind]);
> > +               return 1;
> > +       }
> > +
> > +       strcpy(ifr.ifr_name, argv[optind]);
> > +       if (ioctl(sock, SIOCGIFINDEX, &ifr) < 0) {
> > +               perror("SIOCGIFINDEX");
> > +               return 1;
> > +       }
> > +       close(sock);
> > +       ifindex = ifr.ifr_ifindex;
> >
> >         snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> >         prog_load_attr.file = filename;
> > --
> > 2.19.1
> >

Right, even better. Will do a v2, thanks.

-- 
Matteo Croce
per aspera ad upstream