* [PATCH v4 bpf-next 1/9] bpf: merge printk and seq_printf VARARG max macros
2021-09-02 17:19 [PATCH v4 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
@ 2021-09-02 17:19 ` Dave Marchevsky
2021-09-02 17:19 ` [PATCH v4 bpf-next 2/9] selftests/bpf: stop using bpf_program__load Dave Marchevsky
` (7 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Dave Marchevsky @ 2021-09-02 17:19 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
MAX_SNPRINTF_VARARGS and MAX_SEQ_PRINTF_VARARGS are used by bpf helpers
bpf_snprintf and bpf_seq_printf to limit their varargs. Both call into
bpf_bprintf_prepare for print formatting logic and have convenience
macros in libbpf (BPF_SNPRINTF, BPF_SEQ_PRINTF) which use the same
helper macros to convert varargs to a byte array.
Changing shared functionality to support more varargs for either bpf
helper would affect the other as well, so let's combine the _VARARGS
macros to make this more obvious.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
include/linux/bpf.h | 2 ++
kernel/bpf/helpers.c | 4 +---
kernel/trace/bpf_trace.c | 4 +---
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f4c16f19f83e..be8d57e6e78a 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2216,6 +2216,8 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
struct btf_id_set;
bool btf_id_set_contains(const struct btf_id_set *set, u32 id);
+#define MAX_BPRINTF_VARARGS 12
+
int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
u32 **bin_buf, u32 num_args);
void bpf_bprintf_cleanup(void);
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 9aabf84afd4b..8f9f392c1322 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -979,15 +979,13 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
return err;
}
-#define MAX_SNPRINTF_VARARGS 12
-
BPF_CALL_5(bpf_snprintf, char *, str, u32, str_size, char *, fmt,
const void *, data, u32, data_len)
{
int err, num_args;
u32 *bin_args;
- if (data_len % 8 || data_len > MAX_SNPRINTF_VARARGS * 8 ||
+ if (data_len % 8 || data_len > MAX_BPRINTF_VARARGS * 8 ||
(data_len && !data))
return -EINVAL;
num_args = data_len / 8;
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 8e2eb950aa82..10672ebc63b7 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -414,15 +414,13 @@ const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
return &bpf_trace_printk_proto;
}
-#define MAX_SEQ_PRINTF_VARARGS 12
-
BPF_CALL_5(bpf_seq_printf, struct seq_file *, m, char *, fmt, u32, fmt_size,
const void *, data, u32, data_len)
{
int err, num_args;
u32 *bin_args;
- if (data_len & 7 || data_len > MAX_SEQ_PRINTF_VARARGS * 8 ||
+ if (data_len & 7 || data_len > MAX_BPRINTF_VARARGS * 8 ||
(data_len && !data))
return -EINVAL;
num_args = data_len / 8;
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 bpf-next 2/9] selftests/bpf: stop using bpf_program__load
2021-09-02 17:19 [PATCH v4 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
2021-09-02 17:19 ` [PATCH v4 bpf-next 1/9] bpf: merge printk and seq_printf VARARG max macros Dave Marchevsky
@ 2021-09-02 17:19 ` Dave Marchevsky
2021-09-02 22:58 ` Andrii Nakryiko
2021-09-02 17:19 ` [PATCH v4 bpf-next 3/9] bpf: add bpf_trace_vprintk helper Dave Marchevsky
` (6 subsequent siblings)
8 siblings, 1 reply; 16+ messages in thread
From: Dave Marchevsky @ 2021-09-02 17:19 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
bpf_program__load is not supposed to be used directly. Replace it with
bpf_object__ APIs for the reference_tracking prog_test, which is the
last offender in bpf selftests.
Some additional complexity is added for this test, namely the use of one
bpf_object to iterate through progs, while a second bpf_object is
created and opened/closed to test actual loading of progs. This is
because the test was doing bpf_program__load then __unload to test
loading of individual progs and same semantics with
bpf_object__load/__unload result in failure to load an __unload-ed obj.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
.../bpf/prog_tests/reference_tracking.c | 39 +++++++++++++++----
1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
index 4e91f4d6466c..ded2dc8ddd79 100644
--- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
+++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
@@ -1,6 +1,21 @@
// SPDX-License-Identifier: GPL-2.0
#include <test_progs.h>
+static void toggle_object_autoload_progs(const struct bpf_object *obj,
+ const char *title_load)
+{
+ struct bpf_program *prog;
+
+ bpf_object__for_each_program(prog, obj) {
+ const char *title = bpf_program__section_name(prog);
+
+ if (!strcmp(title_load, title))
+ bpf_program__set_autoload(prog, true);
+ else
+ bpf_program__set_autoload(prog, false);
+ }
+}
+
void test_reference_tracking(void)
{
const char *file = "test_sk_lookup_kern.o";
@@ -9,21 +24,21 @@ void test_reference_tracking(void)
.object_name = obj_name,
.relaxed_maps = true,
);
- struct bpf_object *obj;
+ struct bpf_object *obj_iter, *obj = NULL;
struct bpf_program *prog;
__u32 duration = 0;
int err = 0;
- obj = bpf_object__open_file(file, &open_opts);
- if (!ASSERT_OK_PTR(obj, "obj_open_file"))
+ obj_iter = bpf_object__open_file(file, &open_opts);
+ if (!ASSERT_OK_PTR(obj_iter, "obj_iter_open_file"))
return;
- if (CHECK(strcmp(bpf_object__name(obj), obj_name), "obj_name",
+ if (CHECK(strcmp(bpf_object__name(obj_iter), obj_name), "obj_name",
"wrong obj name '%s', expected '%s'\n",
- bpf_object__name(obj), obj_name))
+ bpf_object__name(obj_iter), obj_name))
goto cleanup;
- bpf_object__for_each_program(prog, obj) {
+ bpf_object__for_each_program(prog, obj_iter) {
const char *title;
/* Ignore .text sections */
@@ -34,19 +49,27 @@ void test_reference_tracking(void)
if (!test__start_subtest(title))
continue;
+ obj = bpf_object__open_file(file, &open_opts);
+ if (!ASSERT_OK_PTR(obj, "obj_open_file"))
+ goto cleanup;
+
+ toggle_object_autoload_progs(obj, title);
/* Expect verifier failure if test name has 'err' */
if (strstr(title, "err_") != NULL) {
libbpf_print_fn_t old_print_fn;
old_print_fn = libbpf_set_print(NULL);
- err = !bpf_program__load(prog, "GPL", 0);
+ err = !bpf_object__load(obj);
libbpf_set_print(old_print_fn);
} else {
- err = bpf_program__load(prog, "GPL", 0);
+ err = bpf_object__load(obj);
}
CHECK(err, title, "\n");
+ bpf_object__close(obj);
+ obj = NULL;
}
cleanup:
bpf_object__close(obj);
+ bpf_object__close(obj_iter);
}
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 bpf-next 2/9] selftests/bpf: stop using bpf_program__load
2021-09-02 17:19 ` [PATCH v4 bpf-next 2/9] selftests/bpf: stop using bpf_program__load Dave Marchevsky
@ 2021-09-02 22:58 ` Andrii Nakryiko
0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2021-09-02 22:58 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, Networking
On Thu, Sep 2, 2021 at 10:20 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> bpf_program__load is not supposed to be used directly. Replace it with
> bpf_object__ APIs for the reference_tracking prog_test, which is the
> last offender in bpf selftests.
>
> Some additional complexity is added for this test, namely the use of one
> bpf_object to iterate through progs, while a second bpf_object is
> created and opened/closed to test actual loading of progs. This is
> because the test was doing bpf_program__load then __unload to test
> loading of individual progs and same semantics with
> bpf_object__load/__unload result in failure to load an __unload-ed obj.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
LGTM.
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> .../bpf/prog_tests/reference_tracking.c | 39 +++++++++++++++----
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
[...]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 bpf-next 3/9] bpf: add bpf_trace_vprintk helper
2021-09-02 17:19 [PATCH v4 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
2021-09-02 17:19 ` [PATCH v4 bpf-next 1/9] bpf: merge printk and seq_printf VARARG max macros Dave Marchevsky
2021-09-02 17:19 ` [PATCH v4 bpf-next 2/9] selftests/bpf: stop using bpf_program__load Dave Marchevsky
@ 2021-09-02 17:19 ` Dave Marchevsky
2021-09-02 17:19 ` [PATCH v4 bpf-next 4/9] libbpf: Modify bpf_printk to choose helper based on arg count Dave Marchevsky
` (5 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Dave Marchevsky @ 2021-09-02 17:19 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
This helper is meant to be "bpf_trace_printk, but with proper vararg
support". Follow bpf_snprintf's example and take a u64 pseudo-vararg
array. Write to /sys/kernel/debug/tracing/trace_pipe using the same
mechanism as bpf_trace_printk.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
include/linux/bpf.h | 1 +
include/uapi/linux/bpf.h | 9 ++++++
kernel/bpf/core.c | 5 ++++
kernel/bpf/helpers.c | 2 ++
kernel/trace/bpf_trace.c | 52 +++++++++++++++++++++++++++++++++-
tools/include/uapi/linux/bpf.h | 9 ++++++
6 files changed, 77 insertions(+), 1 deletion(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index be8d57e6e78a..b6c45a6cbbba 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1088,6 +1088,7 @@ bool bpf_prog_array_compatible(struct bpf_array *array, const struct bpf_prog *f
int bpf_prog_calc_tag(struct bpf_prog *fp);
const struct bpf_func_proto *bpf_get_trace_printk_proto(void);
+const struct bpf_func_proto *bpf_get_trace_vprintk_proto(void);
typedef unsigned long (*bpf_ctx_copy_t)(void *dst, const void *src,
unsigned long off, unsigned long len);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 791f31dd0abe..f171d4d33136 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4877,6 +4877,14 @@ union bpf_attr {
* Get the struct pt_regs associated with **task**.
* Return
* A pointer to struct pt_regs.
+ *
+ * u64 bpf_trace_vprintk(const char *fmt, u32 fmt_size, const void *data, u32 data_len)
+ * Description
+ * Behaves like **bpf_trace_printk**\ () helper, but takes an array of u64
+ * to format. Arguments are to be used as in **bpf_seq_printf**\ () helper.
+ * Return
+ * The number of bytes written to the buffer, or a negative error
+ * in case of failure.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -5055,6 +5063,7 @@ union bpf_attr {
FN(get_func_ip), \
FN(get_attach_cookie), \
FN(task_pt_regs), \
+ FN(trace_vprintk), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 9f4636d021b1..6fddc13fe67f 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2357,6 +2357,11 @@ const struct bpf_func_proto * __weak bpf_get_trace_printk_proto(void)
return NULL;
}
+const struct bpf_func_proto * __weak bpf_get_trace_vprintk_proto(void)
+{
+ return NULL;
+}
+
u64 __weak
bpf_event_output(struct bpf_map *map, u64 flags, void *meta, u64 meta_size,
void *ctx, u64 ctx_size, bpf_ctx_copy_t ctx_copy)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 8f9f392c1322..2c604ff8c7fb 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1435,6 +1435,8 @@ bpf_base_func_proto(enum bpf_func_id func_id)
return &bpf_snprintf_proto;
case BPF_FUNC_task_pt_regs:
return &bpf_task_pt_regs_proto;
+ case BPF_FUNC_trace_vprintk:
+ return bpf_get_trace_vprintk_proto();
default:
return NULL;
}
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 10672ebc63b7..ea8358b0c748 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -398,7 +398,7 @@ static const struct bpf_func_proto bpf_trace_printk_proto = {
.arg2_type = ARG_CONST_SIZE,
};
-const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
+static void __set_printk_clr_event(void)
{
/*
* This program might be calling bpf_trace_printk,
@@ -410,10 +410,58 @@ const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
*/
if (trace_set_clr_event("bpf_trace", "bpf_trace_printk", 1))
pr_warn_ratelimited("could not enable bpf_trace_printk events");
+}
+const struct bpf_func_proto *bpf_get_trace_printk_proto(void)
+{
+ __set_printk_clr_event();
return &bpf_trace_printk_proto;
}
+BPF_CALL_4(bpf_trace_vprintk, char *, fmt, u32, fmt_size, const void *, data,
+ u32, data_len)
+{
+ static char buf[BPF_TRACE_PRINTK_SIZE];
+ unsigned long flags;
+ int ret, num_args;
+ u32 *bin_args;
+
+ if (data_len & 7 || data_len > MAX_BPRINTF_VARARGS * 8 ||
+ (data_len && !data))
+ return -EINVAL;
+ num_args = data_len / 8;
+
+ ret = bpf_bprintf_prepare(fmt, fmt_size, data, &bin_args, num_args);
+ if (ret < 0)
+ return ret;
+
+ raw_spin_lock_irqsave(&trace_printk_lock, flags);
+ ret = bstr_printf(buf, sizeof(buf), fmt, bin_args);
+
+ trace_bpf_trace_printk(buf);
+ raw_spin_unlock_irqrestore(&trace_printk_lock, flags);
+
+ bpf_bprintf_cleanup();
+
+ return ret;
+}
+
+static const struct bpf_func_proto bpf_trace_vprintk_proto = {
+ .func = bpf_trace_vprintk,
+ .gpl_only = true,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_MEM,
+ .arg2_type = ARG_CONST_SIZE,
+ .arg3_type = ARG_PTR_TO_MEM_OR_NULL,
+ .arg4_type = ARG_CONST_SIZE_OR_ZERO,
+};
+
+const struct bpf_func_proto *bpf_get_trace_vprintk_proto(void)
+{
+ __set_printk_clr_event();
+ return &bpf_trace_vprintk_proto;
+}
+
BPF_CALL_5(bpf_seq_printf, struct seq_file *, m, char *, fmt, u32, fmt_size,
const void *, data, u32, data_len)
{
@@ -1130,6 +1178,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_snprintf_proto;
case BPF_FUNC_get_func_ip:
return &bpf_get_func_ip_proto_tracing;
+ case BPF_FUNC_trace_vprintk:
+ return bpf_get_trace_vprintk_proto();
default:
return bpf_base_func_proto(func_id);
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 791f31dd0abe..f171d4d33136 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4877,6 +4877,14 @@ union bpf_attr {
* Get the struct pt_regs associated with **task**.
* Return
* A pointer to struct pt_regs.
+ *
+ * u64 bpf_trace_vprintk(const char *fmt, u32 fmt_size, const void *data, u32 data_len)
+ * Description
+ * Behaves like **bpf_trace_printk**\ () helper, but takes an array of u64
+ * to format. Arguments are to be used as in **bpf_seq_printf**\ () helper.
+ * Return
+ * The number of bytes written to the buffer, or a negative error
+ * in case of failure.
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -5055,6 +5063,7 @@ union bpf_attr {
FN(get_func_ip), \
FN(get_attach_cookie), \
FN(task_pt_regs), \
+ FN(trace_vprintk), \
/* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 bpf-next 4/9] libbpf: Modify bpf_printk to choose helper based on arg count
2021-09-02 17:19 [PATCH v4 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (2 preceding siblings ...)
2021-09-02 17:19 ` [PATCH v4 bpf-next 3/9] bpf: add bpf_trace_vprintk helper Dave Marchevsky
@ 2021-09-02 17:19 ` Dave Marchevsky
2021-09-02 23:08 ` Andrii Nakryiko
2021-09-02 17:19 ` [PATCH v4 bpf-next 5/9] libbpf: use static const fmt string in __bpf_printk Dave Marchevsky
` (4 subsequent siblings)
8 siblings, 1 reply; 16+ messages in thread
From: Dave Marchevsky @ 2021-09-02 17:19 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
Instead of being a thin wrapper which calls into bpf_trace_printk,
libbpf's bpf_printk convenience macro now chooses between
bpf_trace_printk and bpf_trace_vprintk. If the arg count (excluding
format string) is >3, use bpf_trace_vprintk, otherwise use the older
helper.
The motivation behind this added complexity - instead of migrating
entirely to bpf_trace_vprintk - is to maintain good developer experience
for users compiling against new libbpf but running on older kernels.
Users who are passing <=3 args to bpf_printk will see no change in their
bytecode.
__bpf_vprintk functions similarly to BPF_SEQ_PRINTF and BPF_SNPRINTF
macros elsewhere in the file - it allows use of bpf_trace_vprintk
without manual conversion of varargs to u64 array. Previous
implementation of bpf_printk macro is moved to __bpf_printk for use by
the new implementation.
This does change behavior of bpf_printk calls with >3 args in the "new
libbpf, old kernels" scenario. Before this patch, attempting to use 4
args to bpf_printk results in a compile-time error. After this patch,
using bpf_printk with 4 args results in a trace_vprintk helper call
being emitted and a load-time failure on older kernels.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
tools/lib/bpf/bpf_helpers.h | 45 ++++++++++++++++++++++++++++++-------
1 file changed, 37 insertions(+), 8 deletions(-)
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index b9987c3efa3c..a7e73be6dac4 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -14,14 +14,6 @@
#define __type(name, val) typeof(val) *name
#define __array(name, val) typeof(val) *name[]
-/* Helper macro to print out debug messages */
-#define bpf_printk(fmt, ...) \
-({ \
- char ____fmt[] = fmt; \
- bpf_trace_printk(____fmt, sizeof(____fmt), \
- ##__VA_ARGS__); \
-})
-
/*
* Helper macro to place programs, maps, license in
* different sections in elf_bpf file. Section names
@@ -224,4 +216,41 @@ enum libbpf_tristate {
___param, sizeof(___param)); \
})
+/* Helper macro to print out debug messages */
+#define __bpf_printk(fmt, ...) \
+({ \
+ char ____fmt[] = fmt; \
+ bpf_trace_printk(____fmt, sizeof(____fmt), \
+ ##__VA_ARGS__); \
+})
+
+/*
+ * __bpf_vprintk wraps the bpf_trace_vprintk helper with variadic arguments
+ * instead of an array of u64.
+ */
+#define __bpf_vprintk(fmt, args...) \
+({ \
+ static const char ___fmt[] = fmt; \
+ unsigned long long ___param[___bpf_narg(args)]; \
+ \
+ _Pragma("GCC diagnostic push") \
+ _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
+ ___bpf_fill(___param, args); \
+ _Pragma("GCC diagnostic pop") \
+ \
+ bpf_trace_vprintk(___fmt, sizeof(___fmt), \
+ ___param, sizeof(___param)); \
+})
+
+/* Use __bpf_printk when bpf_printk call has 3 or fewer fmt args
+ * Otherwise use __bpf_vprintk
+ */
+#define ___bpf_pick_printk(...) \
+ ___bpf_nth(_, ##__VA_ARGS__, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \
+ __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \
+ __bpf_vprintk, __bpf_vprintk, __bpf_printk /*3*/, __bpf_printk /*2*/,\
+ __bpf_printk /*1*/, __bpf_printk /*0*/)
+
+#define bpf_printk(fmt, args...) ___bpf_pick_printk(args)(fmt, ##args)
+
#endif
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 bpf-next 4/9] libbpf: Modify bpf_printk to choose helper based on arg count
2021-09-02 17:19 ` [PATCH v4 bpf-next 4/9] libbpf: Modify bpf_printk to choose helper based on arg count Dave Marchevsky
@ 2021-09-02 23:08 ` Andrii Nakryiko
0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2021-09-02 23:08 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, Networking
On Thu, Sep 2, 2021 at 10:23 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> Instead of being a thin wrapper which calls into bpf_trace_printk,
> libbpf's bpf_printk convenience macro now chooses between
> bpf_trace_printk and bpf_trace_vprintk. If the arg count (excluding
> format string) is >3, use bpf_trace_vprintk, otherwise use the older
> helper.
>
> The motivation behind this added complexity - instead of migrating
> entirely to bpf_trace_vprintk - is to maintain good developer experience
> for users compiling against new libbpf but running on older kernels.
> Users who are passing <=3 args to bpf_printk will see no change in their
> bytecode.
>
> __bpf_vprintk functions similarly to BPF_SEQ_PRINTF and BPF_SNPRINTF
> macros elsewhere in the file - it allows use of bpf_trace_vprintk
> without manual conversion of varargs to u64 array. Previous
> implementation of bpf_printk macro is moved to __bpf_printk for use by
> the new implementation.
>
> This does change behavior of bpf_printk calls with >3 args in the "new
> libbpf, old kernels" scenario. Before this patch, attempting to use 4
> args to bpf_printk results in a compile-time error. After this patch,
> using bpf_printk with 4 args results in a trace_vprintk helper call
> being emitted and a load-time failure on older kernels.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
Minor nit below, otherwise looks good:
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> tools/lib/bpf/bpf_helpers.h | 45 ++++++++++++++++++++++++++++++-------
> 1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index b9987c3efa3c..a7e73be6dac4 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -14,14 +14,6 @@
> #define __type(name, val) typeof(val) *name
> #define __array(name, val) typeof(val) *name[]
>
> -/* Helper macro to print out debug messages */
> -#define bpf_printk(fmt, ...) \
> -({ \
> - char ____fmt[] = fmt; \
> - bpf_trace_printk(____fmt, sizeof(____fmt), \
> - ##__VA_ARGS__); \
> -})
> -
> /*
> * Helper macro to place programs, maps, license in
> * different sections in elf_bpf file. Section names
> @@ -224,4 +216,41 @@ enum libbpf_tristate {
> ___param, sizeof(___param)); \
> })
>
> +/* Helper macro to print out debug messages */
this comment probably should have been moved into the new bpf_printk() macro...
> +#define __bpf_printk(fmt, ...) \
> +({ \
> + char ____fmt[] = fmt; \
> + bpf_trace_printk(____fmt, sizeof(____fmt), \
> + ##__VA_ARGS__); \
> +})
> +
> +/*
> + * __bpf_vprintk wraps the bpf_trace_vprintk helper with variadic arguments
> + * instead of an array of u64.
> + */
> +#define __bpf_vprintk(fmt, args...) \
> +({ \
> + static const char ___fmt[] = fmt; \
> + unsigned long long ___param[___bpf_narg(args)]; \
> + \
> + _Pragma("GCC diagnostic push") \
> + _Pragma("GCC diagnostic ignored \"-Wint-conversion\"") \
> + ___bpf_fill(___param, args); \
> + _Pragma("GCC diagnostic pop") \
> + \
> + bpf_trace_vprintk(___fmt, sizeof(___fmt), \
> + ___param, sizeof(___param)); \
> +})
> +
> +/* Use __bpf_printk when bpf_printk call has 3 or fewer fmt args
> + * Otherwise use __bpf_vprintk
> + */
> +#define ___bpf_pick_printk(...) \
> + ___bpf_nth(_, ##__VA_ARGS__, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \
> + __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, __bpf_vprintk, \
> + __bpf_vprintk, __bpf_vprintk, __bpf_printk /*3*/, __bpf_printk /*2*/,\
> + __bpf_printk /*1*/, __bpf_printk /*0*/)
> +
> +#define bpf_printk(fmt, args...) ___bpf_pick_printk(args)(fmt, ##args)
> +
> #endif
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 bpf-next 5/9] libbpf: use static const fmt string in __bpf_printk
2021-09-02 17:19 [PATCH v4 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (3 preceding siblings ...)
2021-09-02 17:19 ` [PATCH v4 bpf-next 4/9] libbpf: Modify bpf_printk to choose helper based on arg count Dave Marchevsky
@ 2021-09-02 17:19 ` Dave Marchevsky
2021-09-02 23:09 ` Andrii Nakryiko
2021-09-02 17:19 ` [PATCH v4 bpf-next 6/9] bpftool: only probe trace_vprintk feature in 'full' mode Dave Marchevsky
` (3 subsequent siblings)
8 siblings, 1 reply; 16+ messages in thread
From: Dave Marchevsky @ 2021-09-02 17:19 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
The __bpf_printk convenience macro was using a 'char' fmt string holder
as it predates support for globals in libbpf. Move to more efficient
'static const char', but provide a fallback to the old way via
BPF_NO_GLOBAL_DATA so users on old kernels can still use the macro.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
tools/lib/bpf/bpf_helpers.h | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index a7e73be6dac4..7df7d9a23099 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -216,10 +216,16 @@ enum libbpf_tristate {
___param, sizeof(___param)); \
})
+#ifdef BPF_NO_GLOBAL_DATA
+#define BPF_PRINTK_FMT_MOD
+#else
+#define BPF_PRINTK_FMT_MOD static const
+#endif
+
/* Helper macro to print out debug messages */
#define __bpf_printk(fmt, ...) \
({ \
- char ____fmt[] = fmt; \
+ BPF_PRINTK_FMT_MOD char ____fmt[] = fmt; \
bpf_trace_printk(____fmt, sizeof(____fmt), \
##__VA_ARGS__); \
})
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 bpf-next 5/9] libbpf: use static const fmt string in __bpf_printk
2021-09-02 17:19 ` [PATCH v4 bpf-next 5/9] libbpf: use static const fmt string in __bpf_printk Dave Marchevsky
@ 2021-09-02 23:09 ` Andrii Nakryiko
0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2021-09-02 23:09 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, Networking
On Thu, Sep 2, 2021 at 10:23 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> The __bpf_printk convenience macro was using a 'char' fmt string holder
> as it predates support for globals in libbpf. Move to more efficient
> 'static const char', but provide a fallback to the old way via
> BPF_NO_GLOBAL_DATA so users on old kernels can still use the macro.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
Great, thanks!
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> tools/lib/bpf/bpf_helpers.h | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index a7e73be6dac4..7df7d9a23099 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -216,10 +216,16 @@ enum libbpf_tristate {
> ___param, sizeof(___param)); \
> })
>
> +#ifdef BPF_NO_GLOBAL_DATA
> +#define BPF_PRINTK_FMT_MOD
> +#else
> +#define BPF_PRINTK_FMT_MOD static const
> +#endif
> +
> /* Helper macro to print out debug messages */
> #define __bpf_printk(fmt, ...) \
> ({ \
> - char ____fmt[] = fmt; \
> + BPF_PRINTK_FMT_MOD char ____fmt[] = fmt; \
> bpf_trace_printk(____fmt, sizeof(____fmt), \
> ##__VA_ARGS__); \
> })
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 bpf-next 6/9] bpftool: only probe trace_vprintk feature in 'full' mode
2021-09-02 17:19 [PATCH v4 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (4 preceding siblings ...)
2021-09-02 17:19 ` [PATCH v4 bpf-next 5/9] libbpf: use static const fmt string in __bpf_printk Dave Marchevsky
@ 2021-09-02 17:19 ` Dave Marchevsky
2021-09-02 23:10 ` Andrii Nakryiko
2021-09-02 17:19 ` [PATCH v4 bpf-next 7/9] selftests/bpf: Migrate prog_tests/trace_printk CHECKs to ASSERTs Dave Marchevsky
` (2 subsequent siblings)
8 siblings, 1 reply; 16+ messages in thread
From: Dave Marchevsky @ 2021-09-02 17:19 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
Since commit 368cb0e7cdb5e ("bpftool: Make probes which emit dmesg
warnings optional"), some helpers aren't probed by bpftool unless
`full` arg is added to `bpftool feature probe`.
bpf_trace_vprintk can emit dmesg warnings when probed, so include it.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
tools/bpf/bpftool/feature.c | 1 +
tools/testing/selftests/bpf/test_bpftool.py | 22 +++++++++------------
2 files changed, 10 insertions(+), 13 deletions(-)
diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
index 7f36385aa9e2..ade44577688e 100644
--- a/tools/bpf/bpftool/feature.c
+++ b/tools/bpf/bpftool/feature.c
@@ -624,6 +624,7 @@ probe_helpers_for_progtype(enum bpf_prog_type prog_type, bool supported_type,
*/
switch (id) {
case BPF_FUNC_trace_printk:
+ case BPF_FUNC_trace_vprintk:
case BPF_FUNC_probe_write_user:
if (!full_mode)
continue;
diff --git a/tools/testing/selftests/bpf/test_bpftool.py b/tools/testing/selftests/bpf/test_bpftool.py
index 4fed2dc25c0a..1c2408ee1f5d 100644
--- a/tools/testing/selftests/bpf/test_bpftool.py
+++ b/tools/testing/selftests/bpf/test_bpftool.py
@@ -57,6 +57,11 @@ def default_iface(f):
return f(*args, iface, **kwargs)
return wrapper
+DMESG_EMITTING_HELPERS = [
+ "bpf_probe_write_user",
+ "bpf_trace_printk",
+ "bpf_trace_vprintk",
+ ]
class TestBpftool(unittest.TestCase):
@classmethod
@@ -67,10 +72,7 @@ class TestBpftool(unittest.TestCase):
@default_iface
def test_feature_dev_json(self, iface):
- unexpected_helpers = [
- "bpf_probe_write_user",
- "bpf_trace_printk",
- ]
+ unexpected_helpers = DMESG_EMITTING_HELPERS
expected_keys = [
"syscall_config",
"program_types",
@@ -94,10 +96,7 @@ class TestBpftool(unittest.TestCase):
bpftool_json(["feature", "probe"]),
bpftool_json(["feature"]),
]
- unexpected_helpers = [
- "bpf_probe_write_user",
- "bpf_trace_printk",
- ]
+ unexpected_helpers = DMESG_EMITTING_HELPERS
expected_keys = [
"syscall_config",
"system_config",
@@ -121,10 +120,7 @@ class TestBpftool(unittest.TestCase):
bpftool_json(["feature", "probe", "kernel", "full"]),
bpftool_json(["feature", "probe", "full"]),
]
- expected_helpers = [
- "bpf_probe_write_user",
- "bpf_trace_printk",
- ]
+ expected_helpers = DMESG_EMITTING_HELPERS
for tc in test_cases:
# Check if expected helpers are included at least once in any
@@ -157,7 +153,7 @@ class TestBpftool(unittest.TestCase):
not_full_set.add(helper)
self.assertCountEqual(full_set - not_full_set,
- {"bpf_probe_write_user", "bpf_trace_printk"})
+ set(DMESG_EMITTING_HELPERS))
self.assertCountEqual(not_full_set - full_set, set())
def test_feature_macros(self):
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 bpf-next 6/9] bpftool: only probe trace_vprintk feature in 'full' mode
2021-09-02 17:19 ` [PATCH v4 bpf-next 6/9] bpftool: only probe trace_vprintk feature in 'full' mode Dave Marchevsky
@ 2021-09-02 23:10 ` Andrii Nakryiko
0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2021-09-02 23:10 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, Networking
On Thu, Sep 2, 2021 at 10:20 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> Since commit 368cb0e7cdb5e ("bpftool: Make probes which emit dmesg
> warnings optional"), some helpers aren't probed by bpftool unless
> `full` arg is added to `bpftool feature probe`.
>
> bpf_trace_vprintk can emit dmesg warnings when probed, so include it.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> tools/bpf/bpftool/feature.c | 1 +
> tools/testing/selftests/bpf/test_bpftool.py | 22 +++++++++------------
> 2 files changed, 10 insertions(+), 13 deletions(-)
>
[...]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 bpf-next 7/9] selftests/bpf: Migrate prog_tests/trace_printk CHECKs to ASSERTs
2021-09-02 17:19 [PATCH v4 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (5 preceding siblings ...)
2021-09-02 17:19 ` [PATCH v4 bpf-next 6/9] bpftool: only probe trace_vprintk feature in 'full' mode Dave Marchevsky
@ 2021-09-02 17:19 ` Dave Marchevsky
2021-09-02 17:19 ` [PATCH v4 bpf-next 8/9] selftests/bpf: add trace_vprintk test prog Dave Marchevsky
2021-09-02 17:19 ` [PATCH v4 bpf-next 9/9] selftests/bpf: Add test for bpf_printk w/ 0 fmt args Dave Marchevsky
8 siblings, 0 replies; 16+ messages in thread
From: Dave Marchevsky @ 2021-09-02 17:19 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
Guidance for new tests is to use ASSERT macros instead of CHECK. Since
trace_vprintk test will borrow heavily from trace_printk's, migrate its
CHECKs so it remains obvious that the two are closely related.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
.../selftests/bpf/prog_tests/trace_printk.c | 24 +++++++------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/trace_printk.c b/tools/testing/selftests/bpf/prog_tests/trace_printk.c
index d39bc00feb45..e47835f0a674 100644
--- a/tools/testing/selftests/bpf/prog_tests/trace_printk.c
+++ b/tools/testing/selftests/bpf/prog_tests/trace_printk.c
@@ -10,7 +10,7 @@
void test_trace_printk(void)
{
- int err, iter = 0, duration = 0, found = 0;
+ int err = 0, iter = 0, found = 0;
struct trace_printk__bss *bss;
struct trace_printk *skel;
char *buf = NULL;
@@ -18,25 +18,24 @@ void test_trace_printk(void)
size_t buflen;
skel = trace_printk__open();
- if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
+ if (!ASSERT_OK_PTR(skel, "trace_printk__open"))
return;
- ASSERT_EQ(skel->rodata->fmt[0], 'T', "invalid printk fmt string");
+ ASSERT_EQ(skel->rodata->fmt[0], 'T', "skel->rodata->fmt[0]");
skel->rodata->fmt[0] = 't';
err = trace_printk__load(skel);
- if (CHECK(err, "skel_load", "failed to load skeleton: %d\n", err))
+ if (!ASSERT_OK(err, "trace_printk__load"))
goto cleanup;
bss = skel->bss;
err = trace_printk__attach(skel);
- if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
+ if (!ASSERT_OK(err, "trace_printk__attach"))
goto cleanup;
fp = fopen(TRACEBUF, "r");
- if (CHECK(fp == NULL, "could not open trace buffer",
- "error %d opening %s", errno, TRACEBUF))
+ if (!ASSERT_OK_PTR(fp, "fopen(TRACEBUF)"))
goto cleanup;
/* We do not want to wait forever if this test fails... */
@@ -46,14 +45,10 @@ void test_trace_printk(void)
usleep(1);
trace_printk__detach(skel);
- if (CHECK(bss->trace_printk_ran == 0,
- "bpf_trace_printk never ran",
- "ran == %d", bss->trace_printk_ran))
+ if (!ASSERT_GT(bss->trace_printk_ran, 0, "bss->trace_printk_ran"))
goto cleanup;
- if (CHECK(bss->trace_printk_ret <= 0,
- "bpf_trace_printk returned <= 0 value",
- "got %d", bss->trace_printk_ret))
+ if (!ASSERT_GT(bss->trace_printk_ret, 0, "bss->trace_printk_ret"))
goto cleanup;
/* verify our search string is in the trace buffer */
@@ -66,8 +61,7 @@ void test_trace_printk(void)
break;
}
- if (CHECK(!found, "message from bpf_trace_printk not found",
- "no instance of %s in %s", SEARCHMSG, TRACEBUF))
+ if (!ASSERT_EQ(found, bss->trace_printk_ran, "found"))
goto cleanup;
cleanup:
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 bpf-next 8/9] selftests/bpf: add trace_vprintk test prog
2021-09-02 17:19 [PATCH v4 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (6 preceding siblings ...)
2021-09-02 17:19 ` [PATCH v4 bpf-next 7/9] selftests/bpf: Migrate prog_tests/trace_printk CHECKs to ASSERTs Dave Marchevsky
@ 2021-09-02 17:19 ` Dave Marchevsky
2021-09-02 23:13 ` Andrii Nakryiko
2021-09-02 17:19 ` [PATCH v4 bpf-next 9/9] selftests/bpf: Add test for bpf_printk w/ 0 fmt args Dave Marchevsky
8 siblings, 1 reply; 16+ messages in thread
From: Dave Marchevsky @ 2021-09-02 17:19 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
This commit adds a test prog for vprintk which confirms that:
* bpf_trace_vprintk is writing to dmesg
* __bpf_vprintk macro works as expected
* >3 args are printed
Approach and code are borrowed from trace_printk test.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
---
tools/testing/selftests/bpf/Makefile | 3 +-
.../selftests/bpf/prog_tests/trace_vprintk.c | 65 +++++++++++++++++++
.../selftests/bpf/progs/trace_vprintk.c | 25 +++++++
3 files changed, 92 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
create mode 100644 tools/testing/selftests/bpf/progs/trace_vprintk.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 866531c08e4f..0a4cdac7ea02 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -315,7 +315,8 @@ LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h \
linked_vars.skel.h linked_maps.skel.h
LSKELS := kfunc_call_test.c fentry_test.c fexit_test.c fexit_sleep.c \
- test_ksyms_module.c test_ringbuf.c atomics.c trace_printk.c
+ test_ksyms_module.c test_ringbuf.c atomics.c trace_printk.c \
+ trace_vprintk.c
SKEL_BLACKLIST += $$(LSKELS)
test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o
diff --git a/tools/testing/selftests/bpf/prog_tests/trace_vprintk.c b/tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
new file mode 100644
index 000000000000..9fc1d279b673
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+
+#include <test_progs.h>
+
+#include "trace_vprintk.lskel.h"
+
+#define TRACEBUF "/sys/kernel/debug/tracing/trace_pipe"
+#define SEARCHMSG "1,2,3,4,5,6,7,8,9,10"
+
+void test_trace_vprintk(void)
+{
+ int err = 0, iter = 0, found = 0;
+ struct trace_vprintk__bss *bss;
+ struct trace_vprintk *skel;
+ char *buf = NULL;
+ FILE *fp = NULL;
+ size_t buflen;
+
+ skel = trace_vprintk__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "trace_vprintk__open_and_load"))
+ goto cleanup;
+
+ bss = skel->bss;
+
+ err = trace_vprintk__attach(skel);
+ if (!ASSERT_OK(err, "trace_vprintk__attach"))
+ goto cleanup;
+
+ fp = fopen(TRACEBUF, "r");
+ if (!ASSERT_OK_PTR(fp, "fopen(TRACEBUF)"))
+ goto cleanup;
+
+ /* We do not want to wait forever if this test fails... */
+ fcntl(fileno(fp), F_SETFL, O_NONBLOCK);
+
+ /* wait for tracepoint to trigger */
+ usleep(1);
+ trace_vprintk__detach(skel);
+
+ if (!ASSERT_GT(bss->trace_vprintk_ran, 0, "bss->trace_vprintk_ran"))
+ goto cleanup;
+
+ if (!ASSERT_GT(bss->trace_vprintk_ret, 0, "bss->trace_vprintk_ret"))
+ goto cleanup;
+
+ /* verify our search string is in the trace buffer */
+ while (getline(&buf, &buflen, fp) >= 0 || errno == EAGAIN) {
+ if (strstr(buf, SEARCHMSG) != NULL)
+ found++;
+ if (found == bss->trace_vprintk_ran)
+ break;
+ if (++iter > 1000)
+ break;
+ }
+
+ if (!ASSERT_EQ(found, bss->trace_vprintk_ran, "found"))
+ goto cleanup;
+
+cleanup:
+ trace_vprintk__destroy(skel);
+ free(buf);
+ if (fp)
+ fclose(fp);
+}
diff --git a/tools/testing/selftests/bpf/progs/trace_vprintk.c b/tools/testing/selftests/bpf/progs/trace_vprintk.c
new file mode 100644
index 000000000000..255e2f018efe
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/trace_vprintk.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021 Facebook */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+int trace_vprintk_ret = 0;
+int trace_vprintk_ran = 0;
+
+SEC("fentry/__x64_sys_nanosleep")
+int sys_enter(void *ctx)
+{
+ static const char one[] = "1";
+ static const char three[] = "3";
+ static const char five[] = "5";
+ static const char seven[] = "7";
+ static const char nine[] = "9";
+
+ trace_vprintk_ret = __bpf_vprintk("%s,%d,%s,%d,%s,%d,%s,%d,%s,%d %d\n",
+ one, 2, three, 4, five, 6, seven, 8, nine, 10, ++trace_vprintk_ran);
+ return 0;
+}
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 bpf-next 8/9] selftests/bpf: add trace_vprintk test prog
2021-09-02 17:19 ` [PATCH v4 bpf-next 8/9] selftests/bpf: add trace_vprintk test prog Dave Marchevsky
@ 2021-09-02 23:13 ` Andrii Nakryiko
0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2021-09-02 23:13 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, Networking
On Thu, Sep 2, 2021 at 10:23 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> This commit adds a test prog for vprintk which confirms that:
> * bpf_trace_vprintk is writing to dmesg
But it doesn't write to dmesg, does it? It's writing to
/sys/kernel/debug/tracing/trace_pipe, which is a different thing
entirely, right?
> * __bpf_vprintk macro works as expected
> * >3 args are printed
>
> Approach and code are borrowed from trace_printk test.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> ---
> tools/testing/selftests/bpf/Makefile | 3 +-
> .../selftests/bpf/prog_tests/trace_vprintk.c | 65 +++++++++++++++++++
> .../selftests/bpf/progs/trace_vprintk.c | 25 +++++++
> 3 files changed, 92 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_vprintk.c
> create mode 100644 tools/testing/selftests/bpf/progs/trace_vprintk.c
>
[...]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 bpf-next 9/9] selftests/bpf: Add test for bpf_printk w/ 0 fmt args
2021-09-02 17:19 [PATCH v4 bpf-next 0/9] bpf: implement variadic printk helper Dave Marchevsky
` (7 preceding siblings ...)
2021-09-02 17:19 ` [PATCH v4 bpf-next 8/9] selftests/bpf: add trace_vprintk test prog Dave Marchevsky
@ 2021-09-02 17:19 ` Dave Marchevsky
2021-09-02 23:14 ` Andrii Nakryiko
8 siblings, 1 reply; 16+ messages in thread
From: Dave Marchevsky @ 2021-09-02 17:19 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, netdev, Dave Marchevsky
This corner case isn't covered by existing selftests' use of bpf_printk.
Just test compilation, not output, as trace_vprintk already tests
trace_pipe output.
Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
tools/testing/selftests/bpf/progs/trace_vprintk.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/trace_vprintk.c b/tools/testing/selftests/bpf/progs/trace_vprintk.c
index 255e2f018efe..33455e48a9ab 100644
--- a/tools/testing/selftests/bpf/progs/trace_vprintk.c
+++ b/tools/testing/selftests/bpf/progs/trace_vprintk.c
@@ -23,3 +23,10 @@ int sys_enter(void *ctx)
one, 2, three, 4, five, 6, seven, 8, nine, 10, ++trace_vprintk_ran);
return 0;
}
+
+SEC("fentry/__x64_sys_nanosleep")
+int zero_fmt_args(void *ctx)
+{
+ bpf_printk("\t"); // runner doesn't search for this, just ensure it compiles
+ return 0;
+}
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 bpf-next 9/9] selftests/bpf: Add test for bpf_printk w/ 0 fmt args
2021-09-02 17:19 ` [PATCH v4 bpf-next 9/9] selftests/bpf: Add test for bpf_printk w/ 0 fmt args Dave Marchevsky
@ 2021-09-02 23:14 ` Andrii Nakryiko
0 siblings, 0 replies; 16+ messages in thread
From: Andrii Nakryiko @ 2021-09-02 23:14 UTC (permalink / raw)
To: Dave Marchevsky
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Yonghong Song, Networking
On Thu, Sep 2, 2021 at 10:23 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> This corner case isn't covered by existing selftests' use of bpf_printk.
>
> Just test compilation, not output, as trace_vprintk already tests
> trace_pipe output.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
> tools/testing/selftests/bpf/progs/trace_vprintk.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/trace_vprintk.c b/tools/testing/selftests/bpf/progs/trace_vprintk.c
> index 255e2f018efe..33455e48a9ab 100644
> --- a/tools/testing/selftests/bpf/progs/trace_vprintk.c
> +++ b/tools/testing/selftests/bpf/progs/trace_vprintk.c
> @@ -23,3 +23,10 @@ int sys_enter(void *ctx)
> one, 2, three, 4, five, 6, seven, 8, nine, 10, ++trace_vprintk_ran);
> return 0;
> }
> +
> +SEC("fentry/__x64_sys_nanosleep")
> +int zero_fmt_args(void *ctx)
> +{
> + bpf_printk("\t"); // runner doesn't search for this, just ensure it compiles
C++ comments :( please use /* */
I'd probably just add this bpf_printk() in the same BPF program above
and roll it into previous patch. Doesn't seem like we need dedicated
BPF program just for this.
> + return 0;
> +}
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 16+ messages in thread