| Message ID | 20241021141616.95160-1-mykyta.yatsenko5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests/bpf: increase verifier log limit in veristat | expand |
On Mon, Oct 21, 2024 at 03:16:16PM +0100, Mykyta Yatsenko wrote: > From: Mykyta Yatsenko <yatsenko@meta.com> > > The current default buffer size of 16MB allocated by veristat is no > longer sufficient to hold the verifier logs of some production BPF > programs. To address this issue, we need to increase the verifier log > limit. > Commit 7a9f5c65abcc ("bpf: increase verifier log limit") has already > increased the supported buffer size by the kernel, but veristat users > need to explicitly pass a log size argument to use the bigger log. > > This patch adds a function to detect the maximum verifier log size > supported by the kernel and uses that by default in veristat. > This ensures that veristat can handle larger verifier logs without > requiring users to manually specify the log size. > > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> > --- > tools/testing/selftests/bpf/veristat.c | 40 +++++++++++++++++++++++++- > 1 file changed, 39 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c > index c8efd44590d9..1d0708839f4b 100644 > --- a/tools/testing/selftests/bpf/veristat.c > +++ b/tools/testing/selftests/bpf/veristat.c > @@ -16,10 +16,12 @@ > #include <sys/stat.h> > #include <bpf/libbpf.h> > #include <bpf/btf.h> > +#include <bpf/bpf.h> > #include <libelf.h> > #include <gelf.h> > #include <float.h> > #include <math.h> > +#include <linux/filter.h> > > #ifndef ARRAY_SIZE > #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) > @@ -1109,6 +1111,42 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch > return; > } > > +static int max_verifier_log_size(void) > +{ > + const int big_log_size = UINT_MAX >> 2; > + const int small_log_size = UINT_MAX >> 8; > + struct bpf_insn insns[] = { > + BPF_MOV64_IMM(BPF_REG_0, 0), > + BPF_EXIT_INSN(), > + }; > + int ret, insn_cnt = ARRAY_SIZE(insns); > + char *log_buf; > + static int log_size; > + > + if (log_size != 0) > + return log_size; > + > + log_size = small_log_size; > + log_buf = malloc(big_log_size); IIUC this would try to use 1GB by default? seems to agresive.. could we perhaps do that gradually and double the size on each failed load attempt? jirka > + > + if (!log_buf) > + return log_size; > + > + LIBBPF_OPTS(bpf_prog_load_opts, opts, > + .log_buf = log_buf, > + .log_size = big_log_size, > + .log_level = 2 > + ); > + ret = bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, NULL, "GPL", insns, insn_cnt, &opts); > + free(log_buf); > + > + if (ret > 0) { > + log_size = big_log_size; > + close(ret); > + } > + return log_size; > +} > + > static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog) > { > const char *base_filename = basename(strdupa(filename)); > @@ -1132,7 +1170,7 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf > memset(stats, 0, sizeof(*stats)); > > if (env.verbose || env.top_src_lines > 0) { > - buf_sz = env.log_size ? env.log_size : 16 * 1024 * 1024; > + buf_sz = env.log_size ? env.log_size : max_verifier_log_size(); > buf = malloc(buf_sz); > if (!buf) > return -ENOMEM; > -- > 2.47.0 > >
On Mon, Oct 21, 2024 at 9:44 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Mon, Oct 21, 2024 at 03:16:16PM +0100, Mykyta Yatsenko wrote: > > From: Mykyta Yatsenko <yatsenko@meta.com> > > > > The current default buffer size of 16MB allocated by veristat is no > > longer sufficient to hold the verifier logs of some production BPF > > programs. To address this issue, we need to increase the verifier log > > limit. > > Commit 7a9f5c65abcc ("bpf: increase verifier log limit") has already > > increased the supported buffer size by the kernel, but veristat users > > need to explicitly pass a log size argument to use the bigger log. > > > > This patch adds a function to detect the maximum verifier log size > > supported by the kernel and uses that by default in veristat. > > This ensures that veristat can handle larger verifier logs without > > requiring users to manually specify the log size. > > > > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> > > --- > > tools/testing/selftests/bpf/veristat.c | 40 +++++++++++++++++++++++++- > > 1 file changed, 39 insertions(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c > > index c8efd44590d9..1d0708839f4b 100644 > > --- a/tools/testing/selftests/bpf/veristat.c > > +++ b/tools/testing/selftests/bpf/veristat.c > > @@ -16,10 +16,12 @@ > > #include <sys/stat.h> > > #include <bpf/libbpf.h> > > #include <bpf/btf.h> > > +#include <bpf/bpf.h> > > #include <libelf.h> > > #include <gelf.h> > > #include <float.h> > > #include <math.h> > > +#include <linux/filter.h> this is kernel-internal header, which will be a problem for Github mirror, so... > > > > #ifndef ARRAY_SIZE > > #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) > > @@ -1109,6 +1111,42 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch > > return; > > } > > > > +static int max_verifier_log_size(void) > > +{ > > + const int big_log_size = UINT_MAX >> 2; > > + const int small_log_size = UINT_MAX >> 8; nit: MAKE_ALL_CAPS, given they are fixed constants > > + struct bpf_insn insns[] = { > > + BPF_MOV64_IMM(BPF_REG_0, 0), > > + BPF_EXIT_INSN(), ... let's instead either define these macro locally or just hard-code bpf_insn structs as is (thankfully we need just two) > > + }; > > + int ret, insn_cnt = ARRAY_SIZE(insns); > > + char *log_buf; > > + static int log_size; > > + > > + if (log_size != 0) > > + return log_size; > > + > > + log_size = small_log_size; > > + log_buf = malloc(big_log_size); we don't really need to allocate anything. We can pass (void*)-1 as log_buf (invalid pointer), set size to UINT_MAX >> 8, log_level = 4. If the kernel doesn't support big log_size, we'll get -EINVAL. If it does, we'll get -EFAULT when the verifier will try to write something to the buffer. No allocation. pw-bot: cr > > IIUC this would try to use 1GB by default? seems to agresive.. could we perhaps > do that gradually and double the size on each failed load attempt? The idea is that verifier will only page in as many pages as there is an actual log content (which normally would be much smaller than a full 1GB). Doing gradual size increase is actually pretty annoying in terms of how the code and logic is structured. So I think this approach is fine, overall. > > jirka > > > > + > > + if (!log_buf) > > + return log_size; > > + > > + LIBBPF_OPTS(bpf_prog_load_opts, opts, > > + .log_buf = log_buf, > > + .log_size = big_log_size, > > + .log_level = 2 no need for log_level = 2, just use 4, we don't need to fill out the buffer, we need a verifier to check parameters. > > + ); LIBBPF_OPTS() macro define a variable, so please move it to the variable declaration block above. > > + ret = bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, NULL, "GPL", insns, insn_cnt, &opts); nit: let's use TRACEPOINT instead, we had some problems with SOCKET_FILTER on some old Red Hat distro due to how they did selective backport, so best to avoid it, if possible. > > + free(log_buf); > > + > > + if (ret > 0) { > > + log_size = big_log_size; > > + close(ret); > > + } > > + return log_size; > > +} > > + > > static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog) > > { > > const char *base_filename = basename(strdupa(filename)); > > @@ -1132,7 +1170,7 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf > > memset(stats, 0, sizeof(*stats)); > > > > if (env.verbose || env.top_src_lines > 0) { > > - buf_sz = env.log_size ? env.log_size : 16 * 1024 * 1024; > > + buf_sz = env.log_size ? env.log_size : max_verifier_log_size(); > > buf = malloc(buf_sz); > > if (!buf) > > return -ENOMEM; > > -- > > 2.47.0 > > > >
On 21/10/2024 17:44, Jiri Olsa wrote: > On Mon, Oct 21, 2024 at 03:16:16PM +0100, Mykyta Yatsenko wrote: >> From: Mykyta Yatsenko <yatsenko@meta.com> >> >> The current default buffer size of 16MB allocated by veristat is no >> longer sufficient to hold the verifier logs of some production BPF >> programs. To address this issue, we need to increase the verifier log >> limit. >> Commit 7a9f5c65abcc ("bpf: increase verifier log limit") has already >> increased the supported buffer size by the kernel, but veristat users >> need to explicitly pass a log size argument to use the bigger log. >> >> This patch adds a function to detect the maximum verifier log size >> supported by the kernel and uses that by default in veristat. >> This ensures that veristat can handle larger verifier logs without >> requiring users to manually specify the log size. >> >> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> >> --- >> tools/testing/selftests/bpf/veristat.c | 40 +++++++++++++++++++++++++- >> 1 file changed, 39 insertions(+), 1 deletion(-) >> >> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c >> index c8efd44590d9..1d0708839f4b 100644 >> --- a/tools/testing/selftests/bpf/veristat.c >> +++ b/tools/testing/selftests/bpf/veristat.c >> @@ -16,10 +16,12 @@ >> #include <sys/stat.h> >> #include <bpf/libbpf.h> >> #include <bpf/btf.h> >> +#include <bpf/bpf.h> >> #include <libelf.h> >> #include <gelf.h> >> #include <float.h> >> #include <math.h> >> +#include <linux/filter.h> >> >> #ifndef ARRAY_SIZE >> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) >> @@ -1109,6 +1111,42 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch >> return; >> } >> >> +static int max_verifier_log_size(void) >> +{ >> + const int big_log_size = UINT_MAX >> 2; >> + const int small_log_size = UINT_MAX >> 8; >> + struct bpf_insn insns[] = { >> + BPF_MOV64_IMM(BPF_REG_0, 0), >> + BPF_EXIT_INSN(), >> + }; >> + int ret, insn_cnt = ARRAY_SIZE(insns); >> + char *log_buf; >> + static int log_size; >> + >> + if (log_size != 0) >> + return log_size; >> + >> + log_size = small_log_size; >> + log_buf = malloc(big_log_size); > IIUC this would try to use 1GB by default? seems to agresive.. could we perhaps > do that gradually and double the size on each failed load attempt? > > jirka Yes, this allocates 1GB by default, I expect this is not a big of a problem if verifier does not touch all that memory. I tried doing gradual allocation initially, but that requires more significant rework of the code. Thanks for looking into this patch! > >> + >> + if (!log_buf) >> + return log_size; >> + >> + LIBBPF_OPTS(bpf_prog_load_opts, opts, >> + .log_buf = log_buf, >> + .log_size = big_log_size, >> + .log_level = 2 >> + ); >> + ret = bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, NULL, "GPL", insns, insn_cnt, &opts); >> + free(log_buf); >> + >> + if (ret > 0) { >> + log_size = big_log_size; >> + close(ret); >> + } >> + return log_size; >> +} >> + >> static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog) >> { >> const char *base_filename = basename(strdupa(filename)); >> @@ -1132,7 +1170,7 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf >> memset(stats, 0, sizeof(*stats)); >> >> if (env.verbose || env.top_src_lines > 0) { >> - buf_sz = env.log_size ? env.log_size : 16 * 1024 * 1024; >> + buf_sz = env.log_size ? env.log_size : max_verifier_log_size(); >> buf = malloc(buf_sz); >> if (!buf) >> return -ENOMEM; >> -- >> 2.47.0 >> >>
On 21/10/2024 21:14, Andrii Nakryiko wrote: > On Mon, Oct 21, 2024 at 9:44 AM Jiri Olsa <olsajiri@gmail.com> wrote: >> On Mon, Oct 21, 2024 at 03:16:16PM +0100, Mykyta Yatsenko wrote: >>> From: Mykyta Yatsenko <yatsenko@meta.com> >>> >>> The current default buffer size of 16MB allocated by veristat is no >>> longer sufficient to hold the verifier logs of some production BPF >>> programs. To address this issue, we need to increase the verifier log >>> limit. >>> Commit 7a9f5c65abcc ("bpf: increase verifier log limit") has already >>> increased the supported buffer size by the kernel, but veristat users >>> need to explicitly pass a log size argument to use the bigger log. >>> >>> This patch adds a function to detect the maximum verifier log size >>> supported by the kernel and uses that by default in veristat. >>> This ensures that veristat can handle larger verifier logs without >>> requiring users to manually specify the log size. >>> >>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com> >>> --- >>> tools/testing/selftests/bpf/veristat.c | 40 +++++++++++++++++++++++++- >>> 1 file changed, 39 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c >>> index c8efd44590d9..1d0708839f4b 100644 >>> --- a/tools/testing/selftests/bpf/veristat.c >>> +++ b/tools/testing/selftests/bpf/veristat.c >>> @@ -16,10 +16,12 @@ >>> #include <sys/stat.h> >>> #include <bpf/libbpf.h> >>> #include <bpf/btf.h> >>> +#include <bpf/bpf.h> >>> #include <libelf.h> >>> #include <gelf.h> >>> #include <float.h> >>> #include <math.h> >>> +#include <linux/filter.h> > this is kernel-internal header, which will be a problem for Github mirror, so... > >>> #ifndef ARRAY_SIZE >>> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) >>> @@ -1109,6 +1111,42 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch >>> return; >>> } >>> >>> +static int max_verifier_log_size(void) >>> +{ >>> + const int big_log_size = UINT_MAX >> 2; >>> + const int small_log_size = UINT_MAX >> 8; > nit: MAKE_ALL_CAPS, given they are fixed constants > >>> + struct bpf_insn insns[] = { >>> + BPF_MOV64_IMM(BPF_REG_0, 0), >>> + BPF_EXIT_INSN(), > ... let's instead either define these macro locally or just hard-code > bpf_insn structs as is (thankfully we need just two) > >>> + }; >>> + int ret, insn_cnt = ARRAY_SIZE(insns); >>> + char *log_buf; >>> + static int log_size; >>> + >>> + if (log_size != 0) >>> + return log_size; >>> + >>> + log_size = small_log_size; >>> + log_buf = malloc(big_log_size); > we don't really need to allocate anything. We can pass (void*)-1 as > log_buf (invalid pointer), set size to UINT_MAX >> 8, log_level = 4. > If the kernel doesn't support big log_size, we'll get -EINVAL. If it > does, we'll get -EFAULT when the verifier will try to write something > to the buffer. No allocation. > > pw-bot: cr > >> IIUC this would try to use 1GB by default? seems to agresive.. could we perhaps >> do that gradually and double the size on each failed load attempt? > The idea is that verifier will only page in as many pages as there is > an actual log content (which normally would be much smaller than a > full 1GB). Doing gradual size increase is actually pretty annoying in > terms of how the code and logic is structured. So I think this > approach is fine, overall. > >> jirka >> >> >>> + >>> + if (!log_buf) >>> + return log_size; >>> + >>> + LIBBPF_OPTS(bpf_prog_load_opts, opts, >>> + .log_buf = log_buf, >>> + .log_size = big_log_size, >>> + .log_level = 2 > no need for log_level = 2, just use 4, we don't need to fill out the > buffer, we need a verifier to check parameters. > >>> + ); > LIBBPF_OPTS() macro define a variable, so please move it to the > variable declaration block above. > >>> + ret = bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, NULL, "GPL", insns, insn_cnt, &opts); > nit: let's use TRACEPOINT instead, we had some problems with > SOCKET_FILTER on some old Red Hat distro due to how they did selective > backport, so best to avoid it, if possible. > >>> + free(log_buf); >>> + >>> + if (ret > 0) { >>> + log_size = big_log_size; >>> + close(ret); >>> + } >>> + return log_size; >>> +} >>> + >>> static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog) >>> { >>> const char *base_filename = basename(strdupa(filename)); >>> @@ -1132,7 +1170,7 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf >>> memset(stats, 0, sizeof(*stats)); >>> >>> if (env.verbose || env.top_src_lines > 0) { >>> - buf_sz = env.log_size ? env.log_size : 16 * 1024 * 1024; >>> + buf_sz = env.log_size ? env.log_size : max_verifier_log_size(); >>> buf = malloc(buf_sz); >>> if (!buf) >>> return -ENOMEM; >>> -- >>> 2.47.0 >>> >>> Thanks for taking a look, I'll apply your suggestions for v2.
On Mon, 2024-10-21 at 13:14 -0700, Andrii Nakryiko wrote: > On Mon, Oct 21, 2024 at 9:44 AM Jiri Olsa <olsajiri@gmail.com> wrote: [...] > > IIUC this would try to use 1GB by default? seems to agresive.. could we perhaps > > do that gradually and double the size on each failed load attempt? > > The idea is that verifier will only page in as many pages as there is > an actual log content (which normally would be much smaller than a > full 1GB). Doing gradual size increase is actually pretty annoying in > terms of how the code and logic is structured. So I think this > approach is fine, overall. As far as I understand, this feature is most useful for programs that are close to 1M instructions limit. Such programs easily produce huge logs. E.g. selftest pyperf600.bpf.o takes 627,288 instructions to verify and generates 281MB of log. I agree with Andrii, that allocating 1G for this purpose seems reasonable. [...]
diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index c8efd44590d9..1d0708839f4b 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -16,10 +16,12 @@ #include <sys/stat.h> #include <bpf/libbpf.h> #include <bpf/btf.h> +#include <bpf/bpf.h> #include <libelf.h> #include <gelf.h> #include <float.h> #include <math.h> +#include <linux/filter.h> #ifndef ARRAY_SIZE #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) @@ -1109,6 +1111,42 @@ static void fixup_obj(struct bpf_object *obj, struct bpf_program *prog, const ch return; } +static int max_verifier_log_size(void) +{ + const int big_log_size = UINT_MAX >> 2; + const int small_log_size = UINT_MAX >> 8; + struct bpf_insn insns[] = { + BPF_MOV64_IMM(BPF_REG_0, 0), + BPF_EXIT_INSN(), + }; + int ret, insn_cnt = ARRAY_SIZE(insns); + char *log_buf; + static int log_size; + + if (log_size != 0) + return log_size; + + log_size = small_log_size; + log_buf = malloc(big_log_size); + + if (!log_buf) + return log_size; + + LIBBPF_OPTS(bpf_prog_load_opts, opts, + .log_buf = log_buf, + .log_size = big_log_size, + .log_level = 2 + ); + ret = bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, NULL, "GPL", insns, insn_cnt, &opts); + free(log_buf); + + if (ret > 0) { + log_size = big_log_size; + close(ret); + } + return log_size; +} + static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog) { const char *base_filename = basename(strdupa(filename)); @@ -1132,7 +1170,7 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf memset(stats, 0, sizeof(*stats)); if (env.verbose || env.top_src_lines > 0) { - buf_sz = env.log_size ? env.log_size : 16 * 1024 * 1024; + buf_sz = env.log_size ? env.log_size : max_verifier_log_size(); buf = malloc(buf_sz); if (!buf) return -ENOMEM;