* [PATCH bpf-next v2] libbpf: don't allocate 16M for log buffer by default
@ 2020-03-25 16:20 Stanislav Fomichev
2020-03-25 19:06 ` Andrii Nakryiko
0 siblings, 1 reply; 3+ messages in thread
From: Stanislav Fomichev @ 2020-03-25 16:20 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
For each prog/btf load we allocate and free 16 megs of verifier buffer.
On production systems it doesn't really make sense because the
programs/btf have gone through extensive testing and (mostly) guaranteed
to successfully load.
Let's assume successful case by default and skip buffer allocation
on the first try. If there is an error, start with BPF_LOG_BUF_SIZE
and double it on each ENOSPC iteration.
v2:
* Don't allocate the buffer at all on the first try (Andrii Nakryiko)
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/lib/bpf/btf.c | 20 +++++++++++++++-----
tools/lib/bpf/libbpf.c | 22 ++++++++++++++--------
2 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 3d1c25fc97ae..bfef3d606b54 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -657,22 +657,32 @@ int btf__finalize_data(struct bpf_object *obj, struct btf *btf)
int btf__load(struct btf *btf)
{
- __u32 log_buf_size = BPF_LOG_BUF_SIZE;
+ __u32 log_buf_size = 0;
char *log_buf = NULL;
int err = 0;
if (btf->fd >= 0)
return -EEXIST;
- log_buf = malloc(log_buf_size);
- if (!log_buf)
- return -ENOMEM;
+retry_load:
+ if (log_buf_size) {
+ log_buf = malloc(log_buf_size);
+ if (!log_buf)
+ return -ENOMEM;
- *log_buf = 0;
+ *log_buf = 0;
+ }
btf->fd = bpf_load_btf(btf->data, btf->data_size,
log_buf, log_buf_size, false);
if (btf->fd < 0) {
+ if (!log_buf || errno == ENOSPC) {
+ log_buf_size = max((__u32)BPF_LOG_BUF_SIZE,
+ log_buf_size << 1);
+ free(log_buf);
+ goto retry_load;
+ }
+
err = -errno;
pr_warn("Error loading BTF: %s(%d)\n", strerror(errno), errno);
if (*log_buf)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 085e41f9b68e..b2fd43a03708 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4855,8 +4855,8 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
{
struct bpf_load_program_attr load_attr;
char *cp, errmsg[STRERR_BUFSIZE];
- int log_buf_size = BPF_LOG_BUF_SIZE;
- char *log_buf;
+ size_t log_buf_size = 0;
+ char *log_buf = NULL;
int btf_fd, ret;
if (!insns || !insns_cnt)
@@ -4896,22 +4896,28 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
load_attr.prog_flags = prog->prog_flags;
retry_load:
- log_buf = malloc(log_buf_size);
- if (!log_buf)
- pr_warn("Alloc log buffer for bpf loader error, continue without log\n");
+ if (log_buf_size) {
+ log_buf = malloc(log_buf_size);
+ if (!log_buf)
+ pr_warn("Alloc log buffer for bpf loader error, continue without log\n");
+
+ *log_buf = 0;
+ }
ret = bpf_load_program_xattr(&load_attr, log_buf, log_buf_size);
if (ret >= 0) {
- if (load_attr.log_level)
+ if (log_buf && load_attr.log_level)
pr_debug("verifier log:\n%s", log_buf);
*pfd = ret;
ret = 0;
goto out;
}
- if (errno == ENOSPC) {
- log_buf_size <<= 1;
+ if (!log_buf || errno == ENOSPC) {
+ log_buf_size = max((size_t)BPF_LOG_BUF_SIZE,
+ log_buf_size << 1);
+
free(log_buf);
goto retry_load;
}
--
2.25.1.696.g5e7596f4ac-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v2] libbpf: don't allocate 16M for log buffer by default
2020-03-25 16:20 [PATCH bpf-next v2] libbpf: don't allocate 16M for log buffer by default Stanislav Fomichev
@ 2020-03-25 19:06 ` Andrii Nakryiko
2020-03-25 19:48 ` Stanislav Fomichev
0 siblings, 1 reply; 3+ messages in thread
From: Andrii Nakryiko @ 2020-03-25 19:06 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, bpf, David S. Miller, Alexei Starovoitov, Daniel Borkmann
On Wed, Mar 25, 2020 at 9:20 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> For each prog/btf load we allocate and free 16 megs of verifier buffer.
> On production systems it doesn't really make sense because the
> programs/btf have gone through extensive testing and (mostly) guaranteed
> to successfully load.
>
> Let's assume successful case by default and skip buffer allocation
> on the first try. If there is an error, start with BPF_LOG_BUF_SIZE
> and double it on each ENOSPC iteration.
>
> v2:
> * Don't allocate the buffer at all on the first try (Andrii Nakryiko)
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> tools/lib/bpf/btf.c | 20 +++++++++++++++-----
> tools/lib/bpf/libbpf.c | 22 ++++++++++++++--------
> 2 files changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 3d1c25fc97ae..bfef3d606b54 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -657,22 +657,32 @@ int btf__finalize_data(struct bpf_object *obj, struct btf *btf)
>
> int btf__load(struct btf *btf)
> {
> - __u32 log_buf_size = BPF_LOG_BUF_SIZE;
> + __u32 log_buf_size = 0;
> char *log_buf = NULL;
> int err = 0;
>
> if (btf->fd >= 0)
> return -EEXIST;
>
> - log_buf = malloc(log_buf_size);
> - if (!log_buf)
> - return -ENOMEM;
> +retry_load:
> + if (log_buf_size) {
> + log_buf = malloc(log_buf_size);
> + if (!log_buf)
> + return -ENOMEM;
>
> - *log_buf = 0;
> + *log_buf = 0;
> + }
>
> btf->fd = bpf_load_btf(btf->data, btf->data_size,
> log_buf, log_buf_size, false);
> if (btf->fd < 0) {
> + if (!log_buf || errno == ENOSPC) {
> + log_buf_size = max((__u32)BPF_LOG_BUF_SIZE,
> + log_buf_size << 1);
> + free(log_buf);
> + goto retry_load;
> + }
> +
> err = -errno;
> pr_warn("Error loading BTF: %s(%d)\n", strerror(errno), errno);
> if (*log_buf)
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 085e41f9b68e..b2fd43a03708 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4855,8 +4855,8 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> {
> struct bpf_load_program_attr load_attr;
> char *cp, errmsg[STRERR_BUFSIZE];
> - int log_buf_size = BPF_LOG_BUF_SIZE;
> - char *log_buf;
> + size_t log_buf_size = 0;
> + char *log_buf = NULL;
> int btf_fd, ret;
>
> if (!insns || !insns_cnt)
> @@ -4896,22 +4896,28 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> load_attr.prog_flags = prog->prog_flags;
>
> retry_load:
> - log_buf = malloc(log_buf_size);
> - if (!log_buf)
> - pr_warn("Alloc log buffer for bpf loader error, continue without log\n");
> + if (log_buf_size) {
> + log_buf = malloc(log_buf_size);
> + if (!log_buf)
> + pr_warn("Alloc log buffer for bpf loader error, continue without log\n");
> +
considering that log_buf is not allocated first time, if it fails to
allocate on retry then it should be an error, no?
Otherwise looks good, please add my ack after fixing this:
Acked-by: Andrii Nakryiko <andriin@fb.com>
> + *log_buf = 0;
> + }
>
> ret = bpf_load_program_xattr(&load_attr, log_buf, log_buf_size);
>
> if (ret >= 0) {
> - if (load_attr.log_level)
> + if (log_buf && load_attr.log_level)
> pr_debug("verifier log:\n%s", log_buf);
> *pfd = ret;
> ret = 0;
> goto out;
> }
>
> - if (errno == ENOSPC) {
> - log_buf_size <<= 1;
> + if (!log_buf || errno == ENOSPC) {
> + log_buf_size = max((size_t)BPF_LOG_BUF_SIZE,
> + log_buf_size << 1);
> +
> free(log_buf);
> goto retry_load;
> }
> --
> 2.25.1.696.g5e7596f4ac-goog
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v2] libbpf: don't allocate 16M for log buffer by default
2020-03-25 19:06 ` Andrii Nakryiko
@ 2020-03-25 19:48 ` Stanislav Fomichev
0 siblings, 0 replies; 3+ messages in thread
From: Stanislav Fomichev @ 2020-03-25 19:48 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Stanislav Fomichev, Networking, bpf, David S. Miller,
Alexei Starovoitov, Daniel Borkmann
On 03/25, Andrii Nakryiko wrote:
> On Wed, Mar 25, 2020 at 9:20 AM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > For each prog/btf load we allocate and free 16 megs of verifier buffer.
> > On production systems it doesn't really make sense because the
> > programs/btf have gone through extensive testing and (mostly) guaranteed
> > to successfully load.
> >
> > Let's assume successful case by default and skip buffer allocation
> > on the first try. If there is an error, start with BPF_LOG_BUF_SIZE
> > and double it on each ENOSPC iteration.
> >
> > v2:
> > * Don't allocate the buffer at all on the first try (Andrii Nakryiko)
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > tools/lib/bpf/btf.c | 20 +++++++++++++++-----
> > tools/lib/bpf/libbpf.c | 22 ++++++++++++++--------
> > 2 files changed, 29 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index 3d1c25fc97ae..bfef3d606b54 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -657,22 +657,32 @@ int btf__finalize_data(struct bpf_object *obj, struct btf *btf)
> >
> > int btf__load(struct btf *btf)
> > {
> > - __u32 log_buf_size = BPF_LOG_BUF_SIZE;
> > + __u32 log_buf_size = 0;
> > char *log_buf = NULL;
> > int err = 0;
> >
> > if (btf->fd >= 0)
> > return -EEXIST;
> >
> > - log_buf = malloc(log_buf_size);
> > - if (!log_buf)
> > - return -ENOMEM;
> > +retry_load:
> > + if (log_buf_size) {
> > + log_buf = malloc(log_buf_size);
> > + if (!log_buf)
> > + return -ENOMEM;
> >
> > - *log_buf = 0;
> > + *log_buf = 0;
> > + }
> >
> > btf->fd = bpf_load_btf(btf->data, btf->data_size,
> > log_buf, log_buf_size, false);
> > if (btf->fd < 0) {
> > + if (!log_buf || errno == ENOSPC) {
> > + log_buf_size = max((__u32)BPF_LOG_BUF_SIZE,
> > + log_buf_size << 1);
> > + free(log_buf);
> > + goto retry_load;
> > + }
> > +
> > err = -errno;
> > pr_warn("Error loading BTF: %s(%d)\n", strerror(errno), errno);
> > if (*log_buf)
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 085e41f9b68e..b2fd43a03708 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -4855,8 +4855,8 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> > {
> > struct bpf_load_program_attr load_attr;
> > char *cp, errmsg[STRERR_BUFSIZE];
> > - int log_buf_size = BPF_LOG_BUF_SIZE;
> > - char *log_buf;
> > + size_t log_buf_size = 0;
> > + char *log_buf = NULL;
> > int btf_fd, ret;
> >
> > if (!insns || !insns_cnt)
> > @@ -4896,22 +4896,28 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> > load_attr.prog_flags = prog->prog_flags;
> >
> > retry_load:
> > - log_buf = malloc(log_buf_size);
> > - if (!log_buf)
> > - pr_warn("Alloc log buffer for bpf loader error, continue without log\n");
> > + if (log_buf_size) {
> > + log_buf = malloc(log_buf_size);
> > + if (!log_buf)
> > + pr_warn("Alloc log buffer for bpf loader error, continue without log\n");
> > +
>
> considering that log_buf is not allocated first time, if it fails to
> allocate on retry then it should be an error, no?
>
> Otherwise looks good, please add my ack after fixing this:
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
Thx, will respin a v3. Returning -ENOMEM also makes it more consistent with
btf retry logic.
> > + *log_buf = 0;
> > + }
> >
> > ret = bpf_load_program_xattr(&load_attr, log_buf, log_buf_size);
> >
> > if (ret >= 0) {
> > - if (load_attr.log_level)
> > + if (log_buf && load_attr.log_level)
> > pr_debug("verifier log:\n%s", log_buf);
> > *pfd = ret;
> > ret = 0;
> > goto out;
> > }
> >
> > - if (errno == ENOSPC) {
> > - log_buf_size <<= 1;
> > + if (!log_buf || errno == ENOSPC) {
> > + log_buf_size = max((size_t)BPF_LOG_BUF_SIZE,
> > + log_buf_size << 1);
> > +
> > free(log_buf);
> > goto retry_load;
> > }
> > --
> > 2.25.1.696.g5e7596f4ac-goog
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-03-25 19:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-25 16:20 [PATCH bpf-next v2] libbpf: don't allocate 16M for log buffer by default Stanislav Fomichev
2020-03-25 19:06 ` Andrii Nakryiko
2020-03-25 19:48 ` Stanislav Fomichev
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.