Netdev List
 help / color / mirror / Atom feed
* [PATCH bpf-next 06/11] tools: bpftool: allow users to specify program type for prog load
From: Jakub Kicinski @ 2018-07-04  2:54 UTC (permalink / raw)
  To: alexei.starovoitov, daniel; +Cc: oss-drivers, netdev, Jakub Kicinski
In-Reply-To: <20180704025417.8848-1-jakub.kicinski@netronome.com>

Sometimes program section names don't match with libbpf's expectation.
In particular XDP's default section names differ between libbpf and
iproute2.  Allow users to pass program type on command line.  Name
the types like the libbpf expected section names.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 .../bpftool/Documentation/bpftool-prog.rst    | 15 ++++++-
 tools/bpf/bpftool/bash-completion/bpftool     |  6 +++
 tools/bpf/bpftool/prog.c                      | 44 +++++++++++++++++--
 3 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 41723c6acaa6..e53e1ad2caf0 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -24,10 +24,19 @@ MAP COMMANDS
 |	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual**}]
 |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes**}]
 |	**bpftool** **prog pin** *PROG* *FILE*
-|	**bpftool** **prog load** *OBJ* *FILE* [**dev** *NAME*]
+|	**bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**dev** *NAME*]
 |	**bpftool** **prog help**
 |
 |	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
+|	*TYPE* := {
+|		**socket** | **kprobe** | **kretprobe** | **classifier** | **action** |
+|		**tracepoint** | **raw_tracepoint** | **xdp** | **perf_event** | **cgroup/skb** |
+|		**cgroup/sock** | **cgroup/dev** | **lwt_in** | **lwt_out** | **lwt_xmit** |
+|		**lwt_seg6local** | **sockops** | **sk_skb** | **sk_msg** | **lirc_mode2** |
+|		**cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** | **cgroup/post_bind6** |
+|		**cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4** | **cgroup/sendmsg6**
+|	}
+
 
 DESCRIPTION
 ===========
@@ -64,8 +73,10 @@ DESCRIPTION
 
 		  Note: *FILE* must be located in *bpffs* mount.
 
-	**bpftool prog load** *OBJ* *FILE* [**dev** *NAME*]
+	**bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**dev** *NAME*]
 		  Load bpf program from binary *OBJ* and pin as *FILE*.
+		  **type** is optional, if not specified program type will be
+		  inferred from section names.
 		  If **dev** *NAME* is specified program will be loaded onto
 		  given networking device (offload).
 
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index ab044f528a85..9ae33a73d732 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -274,11 +274,17 @@ _bpftool()
                     fi
 
                     case $prev in
+                        type)
+                            COMPREPLY=( $( compgen -W "socket kprobe kretprobe classifier action tracepoint raw_tracepoint xdp perf_event cgroup/skb cgroup/sock cgroup/dev lwt_in lwt_out lwt_xmit lwt_seg6local sockops sk_skb sk_msg lirc_mode2 cgroup/bind4 cgroup/bind6 cgroup/connect4 cgroup/connect6 cgroup/sendmsg4 cgroup/sendmsg6 cgroup/post_bind4 cgroup/post_bind6" -- \
+                                                   "$cur" ) )
+                            return 0
+                            ;;
                         dev)
                             _sysfs_get_netdevs
                             return 0
                             ;;
                         *)
+                            _bpftool_once_attr 'type'
                             _bpftool_once_attr 'dev'
                             return 0
                             ;;
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 21c74de7156f..7a06fd4c5d27 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -688,6 +688,7 @@ static int do_load(int argc, char **argv)
 	const char *objfile, *pinfile;
 	struct bpf_object *obj;
 	int prog_fd;
+	int err;
 
 	if (!REQ_ARGS(2))
 		return -1;
@@ -695,7 +696,37 @@ static int do_load(int argc, char **argv)
 	pinfile = GET_ARG();
 
 	while (argc) {
-		if (is_prefix(*argv, "dev")) {
+		if (is_prefix(*argv, "type")) {
+			char *type;
+
+			NEXT_ARG();
+
+			if (attr.prog_type != BPF_PROG_TYPE_UNSPEC) {
+				p_err("program type already specified");
+				return -1;
+			}
+			if (!REQ_ARGS(1))
+				return -1;
+
+			/* Put a '/' at the end of type to appease libbpf */
+			type = malloc(strlen(*argv) + 2);
+			if (!type) {
+				p_err("mem alloc failed");
+				return -1;
+			}
+			*type = 0;
+			strcat(type, *argv);
+			strcat(type, "/");
+
+			err = libbpf_prog_type_by_string(type, &attr.prog_type,
+							 &attr.expected_attach_type);
+			free(type);
+			if (err < 0) {
+				p_err("unknown program type '%s'", *argv);
+				return err;
+			}
+			NEXT_ARG();
+		} else if (is_prefix(*argv, "dev")) {
 			NEXT_ARG();
 
 			if (attr.ifindex) {
@@ -713,7 +744,7 @@ static int do_load(int argc, char **argv)
 			}
 			NEXT_ARG();
 		} else {
-			p_err("expected no more arguments or 'dev', got: '%s'?",
+			p_err("expected no more arguments, 'type' or 'dev', got: '%s'?",
 			      *argv);
 			return -1;
 		}
@@ -753,10 +784,17 @@ static int do_help(int argc, char **argv)
 		"       %s %s dump xlated PROG [{ file FILE | opcodes | visual }]\n"
 		"       %s %s dump jited  PROG [{ file FILE | opcodes }]\n"
 		"       %s %s pin   PROG FILE\n"
-		"       %s %s load  OBJ  FILE [dev NAME]\n"
+		"       %s %s load  OBJ  FILE [type TYPE] [dev NAME]\n"
 		"       %s %s help\n"
 		"\n"
 		"       " HELP_SPEC_PROGRAM "\n"
+		"       TYPE := { socket | kprobe | kretprobe | classifier | action |\n"
+		"                 tracepoint | raw_tracepoint | xdp | perf_event | cgroup/skb |\n"
+		"                 cgroup/sock | cgroup/dev | lwt_in | lwt_out | lwt_xmit |\n"
+		"                 lwt_seg6local | sockops | sk_skb | sk_msg | lirc_mode2 |\n"
+		"                 cgroup/bind4 | cgroup/bind6 | cgroup/post_bind4 |\n"
+		"                 cgroup/post_bind6 | cgroup/connect4 | cgroup/connect6 |\n"
+		"                 cgroup/sendmsg4 | cgroup/sendmsg6 }\n"
 		"       " HELP_SPEC_OPTIONS "\n"
 		"",
 		bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
-- 
2.17.1

^ permalink raw reply related

* [PATCH bpf-next 05/11] tools: libbpf: expose the prog type guessing from section name logic
From: Jakub Kicinski @ 2018-07-04  2:54 UTC (permalink / raw)
  To: alexei.starovoitov, daniel; +Cc: oss-drivers, netdev, Jakub Kicinski
In-Reply-To: <20180704025417.8848-1-jakub.kicinski@netronome.com>

libbpf can guess program type based on ELF section names.  As libbpf
becomes more popular its association between section name strings and
types becomes more of a standard.  Allow libbpf users to use the same
logic for matching strings to types, e.g. when the string originates
from command line.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 tools/lib/bpf/libbpf.c | 43 ++++++++++++++++++++++++------------------
 tools/lib/bpf/libbpf.h |  3 +++
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 38ed3e92e393..30f3e58bd563 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2081,25 +2081,33 @@ static const struct {
 #undef BPF_S_PROG_SEC
 #undef BPF_SA_PROG_SEC
 
-static int bpf_program__identify_section(struct bpf_program *prog)
+int libbpf_prog_type_by_string(const char *name, enum bpf_prog_type *prog_type,
+			       enum bpf_attach_type *expected_attach_type)
 {
 	int i;
 
-	if (!prog->section_name)
-		goto err;
-
-	for (i = 0; i < ARRAY_SIZE(section_names); i++)
-		if (strncmp(prog->section_name, section_names[i].sec,
-			    section_names[i].len) == 0)
-			return i;
-
-err:
-	pr_warning("failed to guess program type based on section name %s\n",
-		   prog->section_name);
+	if (!name)
+		return -1;
 
+	for (i = 0; i < ARRAY_SIZE(section_names); i++) {
+		if (strncmp(name, section_names[i].sec, section_names[i].len))
+			continue;
+		*prog_type = section_names[i].prog_type;
+		*expected_attach_type = section_names[i].expected_attach_type;
+		return 0;
+	}
 	return -1;
 }
 
+static int
+bpf_program__identify_section(struct bpf_program *prog,
+			      enum bpf_prog_type *prog_type,
+			      enum bpf_attach_type *expected_attach_type)
+{
+	return libbpf_prog_type_by_string(prog->section_name, prog_type,
+					  expected_attach_type);
+}
+
 int bpf_map__fd(struct bpf_map *map)
 {
 	return map ? map->fd : -EINVAL;
@@ -2230,7 +2238,6 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
 	enum bpf_prog_type prog_type;
 	struct bpf_object *obj;
 	struct bpf_map *map;
-	int section_idx;
 	int err;
 
 	if (!attr)
@@ -2252,14 +2259,14 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
 		prog->prog_ifindex = attr->ifindex;
 		expected_attach_type = attr->expected_attach_type;
 		if (prog_type == BPF_PROG_TYPE_UNSPEC) {
-			section_idx = bpf_program__identify_section(prog);
-			if (section_idx < 0) {
+			err = bpf_program__identify_section(prog, &prog_type,
+							    &expected_attach_type);
+			if (err < 0) {
+				pr_warning("failed to guess program type based on section name %s\n",
+					   prog->section_name);
 				bpf_object__close(obj);
 				return -EINVAL;
 			}
-			prog_type = section_names[section_idx].prog_type;
-			expected_attach_type =
-				section_names[section_idx].expected_attach_type;
 		}
 
 		bpf_program__set_type(prog, prog_type);
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 564f4be9bae0..617dacfc6704 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -92,6 +92,9 @@ int bpf_object__set_priv(struct bpf_object *obj, void *priv,
 			 bpf_object_clear_priv_t clear_priv);
 void *bpf_object__priv(struct bpf_object *prog);
 
+int libbpf_prog_type_by_string(const char *name, enum bpf_prog_type *prog_type,
+			       enum bpf_attach_type *expected_attach_type);
+
 /* Accessors of bpf_program */
 struct bpf_program;
 struct bpf_program *bpf_program__next(struct bpf_program *prog,
-- 
2.17.1

^ permalink raw reply related

* [PATCH bpf-next 04/11] tools: bpftool: add support for loading programs for offload
From: Jakub Kicinski @ 2018-07-04  2:54 UTC (permalink / raw)
  To: alexei.starovoitov, daniel; +Cc: oss-drivers, netdev, Jakub Kicinski
In-Reply-To: <20180704025417.8848-1-jakub.kicinski@netronome.com>

Extend the bpftool prog load command to also accept "dev"
parameter, which will allow us to load programs onto devices.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 .../bpftool/Documentation/bpftool-prog.rst    |  6 ++--
 tools/bpf/bpftool/bash-completion/bpftool     | 23 ++++++++++--
 tools/bpf/bpftool/prog.c                      | 35 +++++++++++++++++--
 3 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 43d34a5c3ec5..41723c6acaa6 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -24,7 +24,7 @@ MAP COMMANDS
 |	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual**}]
 |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes**}]
 |	**bpftool** **prog pin** *PROG* *FILE*
-|	**bpftool** **prog load** *OBJ* *FILE*
+|	**bpftool** **prog load** *OBJ* *FILE* [**dev** *NAME*]
 |	**bpftool** **prog help**
 |
 |	*PROG* := { **id** *PROG_ID* | **pinned** *FILE* | **tag** *PROG_TAG* }
@@ -64,8 +64,10 @@ DESCRIPTION
 
 		  Note: *FILE* must be located in *bpffs* mount.
 
-	**bpftool prog load** *OBJ* *FILE*
+	**bpftool prog load** *OBJ* *FILE* [**dev** *NAME*]
 		  Load bpf program from binary *OBJ* and pin as *FILE*.
+		  If **dev** *NAME* is specified program will be loaded onto
+		  given networking device (offload).
 
 		  Note: *FILE* must be located in *bpffs* mount.
 
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index fffd76f4998b..ab044f528a85 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -99,6 +99,12 @@ _bpftool_get_prog_tags()
         command sed -n 's/.*"tag": "\(.*\)",$/\1/p' )" -- "$cur" ) )
 }
 
+_sysfs_get_netdevs()
+{
+    COMPREPLY+=( $( compgen -W "$( ls /sys/class/net 2>/dev/null )" -- \
+        "$cur" ) )
+}
+
 # For bpftool map update: retrieve type of the map to update.
 _bpftool_map_update_map_type()
 {
@@ -262,8 +268,21 @@ _bpftool()
                     return 0
                     ;;
                 load)
-                    _filedir
-                    return 0
+                    if [[ ${#words[@]} -lt 6 ]]; then
+                        _filedir
+                        return 0
+                    fi
+
+                    case $prev in
+                        dev)
+                            _sysfs_get_netdevs
+                            return 0
+                            ;;
+                        *)
+                            _bpftool_once_attr 'dev'
+                            return 0
+                            ;;
+                    esac
                     ;;
                 *)
                     [[ $prev == $object ]] && \
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index a5ef46c59029..21c74de7156f 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -39,6 +39,7 @@
 #include <string.h>
 #include <time.h>
 #include <unistd.h>
+#include <net/if.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 
@@ -681,6 +682,9 @@ static int do_pin(int argc, char **argv)
 
 static int do_load(int argc, char **argv)
 {
+	struct bpf_prog_load_attr attr = {
+		.prog_type	= BPF_PROG_TYPE_UNSPEC,
+	};
 	const char *objfile, *pinfile;
 	struct bpf_object *obj;
 	int prog_fd;
@@ -690,7 +694,34 @@ static int do_load(int argc, char **argv)
 	objfile = GET_ARG();
 	pinfile = GET_ARG();
 
-	if (bpf_prog_load(objfile, BPF_PROG_TYPE_UNSPEC, &obj, &prog_fd)) {
+	while (argc) {
+		if (is_prefix(*argv, "dev")) {
+			NEXT_ARG();
+
+			if (attr.ifindex) {
+				p_err("offload device already specified");
+				return -1;
+			}
+			if (!REQ_ARGS(1))
+				return -1;
+
+			attr.ifindex = if_nametoindex(*argv);
+			if (!attr.ifindex) {
+				p_err("unrecognized netdevice '%s': %s",
+				      *argv, strerror(errno));
+				return -1;
+			}
+			NEXT_ARG();
+		} else {
+			p_err("expected no more arguments or 'dev', got: '%s'?",
+			      *argv);
+			return -1;
+		}
+	}
+
+	attr.file = objfile;
+
+	if (bpf_prog_load_xattr(&attr, &obj, &prog_fd)) {
 		p_err("failed to load program");
 		return -1;
 	}
@@ -722,7 +753,7 @@ static int do_help(int argc, char **argv)
 		"       %s %s dump xlated PROG [{ file FILE | opcodes | visual }]\n"
 		"       %s %s dump jited  PROG [{ file FILE | opcodes }]\n"
 		"       %s %s pin   PROG FILE\n"
-		"       %s %s load  OBJ  FILE\n"
+		"       %s %s load  OBJ  FILE [dev NAME]\n"
 		"       %s %s help\n"
 		"\n"
 		"       " HELP_SPEC_PROGRAM "\n"
-- 
2.17.1

^ permalink raw reply related

* [PATCH bpf-next 03/11] tools: bpftool: refactor argument parsing for prog load
From: Jakub Kicinski @ 2018-07-04  2:54 UTC (permalink / raw)
  To: alexei.starovoitov, daniel; +Cc: oss-drivers, netdev, Jakub Kicinski
In-Reply-To: <20180704025417.8848-1-jakub.kicinski@netronome.com>

Add a new macro for printing more informative message than straight
usage() when parameters are missing, and use it for prog do_load().
Save the object and pin path argument to variables for clarity.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 tools/bpf/bpftool/main.h | 15 +++++++++++++++
 tools/bpf/bpftool/prog.c | 11 +++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index d39f7ef01d23..15b6c49ae533 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -50,6 +50,21 @@
 #define NEXT_ARG()	({ argc--; argv++; if (argc < 0) usage(); })
 #define NEXT_ARGP()	({ (*argc)--; (*argv)++; if (*argc < 0) usage(); })
 #define BAD_ARG()	({ p_err("what is '%s'?", *argv); -1; })
+#define GET_ARG()	({ argc--; *argv++; })
+#define REQ_ARGS(cnt)							\
+	({								\
+		int _cnt = (cnt);					\
+		bool _res;						\
+									\
+		if (argc < _cnt) {					\
+			p_err("'%s' needs at least %d arguments, %d found", \
+			      argv[-1], _cnt, argc);			\
+			_res = false;					\
+		} else {						\
+			_res = true;					\
+		}							\
+		_res;							\
+	})
 
 #define ERR_MAX_LEN	1024
 
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index a740da99d477..a5ef46c59029 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -681,18 +681,21 @@ static int do_pin(int argc, char **argv)
 
 static int do_load(int argc, char **argv)
 {
+	const char *objfile, *pinfile;
 	struct bpf_object *obj;
 	int prog_fd;
 
-	if (argc != 2)
-		usage();
+	if (!REQ_ARGS(2))
+		return -1;
+	objfile = GET_ARG();
+	pinfile = GET_ARG();
 
-	if (bpf_prog_load(argv[0], BPF_PROG_TYPE_UNSPEC, &obj, &prog_fd)) {
+	if (bpf_prog_load(objfile, BPF_PROG_TYPE_UNSPEC, &obj, &prog_fd)) {
 		p_err("failed to load program");
 		return -1;
 	}
 
-	if (do_pin_fd(prog_fd, argv[1]))
+	if (do_pin_fd(prog_fd, pinfile))
 		goto err_close_obj;
 
 	if (json_output)
-- 
2.17.1

^ permalink raw reply related

* [PATCH bpf-next 02/11] selftests/bpf: add Error: prefix in check_extack helper
From: Jakub Kicinski @ 2018-07-04  2:54 UTC (permalink / raw)
  To: alexei.starovoitov, daniel; +Cc: oss-drivers, netdev, Jakub Kicinski
In-Reply-To: <20180704025417.8848-1-jakub.kicinski@netronome.com>

Currently the test only checks errors, not warnings, so save typing
and prefix the extack messages with "Error:" inside the check helper.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 tools/testing/selftests/bpf/test_offload.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_offload.py b/tools/testing/selftests/bpf/test_offload.py
index a257e4b08392..f8d9bd81d9a4 100755
--- a/tools/testing/selftests/bpf/test_offload.py
+++ b/tools/testing/selftests/bpf/test_offload.py
@@ -547,11 +547,11 @@ netns = [] # net namespaces to be removed
     if skip_extack:
         return
     lines = output.split("\n")
-    comp = len(lines) >= 2 and lines[1] == reference
+    comp = len(lines) >= 2 and lines[1] == 'Error: ' + reference
     fail(not comp, "Missing or incorrect netlink extack message")
 
 def check_extack_nsim(output, reference, args):
-    check_extack(output, "Error: netdevsim: " + reference, args)
+    check_extack(output, "netdevsim: " + reference, args)
 
 def check_no_extack(res, needle):
     fail((res[1] + res[2]).count(needle) or (res[1] + res[2]).count("Warning:"),
@@ -654,7 +654,7 @@ netns = []
     ret, _, err = sim.cls_bpf_add_filter(obj, skip_sw=True,
                                          fail=False, include_stderr=True)
     fail(ret == 0, "TC filter loaded without enabling TC offloads")
-    check_extack(err, "Error: TC offload is disabled on net device.", args)
+    check_extack(err, "TC offload is disabled on net device.", args)
     sim.wait_for_flush()
 
     sim.set_ethtool_tc_offloads(True)
@@ -694,7 +694,7 @@ netns = []
                                          skip_sw=True,
                                          fail=False, include_stderr=True)
     fail(ret == 0, "Offloaded a filter to chain other than 0")
-    check_extack(err, "Error: Driver supports only offload of chain 0.", args)
+    check_extack(err, "Driver supports only offload of chain 0.", args)
     sim.tc_flush_filters()
 
     start_test("Test TC replace...")
-- 
2.17.1

^ permalink raw reply related

* [PATCH bpf-next 01/11] selftests/bpf: remove duplicated word from test offloads
From: Jakub Kicinski @ 2018-07-04  2:54 UTC (permalink / raw)
  To: alexei.starovoitov, daniel; +Cc: oss-drivers, netdev, Jakub Kicinski
In-Reply-To: <20180704025417.8848-1-jakub.kicinski@netronome.com>

Trivial removal of duplicated "mode" in error message.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
 tools/testing/selftests/bpf/test_offload.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_offload.py b/tools/testing/selftests/bpf/test_offload.py
index be800d0e7a84..a257e4b08392 100755
--- a/tools/testing/selftests/bpf/test_offload.py
+++ b/tools/testing/selftests/bpf/test_offload.py
@@ -830,7 +830,7 @@ netns = []
     check_extack_nsim(err, "program loaded with different flags.", args)
     ret, _, err = sim.unset_xdp("", force=True,
                                 fail=False, include_stderr=True)
-    fail(ret == 0, "Removed program with a bad mode mode")
+    fail(ret == 0, "Removed program with a bad mode")
     check_extack_nsim(err, "program loaded with different flags.", args)
 
     start_test("Test MTU restrictions...")
-- 
2.17.1

^ permalink raw reply related

* [PATCH bpf-next 00/11] tools: bpf: extend bpftool prog load
From: Jakub Kicinski @ 2018-07-04  2:54 UTC (permalink / raw)
  To: alexei.starovoitov, daniel; +Cc: oss-drivers, netdev, Jakub Kicinski

Hi!

This series starts with two minor clean ups to test_offload.py
selftest script.

The next 9 patches extend the abilities of bpftool prog load
beyond the simple cgroup use cases.  Three new parameters are
added:

 - type - allows specifying program type, independent of how
	  code sections are named;
 - map  - allows reusing existing maps, instead of creating a new
	  map on every program load;
 - dev  - offload/binding to a device.

A number of changes to libbpf is required to accomplish the task.
The section - program type logic mapping is exposed.  We should
probably aim to use the libbpf program section naming everywhere.
For reuse of maps we need to allow users to set FD for bpf map
object in libbpf.

Examples

Load program my_xdp.o and pin it as /sys/fs/bpf/my_xdp, for xdp
program type:

$ bpftool prog load my_xdp.o /sys/fs/bpf/my_xdp \
  type xdp

As above but for offload:

$ bpftool prog load my_xdp.o /sys/fs/bpf/my_xdp \
  type xdp \
  dev netdevsim0

Load program my_maps.o, but for the first map reuse map id 17,
and for the map called "other_map" reuse pinned map /sys/fs/bpf/map0:

$ bpftool prog load my_maps.o /sys/fs/bpf/prog \
  map idx 0 id 17 \
  map name other_map pinned /sys/fs/bpf/map0

---
This set needs a net-next -> bpf-next merge back, which I believe is
imminent.  It should not conflict with Okash's work on BTF.

Jakub Kicinski (11):
  selftests/bpf: remove duplicated word from test offloads
  selftests/bpf: add Error: prefix in check_extack helper
  tools: bpftool: refactor argument parsing for prog load
  tools: bpftool: add support for loading programs for offload
  tools: libbpf: expose the prog type guessing from section name logic
  tools: bpftool: allow users to specify program type for prog load
  tools: libbpf: recognize offload neutral maps
  tools: libbpf: add extended attributes version of bpf_object__open()
  tools: bpftool: reimplement bpf_prog_load() for prog load
  tools: libbpf: allow map reuse
  tools: bpftool: allow reuse of maps with bpftool prog load

 .../bpftool/Documentation/bpftool-prog.rst    |  33 ++-
 tools/bpf/bpftool/bash-completion/bpftool     |  96 ++++++-
 tools/bpf/bpftool/main.h                      |  18 ++
 tools/bpf/bpftool/map.c                       |   4 +-
 tools/bpf/bpftool/prog.c                      | 245 +++++++++++++++++-
 tools/lib/bpf/libbpf.c                        | 107 ++++++--
 tools/lib/bpf/libbpf.h                        |  11 +
 tools/testing/selftests/bpf/test_offload.py   |  10 +-
 8 files changed, 476 insertions(+), 48 deletions(-)

-- 
2.17.1

^ permalink raw reply

* Re: [PATCH] net: phy: marvell: change default m88e1510 LED configuration
From: David Miller @ 2018-07-04  2:35 UTC (permalink / raw)
  To: dongsheng.wang
  Cc: andrew, clemens.gruber, kstewart, pombredanne, tglx, gregkh,
	netdev
In-Reply-To: <1530512146-7012-1-git-send-email-dongsheng.wang@hxt-semitech.com>

From: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
Date: Sun, 1 Jul 2018 23:15:46 -0700

> The m88e1121 LED default configuration does not apply m88e151x.
> So add a function to relpace m88e1121 LED configuration.
> 
> Signed-off-by: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>

Applid, thank you.

^ permalink raw reply

* Re: [PATCH 3/3 v2] net: dsa: Add Vitesse VSC73xx DSA router driver
From: David Miller @ 2018-07-04  2:32 UTC (permalink / raw)
  To: linus.walleij
  Cc: andrew, vivien.didelot, f.fainelli, netdev, openwrt-devel,
	lede-dev, juhosg
In-Reply-To: <20180630111731.19551-3-linus.walleij@linaro.org>

From: Linus Walleij <linus.walleij@linaro.org>
Date: Sat, 30 Jun 2018 13:17:31 +0200

> This adds a DSA driver for:
> 
> Vitesse VSC7385 SparX-G5 5-port Integrated Gigabit Ethernet Switch
> Vitesse VSC7388 SparX-G8 8-port Integrated Gigabit Ethernet Switch
> Vitesse VSC7395 SparX-G5e 5+1-port Integrated Gigabit Ethernet Switch
> Vitesse VSC7398 SparX-G8e 8-port Integrated Gigabit Ethernet Switch
> 
> These switches have a built-in 8051 CPU and can download and execute
> firmware in this CPU. They can also be configured to use an external
> CPU handling the switch in a memory-mapped manner by connecting to
> that external CPU's memory bus.
> 
> This driver (currently) only takes control of the switch chip over
> SPI and configures it to route packages around when connected to a
> CPU port. The chip has embedded PHYs and VLAN support so we model it
> using DSA as a best fit so we can easily add VLAN support and maybe
> later also exploit the internal frame header to get more direct
> control over the switch.
> 
> The four built-in GPIO lines are exposed using a standard GPIO chip.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Update .get_strings() and .get_sset_count() to match the signature
>   with the new argument for sset type.
> - Drop DSA trailer select from Kconfig.
> - Use MII_* namespace definitions instead of hard-coded hex
>   values and calls to read into the PHY: instead use the genphy
>   decoded link state already present in the phydev.
> - Drop extraneous port number check in .enable() and .disable(),
>   this should not happen.
> - Move the GPIO chip set-up into a separate function.
> - Add some missing static in front of a counter function.
> - Drop the bool flags in the state container, use some macros with
>   the chipid to identify model instead like IS_VSC739X().
> - Check phydev->interface() for configuring the CPU port into
>   RGMII mode.

Applied.

^ permalink raw reply

* Re: [PATCH 2/3 v2] net: phy: vitesse: Add support for VSC73xx
From: David Miller @ 2018-07-04  2:32 UTC (permalink / raw)
  To: linus.walleij
  Cc: andrew, vivien.didelot, f.fainelli, netdev, openwrt-devel,
	lede-dev, juhosg
In-Reply-To: <20180630111731.19551-2-linus.walleij@linaro.org>

From: Linus Walleij <linus.walleij@linaro.org>
Date: Sat, 30 Jun 2018 13:17:30 +0200

> The VSC7385, VSC7388, VSC7395 and VSC7398 are integrated
> switch/router chips for 5+1 or 8-port switches/routers. When
> managed directly by Linux using DSA we need to do a special
> set-up "dance" on the PHY. Unfortunately these sequences
> switches the PHY to undocumented pages named 2a30 and 52b6
> and does undocumented things. It is described by these opaque
> sequences also in the reference manual. This is a best
> effort to integrate it anyways.
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Drop <linux/delay.h> from an earlier iteration.
> - Implement an .config_aneg() routine that does nothing: the
>   imroved genphy_config_aneg() makes the device fail as of
>   v4.18-rc1 and the device seems to feel best like this: it
>   comes up in autonegotiation mode and we do not try to instruct
>   it.
> - Use some MII defines when reading/writing registers.
> - Collect Florian's ACK.

Applied.

^ permalink raw reply

* Re: [PATCH 1/3 v2] net: dsa: Add DT bindings for Vitesse VSC73xx switches
From: David Miller @ 2018-07-04  2:32 UTC (permalink / raw)
  To: linus.walleij
  Cc: andrew, vivien.didelot, f.fainelli, netdev, openwrt-devel,
	lede-dev, juhosg, devicetree
In-Reply-To: <20180630111731.19551-1-linus.walleij@linaro.org>

From: Linus Walleij <linus.walleij@linaro.org>
Date: Sat, 30 Jun 2018 13:17:29 +0200

> This adds the device tree bindings for the Vitesse VSC73xx
> switches. We also add the vendor name for Vitesse.
> 
> Cc: devicetree@vger.kernel.org
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Fix spelling error
> - Properly reference the GPIO bindings
> - Collect Florians ACK

Applied.

^ permalink raw reply

* Re: [PATCHv2 net] sctp: fix the issue that pathmtu may be set lower than MINSEGMENT
From: Marcelo Ricardo Leitner @ 2018-07-04  2:28 UTC (permalink / raw)
  To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman, syzkaller
In-Reply-To: <0ebc621b57952699c67c558dde3ce61b784e3f74.1530606647.git.lucien.xin@gmail.com>

On Tue, Jul 03, 2018 at 04:30:47PM +0800, Xin Long wrote:
> After commit b6c5734db070 ("sctp: fix the handling of ICMP Frag Needed
> for too small MTUs"), sctp_transport_update_pmtu would refetch pathmtu
> from the dst and set it to transport's pathmtu without any check.
> 
> The new pathmtu may be lower than MINSEGMENT if the dst is obsolete and
> updated by .get_dst() in sctp_transport_update_pmtu. In this case, it
> could have a smaller MTU as well, and thus we should validate it
> against MINSEGMENT instead.
> 
> Syzbot reported a warning in sctp_mtu_payload caused by this.
> 
> This patch refetches the pathmtu by calling sctp_dst_mtu where it does
> the check against MINSEGMENT.
> 
> v1->v2:
>   - refetch the pathmtu by calling sctp_dst_mtu instead as Marcelo's
>     suggestion.
> 
> Fixes: b6c5734db070 ("sctp: fix the handling of ICMP Frag Needed for too small MTUs")
> Reported-by: syzbot+f0d9d7cba052f9344b03@syzkaller.appspotmail.com
> Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/transport.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 445b7ef..12cac85 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -282,7 +282,7 @@ bool sctp_transport_update_pmtu(struct sctp_transport *t, u32 pmtu)
>  
>  	if (dst) {
>  		/* Re-fetch, as under layers may have a higher minimum size */
> -		pmtu = SCTP_TRUNC4(dst_mtu(dst));
> +		pmtu = sctp_dst_mtu(dst);
>  		change = t->pathmtu != pmtu;
>  	}
>  	t->pathmtu = pmtu;
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply

* Re: [PATCH net-next v4 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Tonghao Zhang @ 2018-07-04  1:40 UTC (permalink / raw)
  To: jasowang
  Cc: mst, makita.toshiaki, virtualization,
	Linux Kernel Network Developers, Tonghao Zhang
In-Reply-To: <2f5d20a0-cc95-ed37-3f6c-3368f92a7586@redhat.com>

On Tue, Jul 3, 2018 at 1:55 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2018年07月03日 13:48, Tonghao Zhang wrote:
> > On Tue, Jul 3, 2018 at 10:12 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >>
> >> On 2018年07月02日 20:57, xiangxia.m.yue@gmail.com wrote:
> >>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> >>>
> >>> Factor out generic busy polling logic and will be
> >>> used for in tx path in the next patch. And with the patch,
> >>> qemu can set differently the busyloop_timeout for rx queue.
> >>>
> >>> Signed-off-by: Tonghao Zhang <zhangtonghao@didichuxing.com>
> >>> ---
> >>>    drivers/vhost/net.c | 94 +++++++++++++++++++++++++++++++----------------------
> >>>    1 file changed, 55 insertions(+), 39 deletions(-)
> >>>
> >>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> >>> index 62bb8e8..2790959 100644
> >>> --- a/drivers/vhost/net.c
> >>> +++ b/drivers/vhost/net.c
> >>> @@ -429,6 +429,52 @@ static int vhost_net_enable_vq(struct vhost_net *n,
> >>>        return vhost_poll_start(poll, sock->file);
> >>>    }
> >>>
> >>> +static int sk_has_rx_data(struct sock *sk)
> >>> +{
> >>> +     struct socket *sock = sk->sk_socket;
> >>> +
> >>> +     if (sock->ops->peek_len)
> >>> +             return sock->ops->peek_len(sock);
> >>> +
> >>> +     return skb_queue_empty(&sk->sk_receive_queue);
> >>> +}
> >>> +
> >>> +static void vhost_net_busy_poll(struct vhost_net *net,
> >>> +                             struct vhost_virtqueue *rvq,
> >>> +                             struct vhost_virtqueue *tvq,
> >>> +                             bool rx)
> >>> +{
> >>> +     unsigned long uninitialized_var(endtime);
> >>> +     unsigned long busyloop_timeout;
> >>> +     struct socket *sock;
> >>> +     struct vhost_virtqueue *vq = rx ? tvq : rvq;
> >>> +
> >>> +     mutex_lock_nested(&vq->mutex, rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
> >>> +
> >>> +     vhost_disable_notify(&net->dev, vq);
> >>> +     sock = rvq->private_data;
> >>> +     busyloop_timeout = rx ? rvq->busyloop_timeout : tvq->busyloop_timeout;
> >>> +
> >>> +     preempt_disable();
> >>> +     endtime = busy_clock() + busyloop_timeout;
> >>> +     while (vhost_can_busy_poll(tvq->dev, endtime) &&
> >>> +            !(sock && sk_has_rx_data(sock->sk)) &&
> >>> +            vhost_vq_avail_empty(tvq->dev, tvq))
> >>> +             cpu_relax();
> >>> +     preempt_enable();
> >>> +
> >>> +     if ((rx && !vhost_vq_avail_empty(&net->dev, vq)) ||
> >>> +         (!rx && (sock && sk_has_rx_data(sock->sk)))) {
> >>> +             vhost_poll_queue(&vq->poll);
> >>> +     } else if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> >> One last question, do we need this for rx? This check will be always
> >> true under light or medium load.
> > will remove the 'unlikely'
>
> Not only the unlikely. We only want rx kick when packet is pending at
> socket but we're out of available buffers. This is not the case here
> (you have polled the vq above).
>
> So we probably want to do this only when rx == true.
got it. change it to:
+       } else if (vhost_enable_notify(&net->dev, vq) && rx) {
+               vhost_disable_notify(&net->dev, vq);
+               vhost_poll_queue(&vq->poll);
+       }

> Thanks
>
> >
> >> Thanks
> >>
> >>> +             vhost_disable_notify(&net->dev, vq);
> >>> +             vhost_poll_queue(&vq->poll);
> >>> +     }
> >>> +
> >>> +     mutex_unlock(&vq->mutex);
> >>> +}
> >>> +
> >>> +
> >>>    static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
> >>>                                    struct vhost_virtqueue *vq,
> >>>                                    struct iovec iov[], unsigned int iov_size,
> >>> @@ -621,16 +667,6 @@ static int peek_head_len(struct vhost_net_virtqueue *rvq, struct sock *sk)
> >>>        return len;
> >>>    }
> >>>
> >>> -static int sk_has_rx_data(struct sock *sk)
> >>> -{
> >>> -     struct socket *sock = sk->sk_socket;
> >>> -
> >>> -     if (sock->ops->peek_len)
> >>> -             return sock->ops->peek_len(sock);
> >>> -
> >>> -     return skb_queue_empty(&sk->sk_receive_queue);
> >>> -}
> >>> -
> >>>    static void vhost_rx_signal_used(struct vhost_net_virtqueue *nvq)
> >>>    {
> >>>        struct vhost_virtqueue *vq = &nvq->vq;
> >>> @@ -645,39 +681,19 @@ static void vhost_rx_signal_used(struct vhost_net_virtqueue *nvq)
> >>>
> >>>    static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk)
> >>>    {
> >>> -     struct vhost_net_virtqueue *rvq = &net->vqs[VHOST_NET_VQ_RX];
> >>> -     struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
> >>> -     struct vhost_virtqueue *vq = &nvq->vq;
> >>> -     unsigned long uninitialized_var(endtime);
> >>> -     int len = peek_head_len(rvq, sk);
> >>> +     struct vhost_net_virtqueue *rnvq = &net->vqs[VHOST_NET_VQ_RX];
> >>> +     struct vhost_net_virtqueue *tnvq = &net->vqs[VHOST_NET_VQ_TX];
> >>>
> >>> -     if (!len && vq->busyloop_timeout) {
> >>> -             /* Flush batched heads first */
> >>> -             vhost_rx_signal_used(rvq);
> >>> -             /* Both tx vq and rx socket were polled here */
> >>> -             mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_TX);
> >>> -             vhost_disable_notify(&net->dev, vq);
> >>> +     int len = peek_head_len(rnvq, sk);
> >>>
> >>> -             preempt_disable();
> >>> -             endtime = busy_clock() + vq->busyloop_timeout;
> >>> -
> >>> -             while (vhost_can_busy_poll(&net->dev, endtime) &&
> >>> -                    !sk_has_rx_data(sk) &&
> >>> -                    vhost_vq_avail_empty(&net->dev, vq))
> >>> -                     cpu_relax();
> >>> -
> >>> -             preempt_enable();
> >>> -
> >>> -             if (!vhost_vq_avail_empty(&net->dev, vq))
> >>> -                     vhost_poll_queue(&vq->poll);
> >>> -             else if (unlikely(vhost_enable_notify(&net->dev, vq))) {
> >>> -                     vhost_disable_notify(&net->dev, vq);
> >>> -                     vhost_poll_queue(&vq->poll);
> >>> -             }
> >>> +     if (!len && rnvq->vq.busyloop_timeout) {
> >>> +             /* Flush batched heads first */
> >>> +             vhost_rx_signal_used(rnvq);
> >>>
> >>> -             mutex_unlock(&vq->mutex);
> >>> +             /* Both tx vq and rx socket were polled here */
> >>> +             vhost_net_busy_poll(net, &rnvq->vq, &tnvq->vq, true);
> >>>
> >>> -             len = peek_head_len(rvq, sk);
> >>> +             len = peek_head_len(rnvq, sk);
> >>>        }
> >>>
> >>>        return len;
>

^ permalink raw reply

* Re: [PATCH v2 net-next 4/4] vhost_net: Avoid rx vring kicks during busyloop
From: Toshiaki Makita @ 2018-07-04  1:21 UTC (permalink / raw)
  To: Jason Wang, David S. Miller, Michael S. Tsirkin
  Cc: netdev, kvm, virtualization
In-Reply-To: <abc4f8ce-35da-f628-811f-053250fa9b0e@redhat.com>

On 2018/07/03 18:05, Jason Wang wrote:
> On 2018年07月03日 15:31, Toshiaki Makita wrote:
>> We may run out of avail rx ring descriptor under heavy load but busypoll
>> did not detect it so busypoll may have exited prematurely. Avoid this by
>> checking rx ring full during busypoll.
> 
> Actually, we're checking whether it was empty in fact?

My understanding is that on rx empty avail means no free descriptors
from the perspective of host so the ring is conceptually full.

>> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> ---
>>   drivers/vhost/net.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 791bc8b..b224036 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -658,6 +658,7 @@ static int vhost_net_rx_peek_head_len(struct
>> vhost_net *net, struct sock *sk,
>>   {
>>       struct vhost_net_virtqueue *rnvq = &net->vqs[VHOST_NET_VQ_RX];
>>       struct vhost_net_virtqueue *tnvq = &net->vqs[VHOST_NET_VQ_TX];
>> +    struct vhost_virtqueue *rvq = &rnvq->vq;
>>       struct vhost_virtqueue *tvq = &tnvq->vq;
>>       unsigned long uninitialized_var(endtime);
>>       int len = peek_head_len(rnvq, sk);
>> @@ -677,7 +678,8 @@ static int vhost_net_rx_peek_head_len(struct
>> vhost_net *net, struct sock *sk,
>>                   *busyloop_intr = true;
>>                   break;
>>               }
>> -            if (sk_has_rx_data(sk) ||
>> +            if ((sk_has_rx_data(sk) &&
>> +                 !vhost_vq_avail_empty(&net->dev, rvq)) ||
>>                   !vhost_vq_avail_empty(&net->dev, tvq))
>>                   break;
>>               cpu_relax();
>> @@ -827,7 +829,6 @@ static void handle_rx(struct vhost_net *net)
>>   
> 
> I thought below codes should belong to patch 3. Or I may miss something.

That codes are added for the case that busypoll is waiting for rx vq
avail but interrupted by another work. At the point of patch 3 busypoll
does not wait for rx vq avail so I don't think we move them to patch 3.

> 
> Thanks
> 
>>       while ((sock_len = vhost_net_rx_peek_head_len(net, sock->sk,
>>                                 &busyloop_intr))) {
>> -        busyloop_intr = false;
>>           sock_len += sock_hlen;
>>           vhost_len = sock_len + vhost_hlen;
>>           headcount = get_rx_bufs(vq, vq->heads + nvq->done_idx,
>> @@ -838,7 +839,9 @@ static void handle_rx(struct vhost_net *net)
>>               goto out;
>>           /* OK, now we need to know about added descriptors. */
>>           if (!headcount) {
>> -            if (unlikely(vhost_enable_notify(&net->dev, vq))) {
>> +            if (unlikely(busyloop_intr)) {
>> +                vhost_poll_queue(&vq->poll);
>> +            } else if (unlikely(vhost_enable_notify(&net->dev, vq))) {
>>                   /* They have slipped one in as we were
>>                    * doing that: check again. */
>>                   vhost_disable_notify(&net->dev, vq);
>> @@ -848,6 +851,7 @@ static void handle_rx(struct vhost_net *net)
>>                * they refilled. */
>>               goto out;
>>           }
>> +        busyloop_intr = false;
>>           if (nvq->rx_ring)
>>               msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
>>           /* On overrun, truncate and discard */

-- 
Toshiaki Makita

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* Re: [PATCH net-next 1/8] vhost: move get_rx_bufs to vhost.c
From: kbuild test robot @ 2018-07-04  1:01 UTC (permalink / raw)
  To: Jason Wang
  Cc: kvm, mst, netdev, linux-kernel, virtualization, maxime.coquelin,
	kbuild-all, wexu
In-Reply-To: <1530596284-4101-2-git-send-email-jasowang@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 12105 bytes --]

Hi Jason,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Jason-Wang/Packed-virtqueue-for-vhost/20180703-154751
config: x86_64-randconfig-s0-07032254 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: the linux-review/Jason-Wang/Packed-virtqueue-for-vhost/20180703-154751 HEAD 01b902f1126212ea2597e6d09802bd9c4431bf82 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   drivers//vhost/net.c: In function 'handle_rx':
>> drivers//vhost/net.c:738:15: error: implicit declaration of function 'get_rx_bufs' [-Werror=implicit-function-declaration]
      headcount = get_rx_bufs(vq, vq->heads + nvq->done_idx,
                  ^~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/get_rx_bufs +738 drivers//vhost/net.c

030881372 Jason Wang         2016-03-04  687  
3a4d5c94e Michael S. Tsirkin 2010-01-14  688  /* Expects to be always run from workqueue - which acts as
3a4d5c94e Michael S. Tsirkin 2010-01-14  689   * read-size critical section for our kind of RCU. */
94249369e Jason Wang         2011-01-17  690  static void handle_rx(struct vhost_net *net)
3a4d5c94e Michael S. Tsirkin 2010-01-14  691  {
81f95a558 Michael S. Tsirkin 2013-04-28  692  	struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_RX];
81f95a558 Michael S. Tsirkin 2013-04-28  693  	struct vhost_virtqueue *vq = &nvq->vq;
8dd014adf David Stevens      2010-07-27  694  	unsigned uninitialized_var(in), log;
8dd014adf David Stevens      2010-07-27  695  	struct vhost_log *vq_log;
8dd014adf David Stevens      2010-07-27  696  	struct msghdr msg = {
8dd014adf David Stevens      2010-07-27  697  		.msg_name = NULL,
8dd014adf David Stevens      2010-07-27  698  		.msg_namelen = 0,
8dd014adf David Stevens      2010-07-27  699  		.msg_control = NULL, /* FIXME: get and handle RX aux data. */
8dd014adf David Stevens      2010-07-27  700  		.msg_controllen = 0,
8dd014adf David Stevens      2010-07-27  701  		.msg_flags = MSG_DONTWAIT,
8dd014adf David Stevens      2010-07-27  702  	};
0960b6417 Jason Wang         2015-02-15  703  	struct virtio_net_hdr hdr = {
0960b6417 Jason Wang         2015-02-15  704  		.flags = 0,
0960b6417 Jason Wang         2015-02-15  705  		.gso_type = VIRTIO_NET_HDR_GSO_NONE
8dd014adf David Stevens      2010-07-27  706  	};
8dd014adf David Stevens      2010-07-27  707  	size_t total_len = 0;
910a578f7 Michael S. Tsirkin 2012-10-24  708  	int err, mergeable;
f5a4941aa Jason Wang         2018-05-29  709  	s16 headcount;
8dd014adf David Stevens      2010-07-27  710  	size_t vhost_hlen, sock_hlen;
8dd014adf David Stevens      2010-07-27  711  	size_t vhost_len, sock_len;
2e26af79b Asias He           2013-05-07  712  	struct socket *sock;
ba7438aed Al Viro            2014-12-10  713  	struct iov_iter fixup;
0960b6417 Jason Wang         2015-02-15  714  	__virtio16 num_buffers;
db688c24e Paolo Abeni        2018-04-24  715  	int recv_pkts = 0;
8dd014adf David Stevens      2010-07-27  716  
aaa3149bb Jason Wang         2018-03-26  717  	mutex_lock_nested(&vq->mutex, 0);
2e26af79b Asias He           2013-05-07  718  	sock = vq->private_data;
2e26af79b Asias He           2013-05-07  719  	if (!sock)
2e26af79b Asias He           2013-05-07  720  		goto out;
6b1e6cc78 Jason Wang         2016-06-23  721  
6b1e6cc78 Jason Wang         2016-06-23  722  	if (!vq_iotlb_prefetch(vq))
6b1e6cc78 Jason Wang         2016-06-23  723  		goto out;
6b1e6cc78 Jason Wang         2016-06-23  724  
8ea8cf89e Michael S. Tsirkin 2011-05-20  725  	vhost_disable_notify(&net->dev, vq);
8241a1e46 Jason Wang         2016-06-01  726  	vhost_net_disable_vq(net, vq);
2e26af79b Asias He           2013-05-07  727  
81f95a558 Michael S. Tsirkin 2013-04-28  728  	vhost_hlen = nvq->vhost_hlen;
81f95a558 Michael S. Tsirkin 2013-04-28  729  	sock_hlen = nvq->sock_hlen;
8dd014adf David Stevens      2010-07-27  730  
ea16c5143 Michael S. Tsirkin 2014-06-05  731  	vq_log = unlikely(vhost_has_feature(vq, VHOST_F_LOG_ALL)) ?
8dd014adf David Stevens      2010-07-27  732  		vq->log : NULL;
ea16c5143 Michael S. Tsirkin 2014-06-05  733  	mergeable = vhost_has_feature(vq, VIRTIO_NET_F_MRG_RXBUF);
8dd014adf David Stevens      2010-07-27  734  
030881372 Jason Wang         2016-03-04  735  	while ((sock_len = vhost_net_rx_peek_head_len(net, sock->sk))) {
8dd014adf David Stevens      2010-07-27  736  		sock_len += sock_hlen;
8dd014adf David Stevens      2010-07-27  737  		vhost_len = sock_len + vhost_hlen;
f5a4941aa Jason Wang         2018-05-29 @738  		headcount = get_rx_bufs(vq, vq->heads + nvq->done_idx,
f5a4941aa Jason Wang         2018-05-29  739  					vhost_len, &in, vq_log, &log,
94249369e Jason Wang         2011-01-17  740  					likely(mergeable) ? UIO_MAXIOV : 1);
8dd014adf David Stevens      2010-07-27  741  		/* On error, stop handling until the next kick. */
8dd014adf David Stevens      2010-07-27  742  		if (unlikely(headcount < 0))
8241a1e46 Jason Wang         2016-06-01  743  			goto out;
8dd014adf David Stevens      2010-07-27  744  		/* OK, now we need to know about added descriptors. */
8dd014adf David Stevens      2010-07-27  745  		if (!headcount) {
8ea8cf89e Michael S. Tsirkin 2011-05-20  746  			if (unlikely(vhost_enable_notify(&net->dev, vq))) {
8dd014adf David Stevens      2010-07-27  747  				/* They have slipped one in as we were
8dd014adf David Stevens      2010-07-27  748  				 * doing that: check again. */
8ea8cf89e Michael S. Tsirkin 2011-05-20  749  				vhost_disable_notify(&net->dev, vq);
8dd014adf David Stevens      2010-07-27  750  				continue;
8dd014adf David Stevens      2010-07-27  751  			}
8dd014adf David Stevens      2010-07-27  752  			/* Nothing new?  Wait for eventfd to tell us
8dd014adf David Stevens      2010-07-27  753  			 * they refilled. */
8241a1e46 Jason Wang         2016-06-01  754  			goto out;
8dd014adf David Stevens      2010-07-27  755  		}
5990a3051 Jason Wang         2018-01-04  756  		if (nvq->rx_ring)
6e474083f Wei Xu             2017-12-01  757  			msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
6e474083f Wei Xu             2017-12-01  758  		/* On overrun, truncate and discard */
6e474083f Wei Xu             2017-12-01  759  		if (unlikely(headcount > UIO_MAXIOV)) {
6e474083f Wei Xu             2017-12-01  760  			iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
6e474083f Wei Xu             2017-12-01  761  			err = sock->ops->recvmsg(sock, &msg,
6e474083f Wei Xu             2017-12-01  762  						 1, MSG_DONTWAIT | MSG_TRUNC);
6e474083f Wei Xu             2017-12-01  763  			pr_debug("Discarded rx packet: len %zd\n", sock_len);
6e474083f Wei Xu             2017-12-01  764  			continue;
6e474083f Wei Xu             2017-12-01  765  		}
8dd014adf David Stevens      2010-07-27  766  		/* We don't need to be notified again. */
ba7438aed Al Viro            2014-12-10  767  		iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
ba7438aed Al Viro            2014-12-10  768  		fixup = msg.msg_iter;
ba7438aed Al Viro            2014-12-10  769  		if (unlikely((vhost_hlen))) {
ba7438aed Al Viro            2014-12-10  770  			/* We will supply the header ourselves
ba7438aed Al Viro            2014-12-10  771  			 * TODO: support TSO.
ba7438aed Al Viro            2014-12-10  772  			 */
ba7438aed Al Viro            2014-12-10  773  			iov_iter_advance(&msg.msg_iter, vhost_hlen);
ba7438aed Al Viro            2014-12-10  774  		}
1b7841404 Ying Xue           2015-03-02  775  		err = sock->ops->recvmsg(sock, &msg,
8dd014adf David Stevens      2010-07-27  776  					 sock_len, MSG_DONTWAIT | MSG_TRUNC);
8dd014adf David Stevens      2010-07-27  777  		/* Userspace might have consumed the packet meanwhile:
8dd014adf David Stevens      2010-07-27  778  		 * it's not supposed to do this usually, but might be hard
8dd014adf David Stevens      2010-07-27  779  		 * to prevent. Discard data we got (if any) and keep going. */
8dd014adf David Stevens      2010-07-27  780  		if (unlikely(err != sock_len)) {
8dd014adf David Stevens      2010-07-27  781  			pr_debug("Discarded rx packet: "
8dd014adf David Stevens      2010-07-27  782  				 " len %d, expected %zd\n", err, sock_len);
8dd014adf David Stevens      2010-07-27  783  			vhost_discard_vq_desc(vq, headcount);
8dd014adf David Stevens      2010-07-27  784  			continue;
8dd014adf David Stevens      2010-07-27  785  		}
ba7438aed Al Viro            2014-12-10  786  		/* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
4c5a84421 Michael S. Tsirkin 2015-02-25  787  		if (unlikely(vhost_hlen)) {
4c5a84421 Michael S. Tsirkin 2015-02-25  788  			if (copy_to_iter(&hdr, sizeof(hdr),
4c5a84421 Michael S. Tsirkin 2015-02-25  789  					 &fixup) != sizeof(hdr)) {
4c5a84421 Michael S. Tsirkin 2015-02-25  790  				vq_err(vq, "Unable to write vnet_hdr "
4c5a84421 Michael S. Tsirkin 2015-02-25  791  				       "at addr %p\n", vq->iov->iov_base);
8241a1e46 Jason Wang         2016-06-01  792  				goto out;
8dd014adf David Stevens      2010-07-27  793  			}
4c5a84421 Michael S. Tsirkin 2015-02-25  794  		} else {
4c5a84421 Michael S. Tsirkin 2015-02-25  795  			/* Header came from socket; we'll need to patch
4c5a84421 Michael S. Tsirkin 2015-02-25  796  			 * ->num_buffers over if VIRTIO_NET_F_MRG_RXBUF
4c5a84421 Michael S. Tsirkin 2015-02-25  797  			 */
4c5a84421 Michael S. Tsirkin 2015-02-25  798  			iov_iter_advance(&fixup, sizeof(hdr));
4c5a84421 Michael S. Tsirkin 2015-02-25  799  		}
8dd014adf David Stevens      2010-07-27  800  		/* TODO: Should check and handle checksum. */
5201aa49b Michael S. Tsirkin 2015-02-03  801  
0960b6417 Jason Wang         2015-02-15  802  		num_buffers = cpu_to_vhost16(vq, headcount);
cfbdab951 Jason Wang         2011-01-17  803  		if (likely(mergeable) &&
0d79a493e Michael S. Tsirkin 2015-02-25  804  		    copy_to_iter(&num_buffers, sizeof num_buffers,
0d79a493e Michael S. Tsirkin 2015-02-25  805  				 &fixup) != sizeof num_buffers) {
8dd014adf David Stevens      2010-07-27  806  			vq_err(vq, "Failed num_buffers write");
8dd014adf David Stevens      2010-07-27  807  			vhost_discard_vq_desc(vq, headcount);
8241a1e46 Jason Wang         2016-06-01  808  			goto out;
8dd014adf David Stevens      2010-07-27  809  		}
f5a4941aa Jason Wang         2018-05-29  810  		nvq->done_idx += headcount;
f5a4941aa Jason Wang         2018-05-29  811  		if (nvq->done_idx > VHOST_RX_BATCH)
f5a4941aa Jason Wang         2018-05-29  812  			vhost_rx_signal_used(nvq);
8dd014adf David Stevens      2010-07-27  813  		if (unlikely(vq_log))
8dd014adf David Stevens      2010-07-27  814  			vhost_log_write(vq, vq_log, log, vhost_len);
8dd014adf David Stevens      2010-07-27  815  		total_len += vhost_len;
db688c24e Paolo Abeni        2018-04-24  816  		if (unlikely(total_len >= VHOST_NET_WEIGHT) ||
db688c24e Paolo Abeni        2018-04-24  817  		    unlikely(++recv_pkts >= VHOST_NET_PKT_WEIGHT)) {
8dd014adf David Stevens      2010-07-27  818  			vhost_poll_queue(&vq->poll);
8241a1e46 Jason Wang         2016-06-01  819  			goto out;
8dd014adf David Stevens      2010-07-27  820  		}
8dd014adf David Stevens      2010-07-27  821  	}
8241a1e46 Jason Wang         2016-06-01  822  	vhost_net_enable_vq(net, vq);
2e26af79b Asias He           2013-05-07  823  out:
f5a4941aa Jason Wang         2018-05-29  824  	vhost_rx_signal_used(nvq);
8dd014adf David Stevens      2010-07-27  825  	mutex_unlock(&vq->mutex);
8dd014adf David Stevens      2010-07-27  826  }
8dd014adf David Stevens      2010-07-27  827  

:::::: The code at line 738 was first introduced by commit
:::::: f5a4941aa6d190e676065e8f4ed35999f52a01c3 vhost_net: flush batched heads before trying to busy polling

:::::: TO: Jason Wang <jasowang@redhat.com>
:::::: CC: David S. Miller <davem@davemloft.net>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34871 bytes --]

[-- Attachment #3: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* Re: [RFC bpf-next 2/6] net: xdp: RX meta data infrastructure
From: Saeed Mahameed @ 2018-07-04  0:57 UTC (permalink / raw)
  To: alexei.starovoitov@gmail.com, saeedm@dev.mellanox.co.il
  Cc: alexander.h.duyck@intel.com, netdev@vger.kernel.org, Tariq Toukan,
	john.fastabend@gmail.com, brouer@redhat.com,
	borkmann@iogearbox.net, peter.waskiewicz.jr@intel.com
In-Reply-To: <20180703230137.hdoy2fujz3x4oeij@ast-mbp.dhcp.thefacebook.com>

On Tue, 2018-07-03 at 16:01 -0700, Alexei Starovoitov wrote:
> On Tue, Jun 26, 2018 at 07:46:11PM -0700, Saeed Mahameed wrote:
> > The idea from this patch is to define a well known structure for
> > XDP meta
> > data fields format and offset placement inside the xdp data meta
> > buffer.
> > 
> > For that driver will need some static information to know what to
> > provide and where, enters struct xdp_md_info and xdp_md_info_arr:
> > 
> > struct xdp_md_info {
> >        __u16 present:1;
> >        __u16 offset:15; /* offset from data_meta in xdp_md buffer
> > */
> > };
> > 
> > /* XDP meta data offsets info array
> >  * present bit describes if a meta data is or will be present in
> > xdp_md buff
> >  * offset describes where a meta data is or should be placed in
> > xdp_md buff
> >  *
> >  * Kernel builds this array using xdp_md_info_build helper on
> > demand.
> >  * User space builds it statically in the xdp program.
> >  */
> > typedef struct xdp_md_info xdp_md_info_arr[XDP_DATA_META_MAX];
> > 
> > Offsets in xdp_md_info_arr are always in ascending order and only
> > for
> > requested meta data:
> > example : flags = XDP_FLAGS_META_HASH | XDP_FLAGS_META_VLAN;
> > 
> > xdp_md_info_arr mdi = {
> > 	[XDP_DATA_META_HASH] = {.offset = 0, .present = 1},
> > 	[XDP_DATA_META_MARK] = {.offset = 0, .present = 0},
> > 	[XDP_DATA_META_VLAN] = {.offset = sizeof(struct xdp_md_hash),
> > .present = 1},
> > 	[XDP_DATA_META_CSUM] = {.offset = 0, .present = 0},
> > }
> > 
> > i.e: hash fields will always appear first then the vlan for every
> > xdp_md.
> > 
> > Once requested to provide xdp meta data, device driver will use a
> > pre-built
> > xdp_md_info_arr which was built via xdp_md_info_build on xdp setup,
> > xdp_md_info_arr will tell the driver what is the offset of each
> > meta data.
> > 
> > This patch also provides helper functions for the device drivers to
> > store
> > meta data into xdp_buff, and helper function for XDP programs to
> > fetch
> > specific xdp meta data from xdp_md buffer.
> > 
> > Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> 
> ...
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 59b19b6a40d7..e320e7421565 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2353,6 +2353,120 @@ struct xdp_md {
> >  	__u32 rx_queue_index;  /* rxq->queue_index  */
> >  };
> >  
> > +enum {
> > +	XDP_DATA_META_HASH = 0,
> > +	XDP_DATA_META_MARK,
> > +	XDP_DATA_META_VLAN,
> > +	XDP_DATA_META_CSUM_COMPLETE,
> > +
> > +	XDP_DATA_META_MAX,
> > +};
> > +
> > +struct xdp_md_hash {
> > +	__u32 hash;
> > +	__u8  type;
> > +} __attribute__((aligned(4)));
> > +
> > +struct xdp_md_mark {
> > +	__u32 mark;
> > +} __attribute__((aligned(4)));
> > +
> > +struct xdp_md_vlan {
> > +	__u16 vlan;
> > +} __attribute__((aligned(4)));
> > +
> > +struct xdp_md_csum {
> > +	__u16 csum;
> > +} __attribute__((aligned(4)));
> > +
> > +static const __u16 xdp_md_size[XDP_DATA_META_MAX] = {
> > +	sizeof(struct xdp_md_hash), /* XDP_DATA_META_HASH */
> > +	sizeof(struct xdp_md_mark), /* XDP_DATA_META_MARK */
> > +	sizeof(struct xdp_md_vlan), /* XDP_DATA_META_VLAN */
> > +	sizeof(struct xdp_md_csum), /* XDP_DATA_META_CSUM_COMPLETE
> > */
> > +};
> > +
> > +struct xdp_md_info {
> > +	__u16 present:1;
> > +	__u16 offset:15; /* offset from data_meta in xdp_md buffer
> > */
> > +};
> > +
> > +/* XDP meta data offsets info array
> > + * present bit describes if a meta data is or will be present in
> > xdp_md buff
> > + * offset describes where a meta data is or should be placed in
> > xdp_md buff
> > + *
> > + * Kernel builds this array using xdp_md_info_build helper on
> > demand.
> > + * User space builds it statically in the xdp program.
> > + */
> > +typedef struct xdp_md_info xdp_md_info_arr[XDP_DATA_META_MAX];
> > +
> > +static __always_inline __u8
> > +xdp_data_meta_present(xdp_md_info_arr mdi, int meta_data)
> > +{
> > +	return mdi[meta_data].present;
> > +}
> > +
> > +static __always_inline void
> > +xdp_data_meta_set_hash(xdp_md_info_arr mdi, void *data_meta, __u32
> > hash, __u8 htype)
> > +{
> > +	struct xdp_md_hash *hash_md;
> > +
> > +	hash_md = (struct xdp_md_hash *)((char*)data_meta +
> > mdi[XDP_DATA_META_HASH].offset);
> > +	hash_md->hash = hash;
> > +	hash_md->type = htype;
> > +}
> > +
> > +static __always_inline struct xdp_md_hash *
> > +xdp_data_meta_get_hash(xdp_md_info_arr mdi, void *data_meta)
> > +{
> > +	return (struct xdp_md_hash *)((char*)data_meta +
> > mdi[XDP_DATA_META_HASH].offset);
> > +}
> 
> I'm afraid this is not scalable uapi.
> This looks very much mlx specific. Every NIC vendor cannot add its
> own
> struct definitions into uapi/bpf.h. It doesn't scale.

No, this is not mlx specific, all you can find here is standard
hash/csum/vlan/flow mark, fields as described in SKB, we want them to
be well defined so you can construct SKBs with these standard values
from xdp_md with no surprises.

I agree this is not scalable, but this patch is not about arbitrary
vendor specific hints, with unknown sizes and shapes, i understand the
demand but we do need the notion of standard hints as well in order to
allow (driver/fw/hw)=>(xdp program)=>(stack) data flow, since the three
of those entities must agree on some well defined hints.

Anyway we discussed this in the last iovisor meeting and we agreed that
along with the standard hints suggested by this patch we want to allow
arbitrary hw specific hints, the question is how to have both.

> Also doing 15 bit bitfield extraction using bpf instructions is not
> that simple.
> mlx should consider doing plain u8 or u16 fields in firmware instead.
> The metadata that "hw" provides is a joint work of actual asic and
> firmware.

Right, this would be a huge flexibility improvement for future hw.
what about current HW, the driver can always translate the current
format to whatever format we decide on.

> I suspect the format can and will change with different firmware.
> Baking this stuff into uapi/bpf.h is not an option.

Yes but we still need to have a well defined structures for standard
hints, for the hints to flow into the stack as well. current and future
HW/FW/driver must respect the well known format of specific hints, such
as hash/csum/vlan, etc..

> How about we make driver+firmware provide a BTF definition of
> metadata that they
> can provide? There can be multiple definitions of such structs.
> Then in userpsace we can have BTF->plain C converter.
> (bpftool practically ready to do that already).
> Then the programmer can take such generated C definition, add it to
> .h and include
> it in their programs. llvm will compile the whole thing and will
> include BTF
> of maps, progs and this md struct in the target elf file.
> During loading the kernel can check that BTF in elf is matching one-
> to-one
> to what driver+firmware are saying they support.

Just thinking out loud, can't we do this at program load ? just run a
setup function in the xdp program to load nic md BTF definition into
the elf section ?

> No ambiguity and no possibility of mistake, since offsets and field
> names
> are verified.

But what about the dynamic nature of this feature ? Sometimes you only
want HW/Driver to provide a subset of whatever the HW can provide and
save md buffer for other stuff.

Yes a well defined format is favorable here, but we need to make sure
there is no computational overhead in data path just to extract each
field! for example if i want to know what is the offset of the hash
will i need to go parse (for every packet) the whole BTF definition of
metadata just to find the offset of type=hash ?

> Every driver can have their own BTF for md and their own special
> features.
> We can try to standardize the names (like vlan and csum), so xdp
> programs
> can stay relatively portable across NICs.

Yes this is a must.

> Such api will address exposing asic+firmware metadata to the xdp
> program.
> Once we tackle this problem, we'll think how to do the backward
> config
> (to do firmware reconfig for specific BTF definition of md supplied
> by the prog).
> What people think?
> 

For legacy HW, we can do it already in the driver, provide whatever the
prog requested, its only a matter of translation to the BTF format in
the driver xdp setup and pushing the values accordingly into the md
offsets on data path.

Question: how can you share the md BTF from the driver/HW with the xdp
program ?

^ permalink raw reply

* [net-next,v2] tcp: Improve setsockopt() TCP_USER_TIMEOUT accuracy
From: Jon Maxwell @ 2018-07-04  0:06 UTC (permalink / raw)
  To: davem; +Cc: ncardwell, edumazet, kuznet, yoshfuji, netdev, linux-kernel,
	jmaxwell

v2 contains the following suggestions by Neal Cardwell:

1) Fix up units mismatch regarding msec/jiffies.
2) Address possiblility of time_remaining being negative.
3) Add a helper routine tcp_clamp_rto_to_user_timeout() to do the rto 
calculation.
4) Move start_ts logic into helper routine tcp_retrans_stamp() to 
validate tcp_sk(sk)->retrans_stamp.

Every time the TCP retransmission timer fires. It checks to see if there is a 
timeout before scheduling the next retransmit timer. The retransmit interval 
between each retransmission increases exponentially. The issue is that in order 
for the timeout to occur the retransmit timer needs to fire again. If the user 
timeout check happens after the 9th retransmit for example. It needs to wait for 
the 10th retransmit timer to fire in order to evaluate whether a timeout has 
occurred or not. If the interval is large enough then the timeout will be 
inaccurate.

For example with a TCP_USER_TIMEOUT of 10 seconds without patch:

1st retransmit:

22:25:18.973488 IP host1.49310 > host2.search-agent: Flags [.]

Last retransmit:

22:25:26.205499 IP host1.49310 > host2.search-agent: Flags [.]

Timeout:

send: Connection timed out
Sun Jul  1 22:25:34 EDT 2018

We can see that last retransmit took ~7 seconds. Which pushed the total 
timeout to ~15 seconds instead of the expected 10 seconds. This gets more 
inaccurate the larger the TCP_USER_TIMEOUT value. As the interval increases.

Add tcp_clamp_rto_to_user_timeout() to determine if the user rto has expired.
Or whether the rto interval needs to be recalculated. Use the original interval
if user rto is not set. 

Test results with the patch is the expected 10 second timeout:

1st retransmit:

01:37:59.022555 IP host1.49310 > host2.search-agent: Flags [.]

Last retransmit:

01:38:06.486558 IP host1.49310 > host2.search-agent: Flags [.]

Timeout:

send: Connection timed out
Mon Jul  2 01:38:09 EDT 2018

Signed-off-by: Jon Maxwell <jmaxwell37@gmail.com>
---
 net/ipv4/tcp_timer.c | 48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index 3b3611729928..d129e670d02a 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -22,6 +22,39 @@
 #include <linux/gfp.h>
 #include <net/tcp.h>
 
+unsigned int tcp_retransmit_stamp(struct sock *sk)
+{
+	unsigned int start_ts = tcp_sk(sk)->retrans_stamp;
+
+	if (unlikely(!start_ts)) {
+		struct sk_buff *head = tcp_rtx_queue_head(sk);
+
+		if (!head)
+			return false;
+		start_ts = tcp_skb_timestamp(head);
+	}
+	return start_ts;
+}
+
+static __u32 tcp_clamp_rto_to_user_timeout(struct sock *sk)
+{
+	struct inet_connection_sock *icsk = inet_csk(sk);
+	__u32 rto = icsk->icsk_rto;
+	__u32 elapsed, user_timeout;
+	unsigned int start_ts;
+
+	start_ts = tcp_retransmit_stamp(sk);
+	if (!icsk->icsk_user_timeout || !start_ts)
+		return rto;
+	elapsed = tcp_time_stamp(tcp_sk(sk)) - start_ts;
+	user_timeout = jiffies_to_msecs(icsk->icsk_user_timeout);
+	if (elapsed >= user_timeout)
+		rto = 1;  /* user timeout has passed; fire ASAP */
+	else
+		rto = min(rto, (__u32)msecs_to_jiffies(user_timeout - elapsed));
+	return rto;
+}
+
 /**
  *  tcp_write_err() - close socket and save error info
  *  @sk:  The socket the error has appeared on.
@@ -166,14 +199,9 @@ static bool retransmits_timed_out(struct sock *sk,
 	if (!inet_csk(sk)->icsk_retransmits)
 		return false;
 
-	start_ts = tcp_sk(sk)->retrans_stamp;
-	if (unlikely(!start_ts)) {
-		struct sk_buff *head = tcp_rtx_queue_head(sk);
-
-		if (!head)
-			return false;
-		start_ts = tcp_skb_timestamp(head);
-	}
+	start_ts = tcp_retransmit_stamp(sk);
+	if (!start_ts)
+		return false;
 
 	if (likely(timeout == 0)) {
 		linear_backoff_thresh = ilog2(TCP_RTO_MAX/rto_base);
@@ -407,6 +435,7 @@ void tcp_retransmit_timer(struct sock *sk)
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct net *net = sock_net(sk);
 	struct inet_connection_sock *icsk = inet_csk(sk);
+	__u32 rto;
 
 	if (tp->fastopen_rsk) {
 		WARN_ON_ONCE(sk->sk_state != TCP_SYN_RECV &&
@@ -535,7 +564,8 @@ void tcp_retransmit_timer(struct sock *sk)
 		/* Use normal (exponential) backoff */
 		icsk->icsk_rto = min(icsk->icsk_rto << 1, TCP_RTO_MAX);
 	}
-	inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS, icsk->icsk_rto, TCP_RTO_MAX);
+	rto = tcp_clamp_rto_to_user_timeout(sk);
+	inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS, rto, TCP_RTO_MAX);
 	if (retransmits_timed_out(sk, net->ipv4.sysctl_tcp_retries1 + 1, 0))
 		__sk_dst_reset(sk);
 
-- 
2.13.6

^ permalink raw reply related

* Re: pull-request: bpf-next 2018-07-03
From: David Miller @ 2018-07-04  0:00 UTC (permalink / raw)
  To: daniel; +Cc: ast, netdev
In-Reply-To: <20180703211813.5645-1-daniel@iogearbox.net>

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Tue,  3 Jul 2018 23:18:13 +0200

> The following pull-request contains BPF updates for your *net-next* tree.
> 
> The main changes are:
 ...
> Please consider pulling these changes from:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git

Pulled, thanks Daniel.

^ permalink raw reply

* Re: [RFC bpf-next 2/6] net: xdp: RX meta data infrastructure
From: Saeed Mahameed @ 2018-07-03 23:52 UTC (permalink / raw)
  To: daniel@iogearbox.net, saeedm@dev.mellanox.co.il,
	brouer@redhat.com
  Cc: alexander.h.duyck@intel.com, peter.waskiewicz.jr@intel.com,
	Rony Efraim, Tariq Toukan, john.fastabend@gmail.com,
	neerav.parikh@intel.com, Opher Reviv,
	alexei.starovoitov@gmail.com, pjwaskiewicz@gmail.com,
	netdev@vger.kernel.org, ttoukan.linux@gmail.com
In-Reply-To: <1fc9e26e-35b9-b14c-eac2-c240649e1417@iogearbox.net>

On Mon, 2018-07-02 at 10:01 +0200, Daniel Borkmann wrote:
> On 06/27/2018 07:55 PM, Saeed Mahameed wrote:
> > On Wed, 2018-06-27 at 16:15 +0200, Jesper Dangaard Brouer wrote:
> > > On Tue, 26 Jun 2018 19:46:11 -0700
> > > Saeed Mahameed <saeedm@dev.mellanox.co.il> wrote:
> > > 
> > > > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > > > index 2deea7166a34..afe302613ae1 100644
> > > > --- a/include/net/xdp.h
> > > > +++ b/include/net/xdp.h
> > > > @@ -138,6 +138,12 @@ xdp_set_data_meta_invalid(struct xdp_buff
> > > > *xdp)
> > > >  	xdp->data_meta = xdp->data + 1;
> > > >  }
> > > >  
> > > > +static __always_inline void
> > > > +xdp_reset_data_meta(struct xdp_buff *xdp)
> > > > +{
> > > > +	xdp->data_meta = xdp->data_hard_start;
> > > > +}
> > > 
> > > This is WRONG ... it should be:
> > > 
> > >  xdp->data_meta = xdp->data;
> > 
> > maybe the name of the function is not suitable for the use case.
> > i need to set xdp->data_meta = xdp->data in the driver to start
> > storing
> > meta data.
> 
> The xdp_set_data_meta_invalid() is a straight forward way for XDP
> drivers
> to tell they do not support xdp->data_meta, since setting xdp->data +
> 1 will
> fail the checks for direct (meta) packet access in the BPF code and
> at the
> same time bpf_xdp_adjust_meta() will know to bail out with error when
> program
> attempts to make headroom for meta data that driver cannot handle
> later on.
> 
> So later setting 'xdp->data_meta = xdp->data' to enable it is as
> setting any
> other of the initializers in xdp_buff, and done so today in the i40e,
> ixgbe,
> ixgbevf and nfp drivers. (Theoretically it wouldn't have to be
> exactly set to
> xdp->data, but anything <= xdp->data works, if the driver would
> prepend info
> from hw in front of it that program can then use or later override.)
> 

Right ! As jesper pointer out as well, 
driver should set: xdp->data_meta = xdp->data
and then adjust it to the offset of the first meta data hint.


^ permalink raw reply

* Re: [PATCH bpf-next v2 2/3] bpf: btf: add btf print functionality
From: Martin KaFai Lau @ 2018-07-03 23:33 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov, Yonghong Song,
	Quentin Monnet, David S. Miller, netdev, kernel-team,
	linux-kernel
In-Reply-To: <20180703153843.7a3425af@cakuba.netronome.com>

On Tue, Jul 03, 2018 at 03:38:43PM -0700, Jakub Kicinski wrote:
> On Tue, 3 Jul 2018 15:23:31 -0700, Jakub Kicinski wrote:
> > > > > +			else
> > > > > +				jsonw_printf(jw, "%hhd", *((char *)data));    
> > > > 
> > > > ... I think you need to always print a string, and express it as
> > > > \u00%02hhx for non-printable.    
> > > Okay that makes sense  
> > 
> > Yeah, IDK, char can be used as a byte as well as a string.  In eBPF
> > it may actually be more likely to just be used as a raw byte buffer...
> 
> Actually, what is the definition/purpose of BTF_INT_CHAR?  There seems
> to be no BTF_INT_SHORT and BTF_INT_SIGNED can simply be of size 8...
> Is normal int only used for bitfields of size 8 and BTF_INT_CHAR for
> char variables?
> 
> The kernel seems to be rejecting combinations of those flags, is
> unsigned char going to not be marked as char then?
BTF_INT_ENOCODING (CHAR/SIGNED/BOOL) is for formatting (e.g. pretty
print).  It is mainly how CTF is using it also.  Hence, BTF_INT_ENCODINGs
is not a 1:1 mapping to C integer types.
The size of an interger is described by BTF_INT_BITS instead.  

> 
> > Either way I think it may be nice to keep it consistent, at least for
> > the JSON output could we do either always ints or always characters?
> 

^ permalink raw reply

* QDisc Implementation: Setting bit rate and getting RTT
From: Taran Lynn @ 2018-07-03 23:13 UTC (permalink / raw)
  To: netdev@vger.kernel.org

Hello,
I'm new to linux development and am working on creating a qdisc module,
similar to those under net/sched/sch_*.c. Currently I'm stuck on two
things.

1. What's the best way to set the maximum bit rate?
2. How do I determine the RTT for packets?

For (1) I'm currently tracking the number of bits sent, and only sending
packets if they stay within the appropriate rate bound. Is there a better
way to do this? I've looked at net/sched/sch_netem.c, and as far as I can
tell it determines the delay between packets for a given rate and sends
them after that delay has passed. However, when I tried to do this I got
inconsistent rates, so I think I may be missing something.

As for (2) I'm not sure where to start.

Thanks in advance for any advise.

^ permalink raw reply

* Re: [net-next,v1] tcp: Improve setsockopt() TCP_USER_TIMEOUT accuracy
From: Jonathan Maxwell @ 2018-07-03 23:03 UTC (permalink / raw)
  To: Neal Cardwell
  Cc: David Miller, Eric Dumazet, Alexey Kuznetsov, Hideaki YOSHIFUJI,
	Netdev, LKML, Jon Maxwell
In-Reply-To: <CADVnQymzHMfjLKRRu0AmYa6PX6YXzyZ-2sF9TjyVfLZs6GVZTw@mail.gmail.com>

On Wed, Jul 4, 2018 at 12:13 AM, Neal Cardwell <ncardwell@google.com> wrote:
> On Tue, Jul 3, 2018 at 3:21 AM Jon Maxwell <jmaxwell37@gmail.com> wrote:
>>
>> v1 contains the following suggestions by Neal Cardwell:
>>
>> 1) Fix up units mismatch regarding msec/jiffies.
>> 2) Address possiblility of time_remaining being negative.
>> 3) Add a helper routine to do the rto calculation.
>>
>> Every time the TCP retransmission timer fires. It checks to see if there is a
>> timeout before scheduling the next retransmit timer. The retransmit interval
>> between each retransmission increases exponentially. The issue is that in order
>> for the timeout to occur the retransmit timer needs to fire again. If the user
>> timeout check happens after the 9th retransmit for example. It needs to wait for
>> the 10th retransmit timer to fire in order to evaluate whether a timeout has
>> occurred or not. If the interval is large enough then the timeout will be
>> inaccurate.
>>
>> For example with a TCP_USER_TIMEOUT of 10 seconds without patch:
>>
>> 1st retransmit:
>>
>> 22:25:18.973488 IP host1.49310 > host2.search-agent: Flags [.]
>>
>> Last retransmit:
>>
>> 22:25:26.205499 IP host1.49310 > host2.search-agent: Flags [.]
>>
>> Timeout:
>>
>> send: Connection timed out
>> Sun Jul  1 22:25:34 EDT 2018
>>
>> We can see that last retransmit took ~7 seconds. Which pushed the total
>> timeout to ~15 seconds instead of the expected 10 seconds. This gets more
>> inaccurate the larger the TCP_USER_TIMEOUT value. As the interval increases.
>>
>> Add tcp_clamp_rto_to_user_timeout() to determine if the user rto has expired.
>> Or whether the rto interval needs to be recalculated. Use the original interval
>> if user rto is not set.
>>
>> Test results with the patch is the expected 10 second timeout:
>>
>> 1st retransmit:
>>
>> 01:37:59.022555 IP host1.49310 > host2.search-agent: Flags [.]
>>
>> Last retransmit:
>>
>> 01:38:06.486558 IP host1.49310 > host2.search-agent: Flags [.]
>>
>> Timeout:
>>
>> send: Connection timed out
>> Mon Jul  2 01:38:09 EDT 2018
>>
>> Signed-off-by: Jon Maxwell <jmaxwell37@gmail.com>
>> ---
>>  net/ipv4/tcp_timer.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
>> index 3b3611729928..82c2a3b3713c 100644
>> --- a/net/ipv4/tcp_timer.c
>> +++ b/net/ipv4/tcp_timer.c
>> @@ -22,6 +22,23 @@
>>  #include <linux/gfp.h>
>>  #include <net/tcp.h>
>>
>> +static __u32 tcp_clamp_rto_to_user_timeout(struct sock *sk)
>> +{
>> +       struct inet_connection_sock *icsk = inet_csk(sk);
>> +       __u32 rto = icsk->icsk_rto;
>> +       __u32 elapsed, user_timeout;
>> +
>> +       if (!icsk->icsk_user_timeout)
>> +               return rto;
>> +       elapsed = tcp_time_stamp(tcp_sk(sk)) - tcp_sk(sk)->retrans_stamp;
>
> Thanks. The local logic seems OK to me now, but from reading
> retransmits_timed_out() it looks like at this point in the code we are
> not guaranteed that tcp_sk(sk)->retrans_stamp is initialized to
> something non-zero. So we probably need a preceding preparatory patch
> that factors out the first few lines of retransmits_timed_out()  into
> a helper frunction to get the start_ts for use in this calculation.
> Perhaps:
>
> u32 tcp_retrans_stamp():
>        start_ts = tcp_sk(sk)->retrans_stamp;
>        if (unlikely(!start_ts)) {
>                head = tcp_rtx_queue_head(sk);
>                if (!head)
>                        return 0;
>                start_ts = tcp_skb_timestamp(head);
>        }
>        return start_ts;
>
> And then the new tcp_clamp_rto_to_user_timeout() can use the helper:
>
> ...
> retrans_stamp = tcp_retransmit_stamp(sk);
> if (!retrans_stamp)
>    return rto;
> elapsed = tcp_time_stamp(tcp_sk(sk)) - retrans_stamp;
> ...
>
> Eric wrote those lines to recalculate start_ts, so we may want to wait
> until Eric returns to review this before merging the resulting patch
> series.
>

You are right. tcp_clamp_rto_to_user_timeout() should do the
same check as retransmits_timed_out() in regards to
tcp_sk(sk)->retrans_stamp. I'll add that to v2/test and then Eric can
comment on that if he has any input when he returns.

Thanks for all your help.

Regards

Jon

> neal

^ permalink raw reply

* Re: [RFC bpf-next 2/6] net: xdp: RX meta data infrastructure
From: Alexei Starovoitov @ 2018-07-03 23:01 UTC (permalink / raw)
  To: Saeed Mahameed
  Cc: brouer, borkmann, tariqt, alexander.h.duyck, peter.waskiewicz.jr,
	netdev, john.fastabend
In-Reply-To: <20180627024615.17856-3-saeedm@mellanox.com>

On Tue, Jun 26, 2018 at 07:46:11PM -0700, Saeed Mahameed wrote:
> The idea from this patch is to define a well known structure for XDP meta
> data fields format and offset placement inside the xdp data meta buffer.
> 
> For that driver will need some static information to know what to
> provide and where, enters struct xdp_md_info and xdp_md_info_arr:
> 
> struct xdp_md_info {
>        __u16 present:1;
>        __u16 offset:15; /* offset from data_meta in xdp_md buffer */
> };
> 
> /* XDP meta data offsets info array
>  * present bit describes if a meta data is or will be present in xdp_md buff
>  * offset describes where a meta data is or should be placed in xdp_md buff
>  *
>  * Kernel builds this array using xdp_md_info_build helper on demand.
>  * User space builds it statically in the xdp program.
>  */
> typedef struct xdp_md_info xdp_md_info_arr[XDP_DATA_META_MAX];
> 
> Offsets in xdp_md_info_arr are always in ascending order and only for
> requested meta data:
> example : flags = XDP_FLAGS_META_HASH | XDP_FLAGS_META_VLAN;
> 
> xdp_md_info_arr mdi = {
> 	[XDP_DATA_META_HASH] = {.offset = 0, .present = 1},
> 	[XDP_DATA_META_MARK] = {.offset = 0, .present = 0},
> 	[XDP_DATA_META_VLAN] = {.offset = sizeof(struct xdp_md_hash), .present = 1},
> 	[XDP_DATA_META_CSUM] = {.offset = 0, .present = 0},
> }
> 
> i.e: hash fields will always appear first then the vlan for every
> xdp_md.
> 
> Once requested to provide xdp meta data, device driver will use a pre-built
> xdp_md_info_arr which was built via xdp_md_info_build on xdp setup,
> xdp_md_info_arr will tell the driver what is the offset of each meta data.
> 
> This patch also provides helper functions for the device drivers to store
> meta data into xdp_buff, and helper function for XDP programs to fetch
> specific xdp meta data from xdp_md buffer.
> 
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
...
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 59b19b6a40d7..e320e7421565 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2353,6 +2353,120 @@ struct xdp_md {
>  	__u32 rx_queue_index;  /* rxq->queue_index  */
>  };
>  
> +enum {
> +	XDP_DATA_META_HASH = 0,
> +	XDP_DATA_META_MARK,
> +	XDP_DATA_META_VLAN,
> +	XDP_DATA_META_CSUM_COMPLETE,
> +
> +	XDP_DATA_META_MAX,
> +};
> +
> +struct xdp_md_hash {
> +	__u32 hash;
> +	__u8  type;
> +} __attribute__((aligned(4)));
> +
> +struct xdp_md_mark {
> +	__u32 mark;
> +} __attribute__((aligned(4)));
> +
> +struct xdp_md_vlan {
> +	__u16 vlan;
> +} __attribute__((aligned(4)));
> +
> +struct xdp_md_csum {
> +	__u16 csum;
> +} __attribute__((aligned(4)));
> +
> +static const __u16 xdp_md_size[XDP_DATA_META_MAX] = {
> +	sizeof(struct xdp_md_hash), /* XDP_DATA_META_HASH */
> +	sizeof(struct xdp_md_mark), /* XDP_DATA_META_MARK */
> +	sizeof(struct xdp_md_vlan), /* XDP_DATA_META_VLAN */
> +	sizeof(struct xdp_md_csum), /* XDP_DATA_META_CSUM_COMPLETE */
> +};
> +
> +struct xdp_md_info {
> +	__u16 present:1;
> +	__u16 offset:15; /* offset from data_meta in xdp_md buffer */
> +};
> +
> +/* XDP meta data offsets info array
> + * present bit describes if a meta data is or will be present in xdp_md buff
> + * offset describes where a meta data is or should be placed in xdp_md buff
> + *
> + * Kernel builds this array using xdp_md_info_build helper on demand.
> + * User space builds it statically in the xdp program.
> + */
> +typedef struct xdp_md_info xdp_md_info_arr[XDP_DATA_META_MAX];
> +
> +static __always_inline __u8
> +xdp_data_meta_present(xdp_md_info_arr mdi, int meta_data)
> +{
> +	return mdi[meta_data].present;
> +}
> +
> +static __always_inline void
> +xdp_data_meta_set_hash(xdp_md_info_arr mdi, void *data_meta, __u32 hash, __u8 htype)
> +{
> +	struct xdp_md_hash *hash_md;
> +
> +	hash_md = (struct xdp_md_hash *)((char*)data_meta + mdi[XDP_DATA_META_HASH].offset);
> +	hash_md->hash = hash;
> +	hash_md->type = htype;
> +}
> +
> +static __always_inline struct xdp_md_hash *
> +xdp_data_meta_get_hash(xdp_md_info_arr mdi, void *data_meta)
> +{
> +	return (struct xdp_md_hash *)((char*)data_meta + mdi[XDP_DATA_META_HASH].offset);
> +}

I'm afraid this is not scalable uapi.
This looks very much mlx specific. Every NIC vendor cannot add its own
struct definitions into uapi/bpf.h. It doesn't scale.
Also doing 15 bit bitfield extraction using bpf instructions is not that simple.
mlx should consider doing plain u8 or u16 fields in firmware instead.
The metadata that "hw" provides is a joint work of actual asic and firmware.
I suspect the format can and will change with different firmware.
Baking this stuff into uapi/bpf.h is not an option.
How about we make driver+firmware provide a BTF definition of metadata that they
can provide? There can be multiple definitions of such structs.
Then in userpsace we can have BTF->plain C converter.
(bpftool practically ready to do that already).
Then the programmer can take such generated C definition, add it to .h and include
it in their programs. llvm will compile the whole thing and will include BTF
of maps, progs and this md struct in the target elf file.
During loading the kernel can check that BTF in elf is matching one-to-one
to what driver+firmware are saying they support.
No ambiguity and no possibility of mistake, since offsets and field names
are verified.
Every driver can have their own BTF for md and their own special features.
We can try to standardize the names (like vlan and csum), so xdp programs
can stay relatively portable across NICs.
Such api will address exposing asic+firmware metadata to the xdp program.
Once we tackle this problem, we'll think how to do the backward config
(to do firmware reconfig for specific BTF definition of md supplied by the prog).
What people think?

^ permalink raw reply

* [PATCH v2 iproute2 2/2] tc: Add support for the ETF Qdisc
From: Jesus Sanchez-Palencia @ 2018-07-03 22:52 UTC (permalink / raw)
  To: netdev
  Cc: jhs, kurt.kanzenbach, xiyou.wangcong, jiri, vinicius.gomes,
	Jesus Sanchez-Palencia
In-Reply-To: <20180703225219.25526-1-jesus.sanchez-palencia@intel.com>

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>

The "Earliest TxTime First" (ETF) queueing discipline allows precise
control of the transmission time of packets by providing a sorted
time-based scheduling of packets.

The syntax is:

tc qdisc add dev DEV parent NODE etf delta <DELTA>
                     clockid <CLOCKID> [offload] [deadline_mode]

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
---
 tc/Makefile |   1 +
 tc/q_etf.c  | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 169 insertions(+)
 create mode 100644 tc/q_etf.c

diff --git a/tc/Makefile b/tc/Makefile
index dfd00267..4525c0fb 100644
--- a/tc/Makefile
+++ b/tc/Makefile
@@ -71,6 +71,7 @@ TCMODULES += q_clsact.o
 TCMODULES += e_bpf.o
 TCMODULES += f_matchall.o
 TCMODULES += q_cbs.o
+TCMODULES += q_etf.o
 
 TCSO :=
 ifeq ($(TC_CONFIG_ATM),y)
diff --git a/tc/q_etf.c b/tc/q_etf.c
new file mode 100644
index 00000000..5db1dd6f
--- /dev/null
+++ b/tc/q_etf.c
@@ -0,0 +1,168 @@
+/*
+ * q_etf.c		Earliest TxTime First (ETF).
+ *
+ *		This program is free software; you can redistribute it and/or
+ *		modify it under the terms of the GNU General Public License
+ *		as published by the Free Software Foundation; either version
+ *		2 of the License, or (at your option) any later version.
+ *
+ * Authors:	Vinicius Costa Gomes <vinicius.gomes@intel.com>
+ *		Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
+ *
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/ioctl.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+#include <string.h>
+
+#include "utils.h"
+#include "tc_util.h"
+
+#define CLOCKID_INVALID (-1)
+static void explain(void)
+{
+	fprintf(stderr, "Usage: ... etf delta NANOS clockid CLOCKID [offload] [deadline_mode]\n");
+	fprintf(stderr, "CLOCKID must be a valid SYS-V id (i.e. CLOCK_TAI)\n");
+}
+
+static void explain1(const char *arg, const char *val)
+{
+	fprintf(stderr, "etf: illegal value for \"%s\": \"%s\"\n", arg, val);
+}
+
+static void explain_clockid(const char *val)
+{
+	fprintf(stderr, "etf: illegal value for \"clockid\": \"%s\".\n", val);
+	fprintf(stderr, "It must be a valid SYS-V id (i.e. CLOCK_TAI)");
+}
+
+static int get_clockid(__s32 *val, const char *arg)
+{
+	const struct static_clockid {
+		const char *name;
+		clockid_t clockid;
+	} clockids_sysv[] = {
+		{ "CLOCK_REALTIME", CLOCK_REALTIME },
+		{ "CLOCK_TAI", CLOCK_TAI },
+		{ "CLOCK_BOOTTIME", CLOCK_BOOTTIME },
+		{ "CLOCK_MONOTONIC", CLOCK_MONOTONIC },
+		{ NULL }
+	};
+
+	const struct static_clockid *c;
+
+	for (c = clockids_sysv; c->name; c++) {
+		if (strncasecmp(c->name, arg, 25) == 0) {
+			*val = c->clockid;
+
+			return 0;
+		}
+	}
+
+	return -1;
+}
+
+
+static int etf_parse_opt(struct qdisc_util *qu, int argc,
+			 char **argv, struct nlmsghdr *n, const char *dev)
+{
+	struct tc_etf_qopt opt = {
+		.clockid = CLOCKID_INVALID,
+	};
+	struct rtattr *tail;
+
+	while (argc > 0) {
+		if (matches(*argv, "offload") == 0) {
+			if (opt.flags & TC_ETF_OFFLOAD_ON) {
+				fprintf(stderr, "etf: duplicate \"offload\" specification\n");
+				return -1;
+			}
+
+			opt.flags |= TC_ETF_OFFLOAD_ON;
+		} else if (matches(*argv, "deadline_mode") == 0) {
+			if (opt.flags & TC_ETF_DEADLINE_MODE_ON) {
+				fprintf(stderr, "etf: duplicate \"deadline_mode\" specification\n");
+				return -1;
+			}
+
+			opt.flags |= TC_ETF_DEADLINE_MODE_ON;
+		} else if (matches(*argv, "delta") == 0) {
+			NEXT_ARG();
+			if (opt.delta) {
+				fprintf(stderr, "etf: duplicate \"delta\" specification\n");
+				return -1;
+			}
+			if (get_s32(&opt.delta, *argv, 0)) {
+				explain1("delta", *argv);
+				return -1;
+			}
+		} else if (matches(*argv, "clockid") == 0) {
+			NEXT_ARG();
+			if (opt.clockid != CLOCKID_INVALID) {
+				fprintf(stderr, "etf: duplicate \"clockid\" specification\n");
+				return -1;
+			}
+			if (get_clockid(&opt.clockid, *argv)) {
+				explain_clockid(*argv);
+				return -1;
+			}
+		} else if (strcmp(*argv, "help") == 0) {
+			explain();
+			return -1;
+		} else {
+			fprintf(stderr, "etf: unknown parameter \"%s\"\n", *argv);
+			explain();
+			return -1;
+		}
+		argc--; argv++;
+	}
+
+	tail = NLMSG_TAIL(n);
+	addattr_l(n, 1024, TCA_OPTIONS, NULL, 0);
+	addattr_l(n, 2024, TCA_ETF_PARMS, &opt, sizeof(opt));
+	tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
+	return 0;
+}
+
+static int etf_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
+{
+	struct rtattr *tb[TCA_ETF_MAX+1];
+	struct tc_etf_qopt *qopt;
+
+	if (opt == NULL)
+		return 0;
+
+	parse_rtattr_nested(tb, TCA_ETF_MAX, opt);
+
+	if (tb[TCA_ETF_PARMS] == NULL)
+		return -1;
+
+	qopt = RTA_DATA(tb[TCA_ETF_PARMS]);
+	if (RTA_PAYLOAD(tb[TCA_ETF_PARMS])  < sizeof(*qopt))
+		return -1;
+
+	if (qopt->clockid == CLOCKID_INVALID)
+		print_string(PRINT_ANY, "clockid", "clockid %s ", "invalid");
+	else
+		print_uint(PRINT_ANY, "clockid", "clockid %d ", qopt->clockid);
+
+	print_uint(PRINT_ANY, "delta", "delta %d ", qopt->delta);
+	print_string(PRINT_ANY, "offload", "offload %s ",
+				(qopt->flags & TC_ETF_OFFLOAD_ON) ? "on" : "off");
+	print_string(PRINT_ANY, "deadline_mode", "deadline_mode %s",
+				(qopt->flags & TC_ETF_DEADLINE_MODE_ON) ? "on" : "off");
+
+	return 0;
+}
+
+struct qdisc_util etf_qdisc_util = {
+	.id		= "etf",
+	.parse_qopt	= etf_parse_opt,
+	.print_qopt	= etf_print_opt,
+};
-- 
2.17.1

^ permalink raw reply related

* [PATCH v2 iproute2 1/2] uapi pkt_sched: Add etf info - DO NOT COMMIT
From: Jesus Sanchez-Palencia @ 2018-07-03 22:52 UTC (permalink / raw)
  To: netdev
  Cc: jhs, kurt.kanzenbach, xiyou.wangcong, jiri, vinicius.gomes,
	Jesus Sanchez-Palencia

This should come from the next uapi headers update.
Sending it now just as a convenience so anyone can build tc with etf
and taprio support.

Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
---
 include/uapi/linux/pkt_sched.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 37b5096a..94911846 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -539,6 +539,7 @@ enum {
 	TCA_NETEM_LATENCY64,
 	TCA_NETEM_JITTER64,
 	TCA_NETEM_SLOT,
+	TCA_NETEM_SLOT_DIST,
 	__TCA_NETEM_MAX,
 };
 
@@ -581,6 +582,8 @@ struct tc_netem_slot {
 	__s64   max_delay;
 	__s32   max_packets;
 	__s32   max_bytes;
+	__s64	dist_delay; /* nsec */
+	__s64	dist_jitter; /* nsec */
 };
 
 enum {
@@ -934,4 +937,22 @@ enum {
 
 #define TCA_CBS_MAX (__TCA_CBS_MAX - 1)
 
+
+/* ETF */
+struct tc_etf_qopt {
+	__s32 delta;
+	__s32 clockid;
+	__u32 flags;
+#define TC_ETF_DEADLINE_MODE_ON	BIT(0)
+#define TC_ETF_OFFLOAD_ON	BIT(1)
+};
+
+enum {
+	TCA_ETF_UNSPEC,
+	TCA_ETF_PARMS,
+	__TCA_ETF_MAX,
+};
+
+#define TCA_ETF_MAX (__TCA_ETF_MAX - 1)
+
 #endif
-- 
2.17.1

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox