netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/6] tools: bpftool: add options for debug info from libbpf and verifier
@ 2019-04-29  9:52 Quentin Monnet
  2019-04-29  9:52 ` [PATCH bpf-next 1/6] tools: bpftool: add --log-libbpf option to get debug info from libbpf Quentin Monnet
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Quentin Monnet @ 2019-04-29  9:52 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann
  Cc: bpf, netdev, oss-drivers, Quentin Monnet

Hi,
This series adds several options to bpftool to make it print additional
information via libbpf or the kernel verifier when attempting to load
programs.

A first option is used to select the log level for libbpf, and a second one
is used for the verifier level. A third option (with a short name) is added
as a shortcut for printing all available information from both components.

A new API function is added to libbpf in order to pass the log_level from
bpftool with the bpf_object__* part of the API. Also, the flags defined to
name the verifier log levels are moved from kernel headers to UAPI headers,
in an effort to make it easier to users to pass the value they want.

Quentin Monnet (6):
  tools: bpftool: add --log-libbpf option to get debug info from libbpf
  tools: bpftool: add --log-all option to print all possible log info
  libbpf: add bpf_object__load_xattr() API function to pass log_level
  bpf: make BPF_LOG_* flags available in UAPI header
  tools: bpf: report latest changes from BPF UAPI header to tools
  tools: bpftool: add --log-verifier option to print kernel debug logs

 include/linux/bpf_verifier.h                  |   3 -
 include/uapi/linux/bpf.h                      |   5 +
 .../bpftool/Documentation/bpftool-prog.rst    |  20 ++++
 tools/bpf/bpftool/bash-completion/bpftool     |  46 +++++++-
 tools/bpf/bpftool/main.c                      | 105 ++++++++++++++++--
 tools/bpf/bpftool/main.h                      |   1 +
 tools/bpf/bpftool/prog.c                      |  24 ++--
 tools/include/uapi/linux/bpf.h                |   5 +
 tools/lib/bpf/libbpf.c                        |  20 +++-
 tools/lib/bpf/libbpf.h                        |   6 +
 tools/lib/bpf/libbpf.map                      |   1 +
 11 files changed, 211 insertions(+), 25 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH bpf-next 1/6] tools: bpftool: add --log-libbpf option to get debug info from libbpf
  2019-04-29  9:52 [PATCH bpf-next 0/6] tools: bpftool: add options for debug info from libbpf and verifier Quentin Monnet
@ 2019-04-29  9:52 ` Quentin Monnet
  2019-04-29 23:32   ` Y Song
  2019-04-29  9:52 ` [PATCH bpf-next 2/6] tools: bpftool: add --log-all option to print all possible log info Quentin Monnet
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Quentin Monnet @ 2019-04-29  9:52 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann
  Cc: bpf, netdev, oss-drivers, Quentin Monnet

libbpf has three levels of priority for output: warn, info, debug. By
default, debug output is not printed to stderr.

Add a new "--log-libbpf LOG_LEVEL" option to bpftool to provide more
flexibility on the log level for libbpf. LOG_LEVEL is a comma-separated
list of levels of log to print ("warn", "info", "debug"). The value
corresponding to the default behaviour would be "warn,info".

Internally, we simply use the function provided by libbpf to replace the
default printing function by one that prints logs for all required
levels.

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

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index e8118544d118..77d9570488d1 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -174,6 +174,12 @@ OPTIONS
 		  Do not automatically attempt to mount any virtual file system
 		  (such as tracefs or BPF virtual file system) when necessary.
 
+	--log-libbpf *LOG_LEVEL*
+		  Set the log level for libbpf output when attempting to load
+		  programs. *LOG_LEVEL* must be a comma-separated list of the
+		  levels of information to print, which can be **warn**,
+		  **info** or **debug**. The default is **warn,info**.
+
 EXAMPLES
 ========
 **# bpftool prog show**
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 50e402a5a9c8..a232da1b158d 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -44,6 +44,34 @@ _bpftool_one_of_list()
     COMPREPLY+=( $( compgen -W "$*" -- "$cur" ) )
 }
 
+# Complete a comma-separated list of items. For example:
+#     _bpftool_cslist "abc def ghi"
+# will suggest:
+#     - "abc" and "abc,"        to complete "a"
+#     - "abc,def" and "abc,ghi" to complete "abc,"
+#     - "abc,ghi,def"           to complete "abc,ghi,"
+_bpftool_cslist()
+{
+    local array_arg array_cur array_comp prevsubwords w ifs_back
+    read -r -a array_arg <<< "$*"
+    ifs_back=$IFS
+    IFS="," read -r -a array_cur <<< "$cur"
+    IFS=$ifs_back
+    prevsubwords=${cur%,*}
+    for w in "${array_arg[@]}"; do
+            if [[ ! "$cur" =~ "," ]]; then
+                array_comp+=( "$w" "$w," )
+            elif [[ ! "$cur" =~ "$w," ]]; then
+                if [[ "${#array_arg[@]}" > ${#array_cur[@]} ]]; then
+                    array_comp+=( "$prevsubwords,$w" "$prevsubwords,$w," )
+                else
+                    array_comp+=( "$prevsubwords,$w" )
+                fi
+            fi
+    done
+    COMPREPLY+=( $( compgen -W "${array_comp[*]}" -- "$cur" ) )
+}
+
 _bpftool_get_map_ids()
 {
     COMPREPLY+=( $( compgen -W "$( bpftool -jp map  2>&1 | \
@@ -181,7 +209,7 @@ _bpftool()
 
     # Deal with options
     if [[ ${words[cword]} == -* ]]; then
-        local c='--version --json --pretty --bpffs --mapcompat'
+        local c='--version --json --pretty --bpffs --mapcompat --log-libbpf'
         COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
         return 0
     fi
@@ -203,12 +231,23 @@ _bpftool()
             COMPREPLY=( $( compgen -W 'file' -- "$cur" ) )
             return 0
             ;;
+        --log-libbpf)
+            _bpftool_cslist 'warn info debug'
+            return 0
+            ;;
     esac
 
     # Remove all options so completions don't have to deal with them.
     local i
     for (( i=1; i < ${#words[@]}; )); do
         if [[ ${words[i]::1} == - ]]; then
+            # Remove arguments for options, if necessary
+            case ${words[i]} in
+                --log-libbpf)
+                    words=( "${words[@]:0:i+1}" "${words[@]:i+2}" )
+                    [[ $i -le $cword ]] && cword=$(( cword - 1 ))
+                    ;;
+            esac
             words=( "${words[@]:0:i}" "${words[@]:i+1}" )
             [[ $i -le $cword ]] && cword=$(( cword - 1 ))
         else
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 1ac1fc520e6a..6318be6feb5c 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -10,6 +10,7 @@
 #include <string.h>
 
 #include <bpf.h>
+#include <libbpf.h>
 
 #include "main.h"
 
@@ -26,6 +27,7 @@ bool json_output;
 bool show_pinned;
 bool block_mount;
 int bpf_flags;
+int log_level_libbpf;
 struct pinned_obj_table prog_table;
 struct pinned_obj_table map_table;
 
@@ -77,6 +79,46 @@ static int do_version(int argc, char **argv)
 	return 0;
 }
 
+static int __printf(2, 0)
+print_selected_levels(enum libbpf_print_level level, const char *format,
+		      va_list args)
+{
+	if (!(log_level_libbpf & (1 << level)))
+		return 0;
+
+	return vfprintf(stderr, format, args);
+}
+
+static int set_libbpf_loglevel(const char *log_str)
+{
+	char *log_str_cpy, *token;
+
+	log_str_cpy = strdup(log_str);
+	if (!log_str_cpy) {
+		p_err("mem alloc failed");
+		return -1;
+	}
+
+	token = strtok(log_str_cpy, ",");
+	while (token) {
+		if (is_prefix(token, "warn"))
+			log_level_libbpf |= (1 << LIBBPF_WARN);
+		else if (is_prefix(token, "info"))
+			log_level_libbpf |= (1 << LIBBPF_INFO);
+		else if (is_prefix(token, "debug"))
+			log_level_libbpf |= (1 << LIBBPF_DEBUG);
+		else
+			p_info("unrecognized log level for libbpf: %s", token);
+
+		token = strtok(NULL, ",");
+	}
+	free(log_str_cpy);
+
+	libbpf_set_print(print_selected_levels);
+
+	return 0;
+}
+
 int cmd_select(const struct cmd *cmds, int argc, char **argv,
 	       int (*help)(int argc, char **argv))
 {
@@ -310,13 +352,14 @@ static int do_batch(int argc, char **argv)
 int main(int argc, char **argv)
 {
 	static const struct option options[] = {
-		{ "json",	no_argument,	NULL,	'j' },
-		{ "help",	no_argument,	NULL,	'h' },
-		{ "pretty",	no_argument,	NULL,	'p' },
-		{ "version",	no_argument,	NULL,	'V' },
-		{ "bpffs",	no_argument,	NULL,	'f' },
-		{ "mapcompat",	no_argument,	NULL,	'm' },
-		{ "nomount",	no_argument,	NULL,	'n' },
+		{ "json",	no_argument,		NULL,	'j' },
+		{ "help",	no_argument,		NULL,	'h' },
+		{ "pretty",	no_argument,		NULL,	'p' },
+		{ "version",	no_argument,		NULL,	'V' },
+		{ "bpffs",	no_argument,		NULL,	'f' },
+		{ "mapcompat",	no_argument,		NULL,	'm' },
+		{ "nomount",	no_argument,		NULL,	'n' },
+		{ "log-libbpf",	required_argument,	NULL,	'd' },
 		{ 0 }
 	};
 	int opt, ret;
@@ -362,6 +405,10 @@ int main(int argc, char **argv)
 		case 'n':
 			block_mount = true;
 			break;
+		case 'd':
+			if (set_libbpf_loglevel(optarg))
+				return -1;
+			break;
 		default:
 			p_err("unrecognized option '%s'", argv[optind - 1]);
 			if (json_output)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH bpf-next 2/6] tools: bpftool: add --log-all option to print all possible log info
  2019-04-29  9:52 [PATCH bpf-next 0/6] tools: bpftool: add options for debug info from libbpf and verifier Quentin Monnet
  2019-04-29  9:52 ` [PATCH bpf-next 1/6] tools: bpftool: add --log-libbpf option to get debug info from libbpf Quentin Monnet
@ 2019-04-29  9:52 ` Quentin Monnet
  2019-04-29  9:52 ` [PATCH bpf-next 3/6] libbpf: add bpf_object__load_xattr() API function to pass log_level Quentin Monnet
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2019-04-29  9:52 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann
  Cc: bpf, netdev, oss-drivers, Quentin Monnet

The --log-all option is a shortcut for "--log-libbpf warn,info,debug".
It tells bpftool to print all possible logs from libbpf, and may be
extended in the future to set other log levels from other components as
well.

This option has a short name: "-l".

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

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 77d9570488d1..0525275f79f1 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -180,6 +180,10 @@ OPTIONS
 		  levels of information to print, which can be **warn**,
 		  **info** or **debug**. The default is **warn,info**.
 
+	-l, --log-all
+		  Print all possible log information. This is a shortcut for
+		  **--log-libbpf warn,info,debug**.
+
 EXAMPLES
 ========
 **# bpftool prog show**
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index a232da1b158d..f4ad75c6b243 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -209,7 +209,8 @@ _bpftool()
 
     # Deal with options
     if [[ ${words[cword]} == -* ]]; then
-        local c='--version --json --pretty --bpffs --mapcompat --log-libbpf'
+        local c='--version --json --pretty --bpffs --mapcompat \
+            --log-libbpf --log-all'
         COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
         return 0
     fi
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 6318be6feb5c..417cff76c7a1 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -360,6 +360,7 @@ int main(int argc, char **argv)
 		{ "mapcompat",	no_argument,		NULL,	'm' },
 		{ "nomount",	no_argument,		NULL,	'n' },
 		{ "log-libbpf",	required_argument,	NULL,	'd' },
+		{ "log-all",	no_argument,		NULL,	'l' },
 		{ 0 }
 	};
 	int opt, ret;
@@ -375,7 +376,7 @@ int main(int argc, char **argv)
 	hash_init(map_table.table);
 
 	opterr = 0;
-	while ((opt = getopt_long(argc, argv, "Vhpjfmn",
+	while ((opt = getopt_long(argc, argv, "Vhpjfmnl",
 				  options, NULL)) >= 0) {
 		switch (opt) {
 		case 'V':
@@ -409,6 +410,10 @@ int main(int argc, char **argv)
 			if (set_libbpf_loglevel(optarg))
 				return -1;
 			break;
+		case 'l':
+			if (set_libbpf_loglevel("warn,info,debug"))
+				return -1;
+			break;
 		default:
 			p_err("unrecognized option '%s'", argv[optind - 1]);
 			if (json_output)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH bpf-next 3/6] libbpf: add bpf_object__load_xattr() API function to pass log_level
  2019-04-29  9:52 [PATCH bpf-next 0/6] tools: bpftool: add options for debug info from libbpf and verifier Quentin Monnet
  2019-04-29  9:52 ` [PATCH bpf-next 1/6] tools: bpftool: add --log-libbpf option to get debug info from libbpf Quentin Monnet
  2019-04-29  9:52 ` [PATCH bpf-next 2/6] tools: bpftool: add --log-all option to print all possible log info Quentin Monnet
@ 2019-04-29  9:52 ` Quentin Monnet
  2019-04-29  9:52 ` [PATCH bpf-next 4/6] bpf: make BPF_LOG_* flags available in UAPI header Quentin Monnet
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2019-04-29  9:52 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann
  Cc: bpf, netdev, oss-drivers, Quentin Monnet

libbpf was recently made aware of the log_level attribute for programs,
used to specify the level of information expected to be dumped by the
verifier.

Create an API function to pass additional attributes when loading a
bpf_object, so we can set this log_level value in programs when loading
them, and so that so that applications relying on libbpf but not calling
bpf_prog_load_xattr() can also use that feature.

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

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 11a65db4b93f..9bba86404e65 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2095,7 +2095,7 @@ static bool bpf_program__is_function_storage(struct bpf_program *prog,
 }
 
 static int
-bpf_object__load_progs(struct bpf_object *obj)
+bpf_object__load_progs(struct bpf_object *obj, int log_level)
 {
 	size_t i;
 	int err;
@@ -2103,6 +2103,7 @@ bpf_object__load_progs(struct bpf_object *obj)
 	for (i = 0; i < obj->nr_programs; i++) {
 		if (bpf_program__is_function_storage(&obj->programs[i], obj))
 			continue;
+		obj->programs[i].log_level = log_level;
 		err = bpf_program__load(&obj->programs[i],
 					obj->license,
 					obj->kern_version);
@@ -2254,10 +2255,14 @@ int bpf_object__unload(struct bpf_object *obj)
 	return 0;
 }
 
-int bpf_object__load(struct bpf_object *obj)
+int bpf_object__load_xattr(struct bpf_object_load_attr *attr)
 {
+	struct bpf_object *obj;
 	int err;
 
+	if (!attr)
+		return -EINVAL;
+	obj = attr->obj;
 	if (!obj)
 		return -EINVAL;
 
@@ -2270,7 +2275,7 @@ int bpf_object__load(struct bpf_object *obj)
 
 	CHECK_ERR(bpf_object__create_maps(obj), err, out);
 	CHECK_ERR(bpf_object__relocate(obj), err, out);
-	CHECK_ERR(bpf_object__load_progs(obj), err, out);
+	CHECK_ERR(bpf_object__load_progs(obj, attr->log_level), err, out);
 
 	return 0;
 out:
@@ -2279,6 +2284,15 @@ int bpf_object__load(struct bpf_object *obj)
 	return err;
 }
 
+int bpf_object__load(struct bpf_object *obj)
+{
+	struct bpf_object_load_attr attr = {
+		.obj = obj,
+	};
+
+	return bpf_object__load_xattr(&attr);
+}
+
 static int check_path(const char *path)
 {
 	char *cp, errmsg[STRERR_BUFSIZE];
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index c5ff00515ce7..e1c748db44f6 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -89,8 +89,14 @@ LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
 LIBBPF_API int bpf_object__pin(struct bpf_object *object, const char *path);
 LIBBPF_API void bpf_object__close(struct bpf_object *object);
 
+struct bpf_object_load_attr {
+	struct bpf_object *obj;
+	int log_level;
+};
+
 /* Load/unload object into/from kernel */
 LIBBPF_API int bpf_object__load(struct bpf_object *obj);
+LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr);
 LIBBPF_API int bpf_object__unload(struct bpf_object *obj);
 LIBBPF_API const char *bpf_object__name(struct bpf_object *obj);
 LIBBPF_API unsigned int bpf_object__kversion(struct bpf_object *obj);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 673001787cba..d2d52c8641d2 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -162,5 +162,6 @@ LIBBPF_0.0.3 {
 	global:
 		bpf_map__is_internal;
 		bpf_map_freeze;
+		bpf_object__load_xattr;
 		btf__finalize_data;
 } LIBBPF_0.0.2;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH bpf-next 4/6] bpf: make BPF_LOG_* flags available in UAPI header
  2019-04-29  9:52 [PATCH bpf-next 0/6] tools: bpftool: add options for debug info from libbpf and verifier Quentin Monnet
                   ` (2 preceding siblings ...)
  2019-04-29  9:52 ` [PATCH bpf-next 3/6] libbpf: add bpf_object__load_xattr() API function to pass log_level Quentin Monnet
@ 2019-04-29  9:52 ` Quentin Monnet
  2019-05-05  6:19   ` Alexei Starovoitov
  2019-04-29  9:52 ` [PATCH bpf-next 5/6] tools: bpf: report latest changes from BPF UAPI header to tools Quentin Monnet
  2019-04-29  9:52 ` [PATCH bpf-next 6/6] tools: bpftool: add --log-verifier option to print kernel debug logs Quentin Monnet
  5 siblings, 1 reply; 13+ messages in thread
From: Quentin Monnet @ 2019-04-29  9:52 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann
  Cc: bpf, netdev, oss-drivers, Quentin Monnet

The kernel verifier combines several flags to select what kind of logs
to print to the log buffer provided by users.

In order to make it easier to provide the relevant flags, move the
related #define-s to the UAPI header, so that applications can set for
example: attr->log_level = BPF_LOG_LEVEL1 | BPF_LOG_STATS.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/linux/bpf_verifier.h | 3 ---
 include/uapi/linux/bpf.h     | 5 +++++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 1305ccbd8fe6..8160a4bb7ad9 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -253,9 +253,6 @@ static inline bool bpf_verifier_log_full(const struct bpf_verifier_log *log)
 	return log->len_used >= log->len_total - 1;
 }
 
-#define BPF_LOG_LEVEL1	1
-#define BPF_LOG_LEVEL2	2
-#define BPF_LOG_STATS	4
 #define BPF_LOG_LEVEL	(BPF_LOG_LEVEL1 | BPF_LOG_LEVEL2)
 #define BPF_LOG_MASK	(BPF_LOG_LEVEL | BPF_LOG_STATS)
 
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 72336bac7573..f8e3e764aff4 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -335,6 +335,11 @@ struct bpf_stack_build_id {
 	};
 };
 
+/* verifier log_level values for loading programs, can be combined */
+#define BPF_LOG_LEVEL1	1
+#define BPF_LOG_LEVEL2	2
+#define BPF_LOG_STATS	4
+
 union bpf_attr {
 	struct { /* anonymous struct used by BPF_MAP_CREATE command */
 		__u32	map_type;	/* one of enum bpf_map_type */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH bpf-next 5/6] tools: bpf: report latest changes from BPF UAPI header to tools
  2019-04-29  9:52 [PATCH bpf-next 0/6] tools: bpftool: add options for debug info from libbpf and verifier Quentin Monnet
                   ` (3 preceding siblings ...)
  2019-04-29  9:52 ` [PATCH bpf-next 4/6] bpf: make BPF_LOG_* flags available in UAPI header Quentin Monnet
@ 2019-04-29  9:52 ` Quentin Monnet
  2019-04-29  9:52 ` [PATCH bpf-next 6/6] tools: bpftool: add --log-verifier option to print kernel debug logs Quentin Monnet
  5 siblings, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2019-04-29  9:52 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann
  Cc: bpf, netdev, oss-drivers, Quentin Monnet

BPF verifier log level flags were moved from an internal header to the
UAPI header file. Report the changes accordingly.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/include/uapi/linux/bpf.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 72336bac7573..f8e3e764aff4 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -335,6 +335,11 @@ struct bpf_stack_build_id {
 	};
 };
 
+/* verifier log_level values for loading programs, can be combined */
+#define BPF_LOG_LEVEL1	1
+#define BPF_LOG_LEVEL2	2
+#define BPF_LOG_STATS	4
+
 union bpf_attr {
 	struct { /* anonymous struct used by BPF_MAP_CREATE command */
 		__u32	map_type;	/* one of enum bpf_map_type */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH bpf-next 6/6] tools: bpftool: add --log-verifier option to print kernel debug logs
  2019-04-29  9:52 [PATCH bpf-next 0/6] tools: bpftool: add options for debug info from libbpf and verifier Quentin Monnet
                   ` (4 preceding siblings ...)
  2019-04-29  9:52 ` [PATCH bpf-next 5/6] tools: bpf: report latest changes from BPF UAPI header to tools Quentin Monnet
@ 2019-04-29  9:52 ` Quentin Monnet
  5 siblings, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2019-04-29  9:52 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann
  Cc: bpf, netdev, oss-drivers, Quentin Monnet

Add a new --log-verifier option to set the log level for the kernel
verifier, even in case the program loads successfully. This can be used
to print verifier statistics, for example.

The mandatory argument is a comma-separated list of values, which can be
"level1", "level2", or "stats". The default behaviour for libbpf
consists in ignoring the verifier logs, unless the program fails to
load, in which case "level1" is used.

Example usage:

        # bpftool --log-verifier level1,stats --log-libbpf debug \
                prog load ...

Note that because of the way libbpf prints verifier output, the log
level for libbpf MUST include "debug" for output from the verifier to be
printed when the program loads successfully.

The "--log-all" option is updated to set all log level flags for the
verifier as well.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 .../bpftool/Documentation/bpftool-prog.rst    | 14 ++++-
 tools/bpf/bpftool/bash-completion/bpftool     |  8 ++-
 tools/bpf/bpftool/main.c                      | 55 ++++++++++++++++---
 tools/bpf/bpftool/main.h                      |  1 +
 tools/bpf/bpftool/prog.c                      | 24 ++++----
 5 files changed, 79 insertions(+), 23 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index 0525275f79f1..9c1fe14e607f 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -174,15 +174,25 @@ OPTIONS
 		  Do not automatically attempt to mount any virtual file system
 		  (such as tracefs or BPF virtual file system) when necessary.
 
-	--log-libbpf *LOG_LEVEL*
+	--log-libbpf   *LOG_LEVEL*
 		  Set the log level for libbpf output when attempting to load
 		  programs. *LOG_LEVEL* must be a comma-separated list of the
 		  levels of information to print, which can be **warn**,
 		  **info** or **debug**. The default is **warn,info**.
 
+	--log-verifier *LOG_LEVEL*
+		  Set the log level for the kernel verifier when attempting to
+		  load programs. *LOG_LEVEL* must be a comma-separated list of
+		  the levels of information to print, which can be **level1**,
+		  **level2** or **stats**. Note that to print this information
+		  on a successful load, the log level for libbpf must also
+		  include **debug**. Default behavior, defined by how libbpf
+		  loads the programs, is to print nothing on success, and
+		  **level1** if the program fails to load.
+
 	-l, --log-all
 		  Print all possible log information. This is a shortcut for
-		  **--log-libbpf warn,info,debug**.
+		  **--log-libbpf warn,info,debug --log-verifier level1,level2,stats**.
 
 EXAMPLES
 ========
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index f4ad75c6b243..f02a607f12ff 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -210,7 +210,7 @@ _bpftool()
     # Deal with options
     if [[ ${words[cword]} == -* ]]; then
         local c='--version --json --pretty --bpffs --mapcompat \
-            --log-libbpf --log-all'
+            --log-libbpf --log-verifier --log-all'
         COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
         return 0
     fi
@@ -236,6 +236,10 @@ _bpftool()
             _bpftool_cslist 'warn info debug'
             return 0
             ;;
+        --log-verifier)
+            _bpftool_cslist 'level1 level2 stats'
+            return 0
+            ;;
     esac
 
     # Remove all options so completions don't have to deal with them.
@@ -244,7 +248,7 @@ _bpftool()
         if [[ ${words[i]::1} == - ]]; then
             # Remove arguments for options, if necessary
             case ${words[i]} in
-                --log-libbpf)
+                --log-libbpf|--log-verifier)
                     words=( "${words[@]:0:i+1}" "${words[@]:i+2}" )
                     [[ $i -le $cword ]] && cword=$(( cword - 1 ))
                     ;;
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index 417cff76c7a1..2a4ea1b25b24 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -28,6 +28,7 @@ bool show_pinned;
 bool block_mount;
 int bpf_flags;
 int log_level_libbpf;
+int log_level_verifier;
 struct pinned_obj_table prog_table;
 struct pinned_obj_table map_table;
 
@@ -119,6 +120,35 @@ static int set_libbpf_loglevel(const char *log_str)
 	return 0;
 }
 
+static int set_verifier_loglevel(const char *log_str)
+{
+	char *log_str_cpy, *token;
+
+	log_str_cpy = strdup(log_str);
+	if (!log_str_cpy) {
+		p_err("mem alloc failed");
+		return -1;
+	}
+
+	token = strtok(log_str_cpy, ",");
+	while (token) {
+		if (is_prefix(token, "level1"))
+			log_level_verifier |= BPF_LOG_LEVEL1;
+		else if (is_prefix(token, "level2"))
+			log_level_verifier |= BPF_LOG_LEVEL2;
+		else if (is_prefix(token, "stats"))
+			log_level_verifier |= BPF_LOG_STATS;
+		else
+			p_info("unrecognized log level for verifier: %s",
+			       token);
+
+		token = strtok(NULL, ",");
+	}
+	free(log_str_cpy);
+
+	return 0;
+}
+
 int cmd_select(const struct cmd *cmds, int argc, char **argv,
 	       int (*help)(int argc, char **argv))
 {
@@ -352,15 +382,16 @@ static int do_batch(int argc, char **argv)
 int main(int argc, char **argv)
 {
 	static const struct option options[] = {
-		{ "json",	no_argument,		NULL,	'j' },
-		{ "help",	no_argument,		NULL,	'h' },
-		{ "pretty",	no_argument,		NULL,	'p' },
-		{ "version",	no_argument,		NULL,	'V' },
-		{ "bpffs",	no_argument,		NULL,	'f' },
-		{ "mapcompat",	no_argument,		NULL,	'm' },
-		{ "nomount",	no_argument,		NULL,	'n' },
-		{ "log-libbpf",	required_argument,	NULL,	'd' },
-		{ "log-all",	no_argument,		NULL,	'l' },
+		{ "json",		no_argument,		NULL,	'j' },
+		{ "help",		no_argument,		NULL,	'h' },
+		{ "pretty",		no_argument,		NULL,	'p' },
+		{ "version",		no_argument,		NULL,	'V' },
+		{ "bpffs",		no_argument,		NULL,	'f' },
+		{ "mapcompat",		no_argument,		NULL,	'm' },
+		{ "nomount",		no_argument,		NULL,	'n' },
+		{ "log-libbpf",		required_argument,	NULL,	'd' },
+		{ "log-verifier",	required_argument,	NULL,	'D' },
+		{ "log-all",		no_argument,		NULL,	'l' },
 		{ 0 }
 	};
 	int opt, ret;
@@ -410,9 +441,15 @@ int main(int argc, char **argv)
 			if (set_libbpf_loglevel(optarg))
 				return -1;
 			break;
+		case 'D':
+			if (set_verifier_loglevel(optarg))
+				return -1;
+			break;
 		case 'l':
 			if (set_libbpf_loglevel("warn,info,debug"))
 				return -1;
+			if (set_verifier_loglevel("level1,level2,stats"))
+				return -1;
 			break;
 		default:
 			p_err("unrecognized option '%s'", argv[optind - 1]);
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 3d63feb7f852..4dbfedbbbfab 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -92,6 +92,7 @@ extern bool json_output;
 extern bool show_pinned;
 extern bool block_mount;
 extern int bpf_flags;
+extern int log_level_verifier;
 extern struct pinned_obj_table prog_table;
 extern struct pinned_obj_table map_table;
 
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index fc495b27f0fc..bebece3b1fbf 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -750,10 +750,11 @@ static int do_detach(int argc, char **argv)
 
 static int load_with_options(int argc, char **argv, bool first_prog_only)
 {
-	enum bpf_attach_type expected_attach_type;
-	struct bpf_object_open_attr attr = {
-		.prog_type	= BPF_PROG_TYPE_UNSPEC,
+	struct bpf_object_load_attr load_attr = { 0 };
+	struct bpf_object_open_attr open_attr = {
+		.prog_type = BPF_PROG_TYPE_UNSPEC,
 	};
+	enum bpf_attach_type expected_attach_type;
 	struct map_replace *map_replace = NULL;
 	struct bpf_program *prog = NULL, *pos;
 	unsigned int old_map_fds = 0;
@@ -767,7 +768,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 
 	if (!REQ_ARGS(2))
 		return -1;
-	attr.file = GET_ARG();
+	open_attr.file = GET_ARG();
 	pinfile = GET_ARG();
 
 	while (argc) {
@@ -776,7 +777,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 
 			NEXT_ARG();
 
-			if (attr.prog_type != BPF_PROG_TYPE_UNSPEC) {
+			if (open_attr.prog_type != BPF_PROG_TYPE_UNSPEC) {
 				p_err("program type already specified");
 				goto err_free_reuse_maps;
 			}
@@ -793,7 +794,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 			strcat(type, *argv);
 			strcat(type, "/");
 
-			err = libbpf_prog_type_by_name(type, &attr.prog_type,
+			err = libbpf_prog_type_by_name(type,
+						       &open_attr.prog_type,
 						       &expected_attach_type);
 			free(type);
 			if (err < 0)
@@ -879,16 +881,16 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 		}
 	}
 
-	obj = __bpf_object__open_xattr(&attr, bpf_flags);
+	obj = __bpf_object__open_xattr(&open_attr, bpf_flags);
 	if (IS_ERR_OR_NULL(obj)) {
 		p_err("failed to open object file");
 		goto err_free_reuse_maps;
 	}
 
 	bpf_object__for_each_program(pos, obj) {
-		enum bpf_prog_type prog_type = attr.prog_type;
+		enum bpf_prog_type prog_type = open_attr.prog_type;
 
-		if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
+		if (open_attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
 			const char *sec_name = bpf_program__title(pos, false);
 
 			err = libbpf_prog_type_by_name(sec_name, &prog_type,
@@ -960,7 +962,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 
 	set_max_rlimit();
 
-	err = bpf_object__load(obj);
+	load_attr.obj = obj;
+	load_attr.log_level = log_level_verifier;
+	err = bpf_object__load_xattr(&load_attr);
 	if (err) {
 		p_err("failed to load object file");
 		goto err_close_obj;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next 1/6] tools: bpftool: add --log-libbpf option to get debug info from libbpf
  2019-04-29  9:52 ` [PATCH bpf-next 1/6] tools: bpftool: add --log-libbpf option to get debug info from libbpf Quentin Monnet
@ 2019-04-29 23:32   ` Y Song
  2019-04-30  9:34     ` Quentin Monnet
  0 siblings, 1 reply; 13+ messages in thread
From: Y Song @ 2019-04-29 23:32 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers

On Mon, Apr 29, 2019 at 2:53 AM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> libbpf has three levels of priority for output: warn, info, debug. By
> default, debug output is not printed to stderr.
>
> Add a new "--log-libbpf LOG_LEVEL" option to bpftool to provide more
> flexibility on the log level for libbpf. LOG_LEVEL is a comma-separated
> list of levels of log to print ("warn", "info", "debug"). The value
> corresponding to the default behaviour would be "warn,info".

Do you think option like "warn,debug" will be useful for bpftool users?
Maybe at bpftool level, we could allow user only to supply minimum level
for log output, e.g., "info" will output "warn,info"?

>
> Internally, we simply use the function provided by libbpf to replace the
> default printing function by one that prints logs for all required
> levels.
>
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>  .../bpftool/Documentation/bpftool-prog.rst    |  6 ++
>  tools/bpf/bpftool/bash-completion/bpftool     | 41 ++++++++++++-
>  tools/bpf/bpftool/main.c                      | 61 ++++++++++++++++---
>  3 files changed, 100 insertions(+), 8 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index e8118544d118..77d9570488d1 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -174,6 +174,12 @@ OPTIONS
>                   Do not automatically attempt to mount any virtual file system
>                   (such as tracefs or BPF virtual file system) when necessary.
>
> +       --log-libbpf *LOG_LEVEL*
> +                 Set the log level for libbpf output when attempting to load
> +                 programs. *LOG_LEVEL* must be a comma-separated list of the
> +                 levels of information to print, which can be **warn**,
> +                 **info** or **debug**. The default is **warn,info**.
> +
>  EXAMPLES
>  ========
>  **# bpftool prog show**
> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> index 50e402a5a9c8..a232da1b158d 100644
> --- a/tools/bpf/bpftool/bash-completion/bpftool
> +++ b/tools/bpf/bpftool/bash-completion/bpftool
> @@ -44,6 +44,34 @@ _bpftool_one_of_list()
>      COMPREPLY+=( $( compgen -W "$*" -- "$cur" ) )
>  }
>
> +# Complete a comma-separated list of items. For example:
> +#     _bpftool_cslist "abc def ghi"
> +# will suggest:
> +#     - "abc" and "abc,"        to complete "a"
> +#     - "abc,def" and "abc,ghi" to complete "abc,"
> +#     - "abc,ghi,def"           to complete "abc,ghi,"
> +_bpftool_cslist()
> +{
> +    local array_arg array_cur array_comp prevsubwords w ifs_back
> +    read -r -a array_arg <<< "$*"
> +    ifs_back=$IFS
> +    IFS="," read -r -a array_cur <<< "$cur"
> +    IFS=$ifs_back
> +    prevsubwords=${cur%,*}
> +    for w in "${array_arg[@]}"; do
> +            if [[ ! "$cur" =~ "," ]]; then
> +                array_comp+=( "$w" "$w," )
> +            elif [[ ! "$cur" =~ "$w," ]]; then
> +                if [[ "${#array_arg[@]}" > ${#array_cur[@]} ]]; then
> +                    array_comp+=( "$prevsubwords,$w" "$prevsubwords,$w," )
> +                else
> +                    array_comp+=( "$prevsubwords,$w" )
> +                fi
> +            fi
> +    done
> +    COMPREPLY+=( $( compgen -W "${array_comp[*]}" -- "$cur" ) )
> +}
> +
>  _bpftool_get_map_ids()
>  {
>      COMPREPLY+=( $( compgen -W "$( bpftool -jp map  2>&1 | \
> @@ -181,7 +209,7 @@ _bpftool()
>
>      # Deal with options
>      if [[ ${words[cword]} == -* ]]; then
> -        local c='--version --json --pretty --bpffs --mapcompat'
> +        local c='--version --json --pretty --bpffs --mapcompat --log-libbpf'
>          COMPREPLY=( $( compgen -W "$c" -- "$cur" ) )
>          return 0
>      fi
> @@ -203,12 +231,23 @@ _bpftool()
>              COMPREPLY=( $( compgen -W 'file' -- "$cur" ) )
>              return 0
>              ;;
> +        --log-libbpf)
> +            _bpftool_cslist 'warn info debug'
> +            return 0
> +            ;;
>      esac
>
>      # Remove all options so completions don't have to deal with them.
>      local i
>      for (( i=1; i < ${#words[@]}; )); do
>          if [[ ${words[i]::1} == - ]]; then
> +            # Remove arguments for options, if necessary
> +            case ${words[i]} in
> +                --log-libbpf)
> +                    words=( "${words[@]:0:i+1}" "${words[@]:i+2}" )
> +                    [[ $i -le $cword ]] && cword=$(( cword - 1 ))
> +                    ;;
> +            esac
>              words=( "${words[@]:0:i}" "${words[@]:i+1}" )
>              [[ $i -le $cword ]] && cword=$(( cword - 1 ))
>          else
> diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> index 1ac1fc520e6a..6318be6feb5c 100644
> --- a/tools/bpf/bpftool/main.c
> +++ b/tools/bpf/bpftool/main.c
> @@ -10,6 +10,7 @@
>  #include <string.h>
>
>  #include <bpf.h>
> +#include <libbpf.h>
>
>  #include "main.h"
>
> @@ -26,6 +27,7 @@ bool json_output;
>  bool show_pinned;
>  bool block_mount;
>  int bpf_flags;
> +int log_level_libbpf;
>  struct pinned_obj_table prog_table;
>  struct pinned_obj_table map_table;
>
> @@ -77,6 +79,46 @@ static int do_version(int argc, char **argv)
>         return 0;
>  }
>
> +static int __printf(2, 0)
> +print_selected_levels(enum libbpf_print_level level, const char *format,
> +                     va_list args)
> +{
> +       if (!(log_level_libbpf & (1 << level)))
> +               return 0;
> +
> +       return vfprintf(stderr, format, args);
> +}
> +
> +static int set_libbpf_loglevel(const char *log_str)
> +{
> +       char *log_str_cpy, *token;
> +
> +       log_str_cpy = strdup(log_str);
> +       if (!log_str_cpy) {
> +               p_err("mem alloc failed");
> +               return -1;
> +       }
> +
> +       token = strtok(log_str_cpy, ",");
> +       while (token) {
> +               if (is_prefix(token, "warn"))
> +                       log_level_libbpf |= (1 << LIBBPF_WARN);
> +               else if (is_prefix(token, "info"))
> +                       log_level_libbpf |= (1 << LIBBPF_INFO);
> +               else if (is_prefix(token, "debug"))
> +                       log_level_libbpf |= (1 << LIBBPF_DEBUG);
> +               else
> +                       p_info("unrecognized log level for libbpf: %s", token);
> +
> +               token = strtok(NULL, ",");
> +       }
> +       free(log_str_cpy);
> +
> +       libbpf_set_print(print_selected_levels);
> +
> +       return 0;
> +}
> +
>  int cmd_select(const struct cmd *cmds, int argc, char **argv,
>                int (*help)(int argc, char **argv))
>  {
> @@ -310,13 +352,14 @@ static int do_batch(int argc, char **argv)
>  int main(int argc, char **argv)
>  {
>         static const struct option options[] = {
> -               { "json",       no_argument,    NULL,   'j' },
> -               { "help",       no_argument,    NULL,   'h' },
> -               { "pretty",     no_argument,    NULL,   'p' },
> -               { "version",    no_argument,    NULL,   'V' },
> -               { "bpffs",      no_argument,    NULL,   'f' },
> -               { "mapcompat",  no_argument,    NULL,   'm' },
> -               { "nomount",    no_argument,    NULL,   'n' },
> +               { "json",       no_argument,            NULL,   'j' },
> +               { "help",       no_argument,            NULL,   'h' },
> +               { "pretty",     no_argument,            NULL,   'p' },
> +               { "version",    no_argument,            NULL,   'V' },
> +               { "bpffs",      no_argument,            NULL,   'f' },
> +               { "mapcompat",  no_argument,            NULL,   'm' },
> +               { "nomount",    no_argument,            NULL,   'n' },
> +               { "log-libbpf", required_argument,      NULL,   'd' },
>                 { 0 }
>         };
>         int opt, ret;
> @@ -362,6 +405,10 @@ int main(int argc, char **argv)
>                 case 'n':
>                         block_mount = true;
>                         break;
> +               case 'd':
> +                       if (set_libbpf_loglevel(optarg))
> +                               return -1;
> +                       break;
>                 default:
>                         p_err("unrecognized option '%s'", argv[optind - 1]);
>                         if (json_output)
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next 1/6] tools: bpftool: add --log-libbpf option to get debug info from libbpf
  2019-04-29 23:32   ` Y Song
@ 2019-04-30  9:34     ` Quentin Monnet
  2019-04-30 15:31       ` Y Song
  0 siblings, 1 reply; 13+ messages in thread
From: Quentin Monnet @ 2019-04-30  9:34 UTC (permalink / raw)
  To: Y Song; +Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers

Hi Yonghong,

2019-04-29 16:32 UTC-0700 ~ Y Song <ys114321@gmail.com>
> On Mon, Apr 29, 2019 at 2:53 AM Quentin Monnet
> <quentin.monnet@netronome.com> wrote:
>>
>> libbpf has three levels of priority for output: warn, info, debug. By
>> default, debug output is not printed to stderr.
>>
>> Add a new "--log-libbpf LOG_LEVEL" option to bpftool to provide more
>> flexibility on the log level for libbpf. LOG_LEVEL is a comma-separated
>> list of levels of log to print ("warn", "info", "debug"). The value
>> corresponding to the default behaviour would be "warn,info".
> 
> Do you think option like "warn,debug" will be useful for bpftool users?
> Maybe at bpftool level, we could allow user only to supply minimum level
> for log output, e.g., "info" will output "warn,info"?
I've been pondering this, too. Since we allow to combine all levels for 
the verifier logs it feels a bit odd to be less flexible for libbpf. And 
we could imagine a user who wants verifier logs (so libbpf "debug") but 
prefers to limit libbpf output (so no "info")... Although I admit this 
might be a bit far-fetched.

I can resend a version with the option taking only the minimal log 
level, as you describe, if you think this is best.

Quentin

> 
>>
>> Internally, we simply use the function provided by libbpf to replace the
>> default printing function by one that prints logs for all required
>> levels.
>>
>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next 1/6] tools: bpftool: add --log-libbpf option to get debug info from libbpf
  2019-04-30  9:34     ` Quentin Monnet
@ 2019-04-30 15:31       ` Y Song
  2019-04-30 19:39         ` Jakub Kicinski
  0 siblings, 1 reply; 13+ messages in thread
From: Y Song @ 2019-04-30 15:31 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers

On Tue, Apr 30, 2019 at 2:34 AM Quentin Monnet
<quentin.monnet@netronome.com> wrote:
>
> Hi Yonghong,
>
> 2019-04-29 16:32 UTC-0700 ~ Y Song <ys114321@gmail.com>
> > On Mon, Apr 29, 2019 at 2:53 AM Quentin Monnet
> > <quentin.monnet@netronome.com> wrote:
> >>
> >> libbpf has three levels of priority for output: warn, info, debug. By
> >> default, debug output is not printed to stderr.
> >>
> >> Add a new "--log-libbpf LOG_LEVEL" option to bpftool to provide more
> >> flexibility on the log level for libbpf. LOG_LEVEL is a comma-separated
> >> list of levels of log to print ("warn", "info", "debug"). The value
> >> corresponding to the default behaviour would be "warn,info".
> >
> > Do you think option like "warn,debug" will be useful for bpftool users?
> > Maybe at bpftool level, we could allow user only to supply minimum level
> > for log output, e.g., "info" will output "warn,info"?
> I've been pondering this, too. Since we allow to combine all levels for
> the verifier logs it feels a bit odd to be less flexible for libbpf. And
> we could imagine a user who wants verifier logs (so libbpf "debug") but
> prefers to limit libbpf output (so no "info")... Although I admit this
> might be a bit far-fetched.
>
> I can resend a version with the option taking only the minimal log
> level, as you describe, if you think this is best.

Thanks, I think providing a single minimum level for output probably
better.

Yonghong

>
> Quentin
>
> >
> >>
> >> Internally, we simply use the function provided by libbpf to replace the
> >> default printing function by one that prints logs for all required
> >> levels.
> >>
> >> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> >> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next 1/6] tools: bpftool: add --log-libbpf option to get debug info from libbpf
  2019-04-30 15:31       ` Y Song
@ 2019-04-30 19:39         ` Jakub Kicinski
  0 siblings, 0 replies; 13+ messages in thread
From: Jakub Kicinski @ 2019-04-30 19:39 UTC (permalink / raw)
  To: Y Song
  Cc: Quentin Monnet, Alexei Starovoitov, Daniel Borkmann, bpf, netdev,
	oss-drivers

On Tue, 30 Apr 2019 08:31:53 -0700, Y Song wrote:
> On Tue, Apr 30, 2019 at 2:34 AM Quentin Monnet
> <quentin.monnet@netronome.com> wrote:
> >
> > Hi Yonghong,
> >
> > 2019-04-29 16:32 UTC-0700 ~ Y Song <ys114321@gmail.com>  
> > > On Mon, Apr 29, 2019 at 2:53 AM Quentin Monnet
> > > <quentin.monnet@netronome.com> wrote:  
> > >>
> > >> libbpf has three levels of priority for output: warn, info, debug. By
> > >> default, debug output is not printed to stderr.
> > >>
> > >> Add a new "--log-libbpf LOG_LEVEL" option to bpftool to provide more
> > >> flexibility on the log level for libbpf. LOG_LEVEL is a comma-separated
> > >> list of levels of log to print ("warn", "info", "debug"). The value
> > >> corresponding to the default behaviour would be "warn,info".  
> > >
> > > Do you think option like "warn,debug" will be useful for bpftool users?
> > > Maybe at bpftool level, we could allow user only to supply minimum level
> > > for log output, e.g., "info" will output "warn,info"?  
> > I've been pondering this, too. Since we allow to combine all levels for
> > the verifier logs it feels a bit odd to be less flexible for libbpf. And
> > we could imagine a user who wants verifier logs (so libbpf "debug") but
> > prefers to limit libbpf output (so no "info")... Although I admit this
> > might be a bit far-fetched.
> >
> > I can resend a version with the option taking only the minimal log
> > level, as you describe, if you think this is best.  
> 
> Thanks, I think providing a single minimum level for output probably
> better.

I have a weak preference for what we have here, because it's similar to
the kernel bit opt in (log level, stats etc)..

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next 4/6] bpf: make BPF_LOG_* flags available in UAPI header
  2019-04-29  9:52 ` [PATCH bpf-next 4/6] bpf: make BPF_LOG_* flags available in UAPI header Quentin Monnet
@ 2019-05-05  6:19   ` Alexei Starovoitov
  2019-05-07 17:06     ` Quentin Monnet
  0 siblings, 1 reply; 13+ messages in thread
From: Alexei Starovoitov @ 2019-05-05  6:19 UTC (permalink / raw)
  To: Quentin Monnet
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers

On Mon, Apr 29, 2019 at 10:52:25AM +0100, Quentin Monnet wrote:
> The kernel verifier combines several flags to select what kind of logs
> to print to the log buffer provided by users.
> 
> In order to make it easier to provide the relevant flags, move the
> related #define-s to the UAPI header, so that applications can set for
> example: attr->log_level = BPF_LOG_LEVEL1 | BPF_LOG_STATS.
> 
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
>  include/linux/bpf_verifier.h | 3 ---
>  include/uapi/linux/bpf.h     | 5 +++++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 1305ccbd8fe6..8160a4bb7ad9 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -253,9 +253,6 @@ static inline bool bpf_verifier_log_full(const struct bpf_verifier_log *log)
>  	return log->len_used >= log->len_total - 1;
>  }
>  
> -#define BPF_LOG_LEVEL1	1
> -#define BPF_LOG_LEVEL2	2
> -#define BPF_LOG_STATS	4
>  #define BPF_LOG_LEVEL	(BPF_LOG_LEVEL1 | BPF_LOG_LEVEL2)
>  #define BPF_LOG_MASK	(BPF_LOG_LEVEL | BPF_LOG_STATS)
>  
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 72336bac7573..f8e3e764aff4 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -335,6 +335,11 @@ struct bpf_stack_build_id {
>  	};
>  };
>  
> +/* verifier log_level values for loading programs, can be combined */
> +#define BPF_LOG_LEVEL1	1
> +#define BPF_LOG_LEVEL2	2
> +#define BPF_LOG_STATS	4

The verifier log levels are kernel implementation details.
They were not exposed before and shouldn't be exposed in the future.
I know that some folks already know about existence of level2 and use it
when the verifier rejects the program, but this is not uapi.
What is being output at level1 and 2 can change.
It's ok for libbpf to use this knowledge of kernel internals,
but it shouldn't be in uapi header.
That was the reason I didn't expose stats=4 in uapi in the first place
when I added that commit.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH bpf-next 4/6] bpf: make BPF_LOG_* flags available in UAPI header
  2019-05-05  6:19   ` Alexei Starovoitov
@ 2019-05-07 17:06     ` Quentin Monnet
  0 siblings, 0 replies; 13+ messages in thread
From: Quentin Monnet @ 2019-05-07 17:06 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Alexei Starovoitov, Daniel Borkmann, bpf, netdev, oss-drivers,
	Yonghong Song, Jakub Kicinski

2019-05-04 23:19 UTC-0700 ~ Alexei Starovoitov
<alexei.starovoitov@gmail.com>
> On Mon, Apr 29, 2019 at 10:52:25AM +0100, Quentin Monnet wrote:
>> The kernel verifier combines several flags to select what kind of logs
>> to print to the log buffer provided by users.
>>
>> In order to make it easier to provide the relevant flags, move the
>> related #define-s to the UAPI header, so that applications can set for
>> example: attr->log_level = BPF_LOG_LEVEL1 | BPF_LOG_STATS.
>>
>> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> ---
>>  include/linux/bpf_verifier.h | 3 ---
>>  include/uapi/linux/bpf.h     | 5 +++++
>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index 1305ccbd8fe6..8160a4bb7ad9 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -253,9 +253,6 @@ static inline bool bpf_verifier_log_full(const struct bpf_verifier_log *log)
>>  	return log->len_used >= log->len_total - 1;
>>  }
>>  
>> -#define BPF_LOG_LEVEL1	1
>> -#define BPF_LOG_LEVEL2	2
>> -#define BPF_LOG_STATS	4
>>  #define BPF_LOG_LEVEL	(BPF_LOG_LEVEL1 | BPF_LOG_LEVEL2)
>>  #define BPF_LOG_MASK	(BPF_LOG_LEVEL | BPF_LOG_STATS)
>>  
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 72336bac7573..f8e3e764aff4 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -335,6 +335,11 @@ struct bpf_stack_build_id {
>>  	};
>>  };
>>  
>> +/* verifier log_level values for loading programs, can be combined */
>> +#define BPF_LOG_LEVEL1	1
>> +#define BPF_LOG_LEVEL2	2
>> +#define BPF_LOG_STATS	4
> 
> The verifier log levels are kernel implementation details.
> They were not exposed before and shouldn't be exposed in the future.
> I know that some folks already know about existence of level2 and use it
> when the verifier rejects the program, but this is not uapi.
> What is being output at level1 and 2 can change.
> It's ok for libbpf to use this knowledge of kernel internals,
> but it shouldn't be in uapi header.
> That was the reason I didn't expose stats=4 in uapi in the first place
> when I added that commit.
> 

Ok, in that case I will not move the macros. I take it there is also
little sense in offering different levels for the verifier through the
command line (the "--log-verifier level1,level2,stats" proposed in patch 6).

Since there was no real consensus on libbpf log level syntax either,
I'll resubmit the series (once bpf-next reopens) with just the shortcut
option, that sets all log levels to their maximum but without presenting
the different levels to the users. We can still add other options for
finer control over log levels after that, if they become desirable.

Thanks,
Quentin

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-05-07 17:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-29  9:52 [PATCH bpf-next 0/6] tools: bpftool: add options for debug info from libbpf and verifier Quentin Monnet
2019-04-29  9:52 ` [PATCH bpf-next 1/6] tools: bpftool: add --log-libbpf option to get debug info from libbpf Quentin Monnet
2019-04-29 23:32   ` Y Song
2019-04-30  9:34     ` Quentin Monnet
2019-04-30 15:31       ` Y Song
2019-04-30 19:39         ` Jakub Kicinski
2019-04-29  9:52 ` [PATCH bpf-next 2/6] tools: bpftool: add --log-all option to print all possible log info Quentin Monnet
2019-04-29  9:52 ` [PATCH bpf-next 3/6] libbpf: add bpf_object__load_xattr() API function to pass log_level Quentin Monnet
2019-04-29  9:52 ` [PATCH bpf-next 4/6] bpf: make BPF_LOG_* flags available in UAPI header Quentin Monnet
2019-05-05  6:19   ` Alexei Starovoitov
2019-05-07 17:06     ` Quentin Monnet
2019-04-29  9:52 ` [PATCH bpf-next 5/6] tools: bpf: report latest changes from BPF UAPI header to tools Quentin Monnet
2019-04-29  9:52 ` [PATCH bpf-next 6/6] tools: bpftool: add --log-verifier option to print kernel debug logs Quentin Monnet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).