On Sun, Dec 05, 2021 at 12:32:29PM -0800, Andrii Nakryiko wrote:
>
> + ret = bpf_prog_load(prog->type, prog_name, license, insns, insns_cnt, &load_attr);
> if (ret >= 0) {
> - if (log_buf && load_attr.log_level) {
> + if (log_level && own_log_buf) {
> pr_debug("prog '%s': -- BEGIN PROG LOAD LOG --\n%s-- END PROG LOAD LOG --\n",
> prog->name, log_buf);
> }
> @@ -6690,19 +6720,20 @@ static int bpf_object_load_prog_instance(struct bpf_object *obj, struct bpf_prog
> goto out;
> }
>
> - if (!log_buf || errno == ENOSPC) {
> - log_buf_size = max((size_t)BPF_LOG_BUF_SIZE,
> - log_buf_size << 1);
> - free(log_buf);
> + if (log_level == 0) {
> + log_level = 1;
> goto retry_load;
> }
I think the new log_level semantics makes sense,
but can we do it only in one layer?
The above piece of bpf_object_load_prog_instance() will change log_level,
but then bpf_prog_load_v0_6_0() will do it again
when log_buf != NULL.
The latter will not malloc log_buf, but the former will.
Though both change log_level.
Can we somehow unify this logic and only do log_level adjustment and log_buf
alloc in bpf_object_load_prog_instance() only ?
> + /* on ENOSPC, increase log buffer size, unless custom log_buf is specified */
> + if (own_log_buf && errno == ENOSPC && log_buf_size < UINT_MAX / 2)
> + goto retry_load;
The kernel allows buf_size <= UINT_MAX >> 2.
Above condition will probably get to the same value, but it's not obvious.
Maybe make it exactly as kernel?
> - if (log_buf && log_buf[0] != '\0') {
> + if (own_log_buf && log_buf && log_buf[0] != '\0') {
> pr_warn("prog '%s': -- BEGIN PROG LOAD LOG --\n%s-- END PROG LOAD LOG --\n",
> prog->name, log_buf);
> }
> @@ -6712,7 +6743,8 @@ static int bpf_object_load_prog_instance(struct bpf_object *obj, struct bpf_prog
> }
>
> out:
> - free(log_buf);
> + if (own_log_buf)
> + free(log_buf);
For lksel I'm thinking to pass allocated log_buf back.
lskel has no ability to printf from inside of it, so log_buf has to be passed back.
I wonder whether it would make sense for libbpf as well?
The own_log_buf flag can be kept in bpf_program and caller can
examine the log_buf instead of doing bpf_program__set_log_buf() below...
> +int bpf_program__set_log_buf(struct bpf_program *prog, char *log_buf, size_t log_size)
> +{
> + if (log_size && !log_buf)
> + return -EINVAL;
> + if (prog->log_size > UINT_MAX)
> + return -EINVAL;
> + if (prog->obj->loaded)
> + return -EBUSY;
> +
> + prog->log_buf = log_buf;
> + prog->log_size = log_size;
> + return 0;
> +}
The problem with this helper is that the user would have to malloc
always even though the prog might load just fine.
But there is a chance of load failure even in production,
so the user would have to rely on libbpf printfs and override print func
or do prog_load without bpf_program__set_log_buf() and then do it again on error
or always do bpf_program__set_log_buf() with giant malloc.
All of these 3 options are not that great.
The 4th option is for libbpf to do a malloc in case of error and return it.
That's the most convenient:
err = ...prog_load(..)
if (err) // user will check what's in the log.
No need to override libbpf print, unconditionally allocated or do double load.
