libbpf: rename btf__get_from_id() and btf__load() APIs, support split BTF

Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to better indicate what the function does. Change the new function so that, instead of requiring a pointer to the pointer to update and returning with an error code, it takes a single argument (the id of the BTF object) and returns the corresponding pointer. This is more in line with the existing constructors.

The other tools calling the deprecated btf__get_from_id() function will be updated in a future commit.

References:

- https://github.com/libbpf/libbpf/issues/278
- https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis

v2:
- Instead of a simple renaming, change the new function to make it return the pointer to the btf struct.
- API v0.5.0 instead of v0.6.0.

Signed-off-by Quentin Monnet <[email protected]>

Acked-by John Fastabend <[email protected]>

---
 tools/lib/bpf/btf.c      | 25 +++++++++++++++++--------
 tools/lib/bpf/btf.h      |  1 +
 tools/lib/bpf/libbpf.c   |  5 +++--
 tools/lib/bpf/libbpf.map |  1 +
 4 files changed, 22 insertions(+), 10 deletions(-)

tools/lib/bpf/btf.c +0 -0

index 7e0de560490e..6654bdee7ad7 100644
@@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
        return btf;

tools/lib/bpf/btf.c +9 -8

+
+       btf = btf_get_from_fd(btf_fd, NULL);
+       close(btf_fd);
+
+       return libbpf_ptr(btf);
+}
+
 int btf__get_from_id(__u32 id, struct btf **btf)
 {
        struct btf *res;
-       int err, btf_fd;
+       int err;
 
        *btf = NULL;
-       btf_fd = bpf_btf_get_fd_by_id(id);
-       if (btf_fd < 0)
-               return libbpf_err(-errno);
-
-       res = btf_get_from_fd(btf_fd, NULL);
+       res = btf__load_from_kernel_by_id(id);
        err = libbpf_get_error(res);
 
-       close(btf_fd);
-
        if (err)
                return libbpf_err(err);
 

tools/lib/bpf/btf.h +0 -0

index fd8a21d936ef..3db9446bc133 100644

tools/lib/bpf/btf.h +0 -0

 LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
                                    __u32 expected_key_size,
                                    __u32 expected_value_size,

tools/lib/bpf/libbpf.c +1 -1

index 242e97892043..eff005b1eba1 100644
@@ -9576,8 +9576,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
 {
        struct bpf_prog_info_linear *info_linear;
        struct bpf_prog_info *info;
-       struct btf *btf = NULL;
        int err = -EINVAL;
+       struct btf *btf;
 
        info_linear = bpf_program__get_prog_info_linear(attach_prog_fd, 0);
        err = libbpf_get_error(info_linear);

tools/lib/bpf/libbpf.c +0 -0

                pr_warn("Failed to get BTF of the program\n");
                goto out;
        }

tools/lib/bpf/libbpf.map +1 -1

index f7d52d76ca3a..ca8cc7a7faad 100644
@@ -373,6 +373,7 @@ LIBBPF_0.5.0 {
                bpf_map__initial_value;
                bpf_map_lookup_and_delete_elem_flags;
                bpf_object__gen_loader;
+               btf__load_from_kernel_by_id;
                btf__load_into_kernel;
                btf_dump__dump_type_data;
                libbpf_set_strict_mode;
--

2.30.2

Quoted 13 lines

Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to better indicate what the function does. Change the new function so that, instead of requiring a pointer to the pointer to update and returning with an error code, it takes a single argument (the id of the BTF object) and returns the corresponding pointer. This is more in line with the existing constructors.

The other tools calling the deprecated btf__get_from_id() function will be updated in a future commit.

References:

Quoted 7 lines

- https://github.com/libbpf/libbpf/issues/278
- https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis

v2:
- Instead of a simple renaming, change the new function to make it return the pointer to the btf struct.
- API v0.5.0 instead of v0.6.0.

We generally keep such version changes to cover letters. It keeps each individual commit clean and collects full history in the cover letter which becomes a body of merge commit when the whole patch set is applied. For next revision please consolidate the history in the cover letter. Thanks!

Quoted 19 lines

Signed-off-by Quentin Monnet <[email protected]>

Acked-by John Fastabend <[email protected]>

---
 tools/lib/bpf/btf.c      | 25 +++++++++++++++++--------
 tools/lib/bpf/btf.h      |  1 +
 tools/lib/bpf/libbpf.c   |  5 +++--
 tools/lib/bpf/libbpf.map |  1 +
 4 files changed, 22 insertions(+), 10 deletions(-)

tools/lib/bpf/btf.c +0 -0

index 7e0de560490e..6654bdee7ad7 100644
@@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
        return btf;
 }
 

tools/lib/bpf/btf.c struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf) · line 1393

Quoted 8 lines

tools/lib/bpf/btf.c +8 -0

+struct btf *btf__load_from_kernel_by_id(__u32 id)
+{
+       struct btf *btf;
+       int btf_fd;
+
+       btf_fd = bpf_btf_get_fd_by_id(id);
+       if (btf_fd < 0)
+               return ERR_PTR(-errno);

please use libbpf_err_ptr() for consistency, see
bpf_object__open_mem() for an example

Quoted 28 lines

tools/lib/bpf/btf.c +9 -8

+
+       btf = btf_get_from_fd(btf_fd, NULL);
+       close(btf_fd);
+
+       return libbpf_ptr(btf);
+}
+
 int btf__get_from_id(__u32 id, struct btf **btf)
 {
        struct btf *res;
-       int err, btf_fd;
+       int err;
 
        *btf = NULL;
-       btf_fd = bpf_btf_get_fd_by_id(id);
-       if (btf_fd < 0)
-               return libbpf_err(-errno);
-
-       res = btf_get_from_fd(btf_fd, NULL);
+       res = btf__load_from_kernel_by_id(id);
        err = libbpf_get_error(res);
 
-       close(btf_fd);
-
        if (err)
                return libbpf_err(err);
 

tools/lib/bpf/btf.h LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size); · line 71

Quoted 9 lines

tools/lib/bpf/btf.h +1 -0

index fd8a21d936ef..3db9446bc133 100644
@@ -68,6 +68,7 @@ LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size);
 LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
 LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
 LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
+LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);

let's move this definition to after btf__parse() to keep all "constructors" together (we can move btf__get_from_id() there for completeness as well, I suppose).

Quoted 16 lines

tools/lib/bpf/btf.h +0 -0

 LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
                                    __u32 expected_key_size,
                                    __u32 expected_value_size,

tools/lib/bpf/libbpf.c +1 -1

index 242e97892043..eff005b1eba1 100644
@@ -9576,8 +9576,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
 {
        struct bpf_prog_info_linear *info_linear;
        struct bpf_prog_info *info;
-       struct btf *btf = NULL;
        int err = -EINVAL;
+       struct btf *btf;
 

tools/lib/bpf/libbpf.c static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) · line 9595

Quoted 9 lines

tools/lib/bpf/libbpf.c +2 -1

        info_linear = bpf_program__get_prog_info_linear(attach_prog_fd, 0);
        err = libbpf_get_error(info_linear);

@@ -9591,7 +9591,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
                pr_warn("The target program doesn't have BTF\n");
                goto out;
        }
-       if (btf__get_from_id(info->btf_id, &btf)) {
+       btf = btf__load_from_kernel_by_id(info->btf_id);
+       if (libbpf_get_error(btf)) {

there seems to be a bug in existing code and you are keeping it. On error err will be 0. Let's fix it. Same for above if (!info->btf_id), please fix that as well while you are at it.

2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 22 lines · Quentin Monnet

Quoted 13 lines

Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to better indicate what the function does. Change the new function so that, instead of requiring a pointer to the pointer to update and returning with an error code, it takes a single argument (the id of the BTF object) and returns the corresponding pointer. This is more in line with the existing constructors.

The other tools calling the deprecated btf__get_from_id() function will be updated in a future commit.

References:

Quoted 7 lines

- https://github.com/libbpf/libbpf/issues/278
- https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis

v2:
- Instead of a simple renaming, change the new function to make it return the pointer to the btf struct.
- API v0.5.0 instead of v0.6.0.

Quoted 5 lines

We generally keep such version changes to cover letters. It keeps each individual commit clean and collects full history in the cover letter which becomes a body of merge commit when the whole patch set is applied. For next revision please consolidate the history in the cover letter. Thanks!

OK will do.
I've seen other folks detailing the changes on individual patches, and done so in the past, although it's true the current trend is to have it in the cover letter (and I understand the motivation).

Quoted 19 lines

Quoted 19 lines

Signed-off-by Quentin Monnet <[email protected]>

Acked-by John Fastabend <[email protected]>

---
 tools/lib/bpf/btf.c      | 25 +++++++++++++++++--------
 tools/lib/bpf/btf.h      |  1 +
 tools/lib/bpf/libbpf.c   |  5 +++--
 tools/lib/bpf/libbpf.map |  1 +
 4 files changed, 22 insertions(+), 10 deletions(-)

tools/lib/bpf/btf.c +0 -0

index 7e0de560490e..6654bdee7ad7 100644
@@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
        return btf;
 }
 

Quoted 11 lines

tools/lib/bpf/btf.c struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf) · line 1393

Quoted 8 lines

tools/lib/bpf/btf.c +8 -0

+struct btf *btf__load_from_kernel_by_id(__u32 id)
+{
+       struct btf *btf;
+       int btf_fd;
+
+       btf_fd = bpf_btf_get_fd_by_id(id);
+       if (btf_fd < 0)
+               return ERR_PTR(-errno);

please use libbpf_err_ptr() for consistency, see
bpf_object__open_mem() for an example

I can do that, but I'll need to uncouple btf__get_from_id() from the new function. If it calls btf__load_from_kernel_by_id() and LIBBPF_STRICT_CLEAN_PTRS is set, it would change its return value.

Quoted 29 lines

Quoted 29 lines

tools/lib/bpf/btf.c +9 -8

+
+       btf = btf_get_from_fd(btf_fd, NULL);
+       close(btf_fd);
+
+       return libbpf_ptr(btf);
+}
+
 int btf__get_from_id(__u32 id, struct btf **btf)
 {
        struct btf *res;
-       int err, btf_fd;
+       int err;
 
        *btf = NULL;
-       btf_fd = bpf_btf_get_fd_by_id(id);
-       if (btf_fd < 0)
-               return libbpf_err(-errno);
-
-       res = btf_get_from_fd(btf_fd, NULL);
+       res = btf__load_from_kernel_by_id(id);
        err = libbpf_get_error(res);
 
-       close(btf_fd);
-
        if (err)
                return libbpf_err(err);
 

Quoted 13 lines

tools/lib/bpf/btf.h LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size); · line 71

Quoted 9 lines

tools/lib/bpf/btf.h +1 -0

index fd8a21d936ef..3db9446bc133 100644
@@ -68,6 +68,7 @@ LIBBPF_API const void *btf__get_raw_data(const struct btf *btf, __u32 *size);
 LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
 LIBBPF_API const char *btf__str_by_offset(const struct btf *btf, __u32 offset);
 LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
+LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);

let's move this definition to after btf__parse() to keep all "constructors" together (we can move btf__get_from_id() there for completeness as well, I suppose).

I thought about that but wasn't sure, OK will do.

Quoted 17 lines

Quoted 17 lines

tools/lib/bpf/btf.h +0 -0

 LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
                                    __u32 expected_key_size,
                                    __u32 expected_value_size,

tools/lib/bpf/libbpf.c +1 -1

index 242e97892043..eff005b1eba1 100644
@@ -9576,8 +9576,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
 {
        struct bpf_prog_info_linear *info_linear;
        struct bpf_prog_info *info;
-       struct btf *btf = NULL;
        int err = -EINVAL;
+       struct btf *btf;
 

Quoted 13 lines

tools/lib/bpf/libbpf.c static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) · line 9595

Quoted 9 lines

tools/lib/bpf/libbpf.c +2 -1

        info_linear = bpf_program__get_prog_info_linear(attach_prog_fd, 0);
        err = libbpf_get_error(info_linear);

@@ -9591,7 +9591,8 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
                pr_warn("The target program doesn't have BTF\n");
                goto out;
        }
-       if (btf__get_from_id(info->btf_id, &btf)) {
+       btf = btf__load_from_kernel_by_id(info->btf_id);
+       if (libbpf_get_error(btf)) {

there seems to be a bug in existing code and you are keeping it. On error err will be 0. Let's fix it. Same for above if (!info->btf_id), please fix that as well while you are at it.

Oh right, I saw that err was initialised at -EINVAL and did not notice it was changed for the info_linear. I'll address it.

Quoted 40 lines

2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 37 lines · Quentin Monnet

Quoted 23 lines

Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to better indicate what the function does. Change the new function so that, instead of requiring a pointer to the pointer to update and returning with an error code, it takes a single argument (the id of the BTF object) and returns the corresponding pointer. This is more in line with the existing constructors.

The other tools calling the deprecated btf__get_from_id() function will be updated in a future commit.

References:

- https://github.com/libbpf/libbpf/issues/278
- https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis

tools/lib/bpf/btf.c +0 -0

index 7e0de560490e..6654bdee7ad7 100644
@@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
        return btf;

tools/lib/bpf/btf.c struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf) · line 1393

Quoted 10 lines

tools/lib/bpf/btf.c +8 -0

 }
 
+struct btf *btf__load_from_kernel_by_id(__u32 id)
+{
+       struct btf *btf;
+       int btf_fd;
+
+       btf_fd = bpf_btf_get_fd_by_id(id);
+       if (btf_fd < 0)
+               return ERR_PTR(-errno);

please use libbpf_err_ptr() for consistency, see
bpf_object__open_mem() for an example

Quoted 3 lines

I can do that, but I'll need to uncouple btf__get_from_id() from the new function. If it calls btf__load_from_kernel_by_id() and LIBBPF_STRICT_CLEAN_PTRS is set, it would change its return value.

No it won't, if libbpf_get_error() is used right after the API call.

With CLEAN_PTRS the result pointer is NULL but actual error is passed through errno. libbpf_get_error() knows about this and extracts error from errno if passed NULL pointer. With returning ERR_PTR(-errno) from btf__load_from_kernel_by_id() you are breaking CLEAN_PTRS guarantees.

2021-07-23 08:54 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 45 lines · Quentin Monnet

Quoted 43 lines

2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 26 lines · Quentin Monnet

Quoted 25 lines

Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to better indicate what the function does. Change the new function so that, instead of requiring a pointer to the pointer to update and returning with an error code, it takes a single argument (the id of the BTF object) and returns the corresponding pointer. This is more in line with the existing constructors.

The other tools calling the deprecated btf__get_from_id() function will be updated in a future commit.

References:

- https://github.com/libbpf/libbpf/issues/278
- https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis

tools/lib/bpf/btf.c +0 -0

index 7e0de560490e..6654bdee7ad7 100644
@@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
        return btf;
 }
 

Quoted 11 lines

tools/lib/bpf/btf.c struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf) · line 1393

Quoted 8 lines

tools/lib/bpf/btf.c +8 -0

+struct btf *btf__load_from_kernel_by_id(__u32 id)
+{
+       struct btf *btf;
+       int btf_fd;
+
+       btf_fd = bpf_btf_get_fd_by_id(id);
+       if (btf_fd < 0)
+               return ERR_PTR(-errno);

please use libbpf_err_ptr() for consistency, see
bpf_object__open_mem() for an example

I can do that, but I'll need to uncouple btf__get_from_id() from the new function. If it calls btf__load_from_kernel_by_id() and LIBBPF_STRICT_CLEAN_PTRS is set, it would change its return value.

Quoted 1 line

No it won't, if libbpf_get_error() is used right after the API call.

But we cannot be sure that users currently call libbpf_get_error() after btf__get_from_id()? I'm fine if we assume they do (users currently selecting the CLEAN_PTRS are probably savvy enough to call it I guess), I'll update as you suggest.

Quoted 5 lines

With CLEAN_PTRS the result pointer is NULL but actual error is passed through errno. libbpf_get_error() knows about this and extracts error from errno if passed NULL pointer. With returning ERR_PTR(-errno) from btf__load_from_kernel_by_id() you are breaking CLEAN_PTRS guarantees.

OK right, this makes sense to me for btf__load_from_kernel_by_id().

Quoted 47 lines

2021-07-23 08:54 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 45 lines · Quentin Monnet

Quoted 43 lines

2021-07-22 17:39 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 26 lines · Quentin Monnet

Quoted 25 lines

Rename function btf__get_from_id() as btf__load_from_kernel_by_id() to better indicate what the function does. Change the new function so that, instead of requiring a pointer to the pointer to update and returning with an error code, it takes a single argument (the id of the BTF object) and returns the corresponding pointer. This is more in line with the existing constructors.

The other tools calling the deprecated btf__get_from_id() function will be updated in a future commit.

References:

- https://github.com/libbpf/libbpf/issues/278
- https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis

tools/lib/bpf/btf.c +0 -0

index 7e0de560490e..6654bdee7ad7 100644
@@ -1383,21 +1383,30 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf)
        return btf;
 }
 

Quoted 11 lines

tools/lib/bpf/btf.c struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf) · line 1393

Quoted 8 lines

tools/lib/bpf/btf.c +8 -0

+struct btf *btf__load_from_kernel_by_id(__u32 id)
+{
+       struct btf *btf;
+       int btf_fd;
+
+       btf_fd = bpf_btf_get_fd_by_id(id);
+       if (btf_fd < 0)
+               return ERR_PTR(-errno);

please use libbpf_err_ptr() for consistency, see
bpf_object__open_mem() for an example

I can do that, but I'll need to uncouple btf__get_from_id() from the new function. If it calls btf__load_from_kernel_by_id() and LIBBPF_STRICT_CLEAN_PTRS is set, it would change its return value.

Quoted 6 lines

Quoted 1 line

No it won't, if libbpf_get_error() is used right after the API call.

But we cannot be sure that users currently call libbpf_get_error() after btf__get_from_id()? I'm fine if we assume they do (users currently selecting the CLEAN_PTRS are probably savvy enough to call it I guess), I'll update as you suggest.

I think you are still confused.

Replace the calls to deprecated function btf__get_from_id() with calls to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests). Update the surrounding code accordingly (instead of passing a pointer to the btf struct, get it as a return value from the function). Also make sure that btf__free() is called on the pointer after use.

v2:
- Given that btf__load_from_kernel_by_id() has changed since v1, adapt the code accordingly instead of just renaming the function. Also add a few calls to btf__free() when necessary.

Signed-off-by Quentin Monnet <[email protected]>

Acked-by John Fastabend <[email protected]>

---
 tools/bpf/bpftool/btf.c                      |  8 ++----
 tools/bpf/bpftool/btf_dumper.c               |  6 ++--
 tools/bpf/bpftool/map.c                      | 16 +++++------
 tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
 tools/perf/util/bpf-event.c                  | 11 ++++----
 tools/perf/util/bpf_counter.c                | 12 ++++++--
 tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
 7 files changed, 51 insertions(+), 35 deletions(-)

tools/bpf/bpftool/map.c +0 -0

index 09ae0381205b..12787758ce03 100644
@@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
                }
                return btf_vmlinux;
        } else if (info->btf_value_type_id) {

tools/bpf/bpftool/map.c +3 -4

                }
        }
 

@@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
                            void *value)
 {
        json_writer_t *btf_wtr;
-       struct btf *btf = NULL;
-       int err;
+       struct btf *btf;
 
-       err = btf__get_from_id(info->btf_id, &btf);
-       if (err) {
+       btf = btf__load_from_kernel_by_id(info->btf_id);
+       if (libbpf_get_error(btf)) {
                p_err("failed to get btf");
                return;
        }

tools/bpf/bpftool/map.c +8 -3

        return 0;
 }
 

@@ -2002,8 +2007,8 @@ static char *profile_target_name(int tgt_fd)
        struct bpf_prog_info_linear *info_linear;
        struct bpf_func_info *func_info;
        const struct btf_type *t;
+       struct btf *btf = NULL;
        char *name = NULL;
-       struct btf *btf;
 
        info_linear = bpf_program__get_prog_info_linear(
                tgt_fd, 1UL << BPF_PROG_INFO_FUNC_INFO);

@@ -2012,12 +2017,17 @@ static char *profile_target_name(int tgt_fd)
                return NULL;
        }
 
-       if (info_linear->info.btf_id == 0 ||
-           btf__get_from_id(info_linear->info.btf_id, &btf)) {
+       if (info_linear->info.btf_id == 0) {
                p_err("prog FD %d doesn't have valid btf", tgt_fd);
                goto out;
        }
 
+       btf = btf__load_from_kernel_by_id(info_linear->info.btf_id);
+       if (libbpf_get_error(btf)) {
+               p_err("failed to load btf for prog FD %d", tgt_fd);
+               goto out;
+       }
+
        func_info = u64_to_ptr(info_linear->info.func_info);
        t = btf__type_by_id(btf, func_info[0].type_id);
        if (!t) {

tools/bpf/bpftool/map.c +0 -0

        free(info_linear);
        return name;
 }

Quoted 24 lines

Replace the calls to deprecated function btf__get_from_id() with calls to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests). Update the surrounding code accordingly (instead of passing a pointer to the btf struct, get it as a return value from the function). Also make sure that btf__free() is called on the pointer after use.

v2:
- Given that btf__load_from_kernel_by_id() has changed since v1, adapt the code accordingly instead of just renaming the function. Also add a few calls to btf__free() when necessary.

Signed-off-by Quentin Monnet <[email protected]>

Acked-by John Fastabend <[email protected]>

---
 tools/bpf/bpftool/btf.c                      |  8 ++----
 tools/bpf/bpftool/btf_dumper.c               |  6 ++--
 tools/bpf/bpftool/map.c                      | 16 +++++------
 tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
 tools/perf/util/bpf-event.c                  | 11 ++++----
 tools/perf/util/bpf_counter.c                | 12 ++++++--
 tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
 7 files changed, 51 insertions(+), 35 deletions(-)

[...]

Quoted 8 lines

tools/bpf/bpftool/map.c +0 -0

index 09ae0381205b..12787758ce03 100644
@@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
                }
                return btf_vmlinux;
        } else if (info->btf_value_type_id) {

tools/bpf/bpftool/map.c static struct btf *get_map_kv_btf(const struct bpf_map_info *info) · line 812

Quoted 10 lines

tools/bpf/bpftool/map.c +4 -5

-               int err;
-
-               err = btf__get_from_id(info->btf_id, &btf);
-               if (err || !btf) {
+               btf = btf__load_from_kernel_by_id(info->btf_id);
+               if (libbpf_get_error(btf)) {
                        p_err("failed to get btf");
-                       btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
+                       if (!btf)
+                               btf = ERR_PTR(-ESRCH);

why not do a simpler (less conditionals)

err = libbpf_get_error(btf);

if (err) {
    btf = ERR_PTR(err);
}

?

Quoted 19 lines

tools/bpf/bpftool/map.c +3 -4

                }
        }
 

@@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
                            void *value)
 {
        json_writer_t *btf_wtr;
-       struct btf *btf = NULL;
-       int err;
+       struct btf *btf;
 
-       err = btf__get_from_id(info->btf_id, &btf);
-       if (err) {
+       btf = btf__load_from_kernel_by_id(info->btf_id);
+       if (libbpf_get_error(btf)) {
                p_err("failed to get btf");
                return;
        }

[...]

Quoted 6 lines

        func_info = u64_to_ptr(info->func_info);

tools/bpf/bpftool/map.c +0 -0

@@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
                kernel_syms_destroy(&dd);
        }
 

tools/bpf/bpftool/map.c prog_dump(struct bpf_prog_info *info, enum dump_mode mode, · line 788

Quoted 2 lines

tools/bpf/bpftool/map.c +2 -0

+       btf__free(btf);
+

warrants a Fixes: tag?

2021-07-22 17:48 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 54 lines · Quentin Monnet

Quoted 24 lines

Replace the calls to deprecated function btf__get_from_id() with calls to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests). Update the surrounding code accordingly (instead of passing a pointer to the btf struct, get it as a return value from the function). Also make sure that btf__free() is called on the pointer after use.

v2:
- Given that btf__load_from_kernel_by_id() has changed since v1, adapt the code accordingly instead of just renaming the function. Also add a few calls to btf__free() when necessary.

Signed-off-by Quentin Monnet <[email protected]>

Acked-by John Fastabend <[email protected]>

---
 tools/bpf/bpftool/btf.c                      |  8 ++----
 tools/bpf/bpftool/btf_dumper.c               |  6 ++--
 tools/bpf/bpftool/map.c                      | 16 +++++------
 tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
 tools/perf/util/bpf-event.c                  | 11 ++++----
 tools/perf/util/bpf_counter.c                | 12 ++++++--
 tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
 7 files changed, 51 insertions(+), 35 deletions(-)

[...]

Quoted 8 lines

tools/bpf/bpftool/map.c +0 -0

index 09ae0381205b..12787758ce03 100644
@@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
                }
                return btf_vmlinux;
        } else if (info->btf_value_type_id) {

tools/bpf/bpftool/map.c static struct btf *get_map_kv_btf(const struct bpf_map_info *info) · line 812

Quoted 10 lines

tools/bpf/bpftool/map.c +4 -5

-               int err;
-
-               err = btf__get_from_id(info->btf_id, &btf);
-               if (err || !btf) {
+               btf = btf__load_from_kernel_by_id(info->btf_id);
+               if (libbpf_get_error(btf)) {
                        p_err("failed to get btf");
-                       btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
+                       if (!btf)
+                               btf = ERR_PTR(-ESRCH);

why not do a simpler (less conditionals)

err = libbpf_get_error(btf);

if (err) {
    btf = ERR_PTR(err);
}

Quoted 1 line

?

Because if btf is NULL at this stage, this would change the return value from -ESRCH to NULL. This would be problematic in mapdump(), since we check this value ("if (IS_ERR(btf))") to detect a failure in get_map_kv_btf().

I could change that check in mapdump() to use libbpf_get_error() instead, but in that case it would similarly change the return value for mapdump() (and errno), which I think would be propagated up to main() and would return 0 instead of -ESRCH. This does not seem suitable and would play badly with batch mode, among other things.

So I'm considering keeping the one additional if.

Quoted 28 lines

Quoted 19 lines

tools/bpf/bpftool/map.c +3 -4

                }
        }
 

@@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
                            void *value)
 {
        json_writer_t *btf_wtr;
-       struct btf *btf = NULL;
-       int err;
+       struct btf *btf;
 
-       err = btf__get_from_id(info->btf_id, &btf);
-       if (err) {
+       btf = btf__load_from_kernel_by_id(info->btf_id);
+       if (libbpf_get_error(btf)) {
                p_err("failed to get btf");
                return;
        }

[...]

Quoted 6 lines

        func_info = u64_to_ptr(info->func_info);

tools/bpf/bpftool/map.c +0 -0

@@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
                kernel_syms_destroy(&dd);
        }
 

Quoted 4 lines

tools/bpf/bpftool/map.c prog_dump(struct bpf_prog_info *info, enum dump_mode mode, · line 788

Quoted 2 lines

tools/bpf/bpftool/map.c +2 -0

+       btf__free(btf);
+

warrants a Fixes: tag?

I don't mind adding the tags, but do they have any advantage here? My understanding is that they tend to be neon signs for backports to stable branches, but this patch depends on btf__load_from_kernel_by_id(), meaning more patches to pull. I'll see if I can move the btf__free() fixes to a separate commit, maybe.

Quoted 56 lines

2021-07-22 17:48 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 54 lines · Quentin Monnet

Quoted 24 lines

Replace the calls to deprecated function btf__get_from_id() with calls to btf__load_from_kernel_by_id() in tools/ (bpftool, perf, selftests). Update the surrounding code accordingly (instead of passing a pointer to the btf struct, get it as a return value from the function). Also make sure that btf__free() is called on the pointer after use.

v2:
- Given that btf__load_from_kernel_by_id() has changed since v1, adapt the code accordingly instead of just renaming the function. Also add a few calls to btf__free() when necessary.

Signed-off-by Quentin Monnet <[email protected]>

Acked-by John Fastabend <[email protected]>

---
 tools/bpf/bpftool/btf.c                      |  8 ++----
 tools/bpf/bpftool/btf_dumper.c               |  6 ++--
 tools/bpf/bpftool/map.c                      | 16 +++++------
 tools/bpf/bpftool/prog.c                     | 29 ++++++++++++++------
 tools/perf/util/bpf-event.c                  | 11 ++++----
 tools/perf/util/bpf_counter.c                | 12 ++++++--
 tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
 7 files changed, 51 insertions(+), 35 deletions(-)

[...]

Quoted 8 lines

tools/bpf/bpftool/map.c +0 -0

index 09ae0381205b..12787758ce03 100644
@@ -805,12 +805,11 @@ static struct btf *get_map_kv_btf(const struct bpf_map_info *info)
                }
                return btf_vmlinux;
        } else if (info->btf_value_type_id) {

tools/bpf/bpftool/map.c static struct btf *get_map_kv_btf(const struct bpf_map_info *info) · line 812

Quoted 10 lines

tools/bpf/bpftool/map.c +4 -5

-               int err;
-
-               err = btf__get_from_id(info->btf_id, &btf);
-               if (err || !btf) {
+               btf = btf__load_from_kernel_by_id(info->btf_id);
+               if (libbpf_get_error(btf)) {
                        p_err("failed to get btf");
-                       btf = err ? ERR_PTR(err) : ERR_PTR(-ESRCH);
+                       if (!btf)
+                               btf = ERR_PTR(-ESRCH);

why not do a simpler (less conditionals)

err = libbpf_get_error(btf);

if (err) {
    btf = ERR_PTR(err);
}

Quoted 6 lines

Quoted 1 line

?

Because if btf is NULL at this stage, this would change the return value from -ESRCH to NULL. This would be problematic in mapdump(), since we check this value ("if (IS_ERR(btf))") to detect a failure in get_map_kv_btf().

see my reply on previous patch. libbpf_get_error() handles this transparently regardless of CLEAN_PTRS mode, as long as it is called right after API call. So the above sample will work as you'd expect, preserving errors.

Quoted 43 lines

I could change that check in mapdump() to use libbpf_get_error() instead, but in that case it would similarly change the return value for mapdump() (and errno), which I think would be propagated up to main() and would return 0 instead of -ESRCH. This does not seem suitable and would play badly with batch mode, among other things.

So I'm considering keeping the one additional if.

Quoted 28 lines

Quoted 19 lines

tools/bpf/bpftool/map.c +3 -4

                }
        }
 

@@ -1039,11 +1038,10 @@ static void print_key_value(struct bpf_map_info *info, void *key,
                            void *value)
 {
        json_writer_t *btf_wtr;
-       struct btf *btf = NULL;
-       int err;
+       struct btf *btf;
 
-       err = btf__get_from_id(info->btf_id, &btf);
-       if (err) {
+       btf = btf__load_from_kernel_by_id(info->btf_id);
+       if (libbpf_get_error(btf)) {
                p_err("failed to get btf");
                return;
        }

[...]

Quoted 6 lines

        func_info = u64_to_ptr(info->func_info);

tools/bpf/bpftool/map.c +0 -0

@@ -781,6 +784,8 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode,
                kernel_syms_destroy(&dd);
        }
 

Quoted 4 lines

tools/bpf/bpftool/map.c prog_dump(struct bpf_prog_info *info, enum dump_mode mode, · line 788

Quoted 2 lines

tools/bpf/bpftool/map.c +2 -0

+       btf__free(btf);
+

warrants a Fixes: tag?

Quoted 5 lines

I don't mind adding the tags, but do they have any advantage here? My understanding is that they tend to be neon signs for backports to stable branches, but this patch depends on btf__load_from_kernel_by_id(), meaning more patches to pull. I'll see if I can move the btf__free() fixes to a separate commit, maybe.

Having Fixes: allows to keep track of where the issue originated. It doesn't necessarily mean something has to be backported, as far as I understand. So it's good to do regardless. Splitting fixes into a separate patch works for me as well, but I don't care all that much given they are small.

Add a new API function btf__load_from_kernel_by_id_split(), which takes a pointer to a base BTF object in order to support split BTF objects when retrieving BTF information from the kernel.

Reference: https://github.com/libbpf/libbpf/issues/314

Signed-off-by Quentin Monnet <[email protected]>

Acked-by John Fastabend <[email protected]>

---
 tools/lib/bpf/btf.c      | 9 +++++++--
 tools/lib/bpf/btf.h      | 2 ++
 tools/lib/bpf/libbpf.map | 1 +
 3 files changed, 10 insertions(+), 2 deletions(-)

tools/lib/bpf/btf.h +0 -0

 LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
                                    __u32 expected_key_size,
                                    __u32 expected_value_size,

tools/lib/bpf/libbpf.map +1 -1

index ca8cc7a7faad..eecf77227aeb 100644
@@ -374,6 +374,7 @@ LIBBPF_0.5.0 {
                bpf_map_lookup_and_delete_elem_flags;
                bpf_object__gen_loader;
                btf__load_from_kernel_by_id;
+               btf__load_from_kernel_by_id_split;
                btf__load_into_kernel;
                btf_dump__dump_type_data;
                libbpf_set_strict_mode;
--

2.30.2

As part of the effort to move towards a v1.0 for libbpf [0], this set improves some confusing function names related to BTF loading from and to the kernel:

- btf__load() becomes btf__load_into_kernel().
- btf__get_from_id becomes btf__load_from_kernel_by_id().
- A new version btf__load_from_kernel_by_id_split() extends the former to add support for split BTF.

The old functions are not removed or marked as deprecated yet, there should be in a future libbpf version.

The last patch is a trivial change to bpftool to add support for dumping split BTF objects by referencing them by their id (and not only by their BTF path).

[0] https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis

v2:
- Remove deprecation marking of legacy functions (patch 4/6 from v1).
- Make btf__load_from_kernel_by_id{,_split}() return the btf struct.
- Add new functions to v0.5.0 API (and not v0.6.0).

Quentin Monnet (5):
libbpf: rename btf__load() as btf__load_into_kernel()
libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()
tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()
libbpf: add split BTF support for btf__load_from_kernel_by_id()
tools: bpftool: support dumping split BTF by id

 tools/bpf/bpftool/btf.c                      |  8 ++---
 tools/bpf/bpftool/btf_dumper.c               |  6 ++--
 tools/bpf/bpftool/map.c                      | 16 +++++-----
 tools/bpf/bpftool/prog.c                     | 29 +++++++++++------
 tools/lib/bpf/btf.c                          | 33 ++++++++++++++------
 tools/lib/bpf/btf.h                          |  4 +++
 tools/lib/bpf/libbpf.c                       |  7 +++--
 tools/lib/bpf/libbpf.map                     |  3 ++
 tools/perf/util/bpf-event.c                  | 11 ++++---
 tools/perf/util/bpf_counter.c                | 12 +++++--
 tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
 11 files changed, 86 insertions(+), 47 deletions(-)

--
2.30.2

Quoted 5 lines · Quentin Monnet

As part of the effort to move towards a v1.0 for libbpf [0], this set improves some confusing function names related to BTF loading from and to the kernel:

Quoted 7 lines

- btf__load() becomes btf__load_into_kernel().
- btf__get_from_id becomes btf__load_from_kernel_by_id().
- A new version btf__load_from_kernel_by_id_split() extends the former to add support for split BTF.

The old functions are not removed or marked as deprecated yet, there should be in a future libbpf version.

Oh, and I was thinking about this whole deprecation having to be done in two steps. It's super annoying to keep track of that. Ideally, we'd have some macro that can mark API deprecated "in the future", when actual libbpf version is >= to defined version. So something like this:

LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6")

We'd need to make sure that during the build time we have some LIBBPF_VERSION macro available against which we compare the expected version and add or not the __attribute__((deprecated)).

Does this make sense? WDYT? I haven't looked into how hard it would be to implement this, but it should be easy enough, so if you'd like some macro challenge, please take a stab at it.

Having this it would be possible to make all the deprecations at the same time that we add replacement APIs and not ask anyone to follow-up potentially a month or two later, right?

Quoted 35 lines

The last patch is a trivial change to bpftool to add support for dumping split BTF objects by referencing them by their id (and not only by their BTF path).

[0] https://github.com/libbpf/libbpf/wiki/Libbpf:-the-road-to-v1.0#btfh-apis

v2:
- Remove deprecation marking of legacy functions (patch 4/6 from v1).
- Make btf__load_from_kernel_by_id{,_split}() return the btf struct.
- Add new functions to v0.5.0 API (and not v0.6.0).

Quentin Monnet (5):
libbpf: rename btf__load() as btf__load_into_kernel()
libbpf: rename btf__get_from_id() as btf__load_from_kernel_by_id()
tools: replace btf__get_from_id() with btf__load_from_kernel_by_id()
libbpf: add split BTF support for btf__load_from_kernel_by_id()
tools: bpftool: support dumping split BTF by id

 tools/bpf/bpftool/btf.c                      |  8 ++---
 tools/bpf/bpftool/btf_dumper.c               |  6 ++--
 tools/bpf/bpftool/map.c                      | 16 +++++-----
 tools/bpf/bpftool/prog.c                     | 29 +++++++++++------
 tools/lib/bpf/btf.c                          | 33 ++++++++++++++------
 tools/lib/bpf/btf.h                          |  4 +++
 tools/lib/bpf/libbpf.c                       |  7 +++--
 tools/lib/bpf/libbpf.map                     |  3 ++
 tools/perf/util/bpf-event.c                  | 11 ++++---
 tools/perf/util/bpf_counter.c                | 12 +++++--
 tools/testing/selftests/bpf/prog_tests/btf.c |  4 ++-
 11 files changed, 86 insertions(+), 47 deletions(-)

--
2.30.2

Quoted 21 lines · Andrii Nakryiko

Quoted 5 lines · Quentin Monnet

As part of the effort to move towards a v1.0 for libbpf [0], this set improves some confusing function names related to BTF loading from and to the kernel:

Quoted 7 lines

- btf__load() becomes btf__load_into_kernel().
- btf__get_from_id becomes btf__load_from_kernel_by_id().
- A new version btf__load_from_kernel_by_id_split() extends the former to add support for split BTF.

The old functions are not removed or marked as deprecated yet, there should be in a future libbpf version.

Oh, and I was thinking about this whole deprecation having to be done in two steps. It's super annoying to keep track of that. Ideally, we'd have some macro that can mark API deprecated "in the future", when actual libbpf version is >= to defined version. So something like this:

Quoted 1 line

LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6")

Better:

LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")

2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 23 lines

Quoted 21 lines · Andrii Nakryiko

Quoted 5 lines · Quentin Monnet

As part of the effort to move towards a v1.0 for libbpf [0], this set improves some confusing function names related to BTF loading from and to the kernel:

Quoted 7 lines

- btf__load() becomes btf__load_into_kernel().
- btf__get_from_id becomes btf__load_from_kernel_by_id().
- A new version btf__load_from_kernel_by_id_split() extends the former to add support for split BTF.

The old functions are not removed or marked as deprecated yet, there should be in a future libbpf version.

Oh, and I was thinking about this whole deprecation having to be done in two steps. It's super annoying to keep track of that. Ideally, we'd have some macro that can mark API deprecated "in the future", when actual libbpf version is >= to defined version. So something like this:

Quoted 5 lines

Quoted 1 line

LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6")

Better:

LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")

I was considering a very advanced feature called “opening a new GitHub

issue” to track this :). But the macro game sounds interesting, I'll look into it for next version.

One nit with LIBBPF_DEPRECATED_SINCE() is that the warning mentions a version (here v0.6) that we are unsure will exist (say we jump from v0.5 to v1.0). But I don't suppose that's a real issue.

Thanks for the feedback!
Quentin

Quoted 25 lines

2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 23 lines

Quoted 21 lines · Andrii Nakryiko

Quoted 5 lines · Quentin Monnet

As part of the effort to move towards a v1.0 for libbpf [0], this set improves some confusing function names related to BTF loading from and to the kernel:

Quoted 7 lines

- btf__load() becomes btf__load_into_kernel().
- btf__get_from_id becomes btf__load_from_kernel_by_id().
- A new version btf__load_from_kernel_by_id_split() extends the former to add support for split BTF.

The old functions are not removed or marked as deprecated yet, there should be in a future libbpf version.

Oh, and I was thinking about this whole deprecation having to be done in two steps. It's super annoying to keep track of that. Ideally, we'd have some macro that can mark API deprecated "in the future", when actual libbpf version is >= to defined version. So something like this:

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6")

Better:

LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")

2021-07-23 08:51 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 26 lines · Quentin Monnet

Quoted 25 lines

2021-07-22 19:45 UTC-0700 ~ Andrii Nakryiko <[email protected]>

Quoted 23 lines

Quoted 21 lines · Andrii Nakryiko

Quoted 5 lines · Quentin Monnet

As part of the effort to move towards a v1.0 for libbpf [0], this set improves some confusing function names related to BTF loading from and to the kernel:

Quoted 7 lines

- btf__load() becomes btf__load_into_kernel().
- btf__get_from_id becomes btf__load_from_kernel_by_id().
- A new version btf__load_from_kernel_by_id_split() extends the former to add support for split BTF.

The old functions are not removed or marked as deprecated yet, there should be in a future libbpf version.

Oh, and I was thinking about this whole deprecation having to be done in two steps. It's super annoying to keep track of that. Ideally, we'd have some macro that can mark API deprecated "in the future", when actual libbpf version is >= to defined version. So something like this:

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

LIBBPF_DEPRECATED_AFTER(V(0,5), "API that will be marked deprecated in v0.6")

Better:

LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")

So I've been looking into this, and it's not _that_ simple to do. Unless I missed something about preprocessing macros, I cannot bake a "#if" in a "#define", to have the attribute printed if and only if the current version is >= 0.6 in this example.

I've come up with something, but it is not optimal because I have to write a check and macros for each version number used with the LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part I guess we could generate a header with those macros from the Makefile and include it in libbpf_common.h, but that does not really look much cleaner to me.

Here's my current code, below - does it correspond to what you had in mind? Or did you think of something else?

------

tools/lib/bpf/Makefile +0 -0

index ec14aa725bb0..095d5dc30d50 100644

tools/lib/bpf/Makefile +2 -0

 MAKEFLAGS += --no-print-directory
 

@@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall
 override CFLAGS += $(INCLUDES)
 override CFLAGS += -fvisibility=hidden
 override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
+override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION)
+override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION)
 
 # flags specific for shared library
 SHLIB_FLAGS := -DSHARED -fPIC

tools/lib/bpf/btf.h +0 -0

index cf8490f95641..8b6b5442dbd8 100644

tools/lib/bpf/libbpf_common.h +6 -0

+
+/* Add checks for other versions below when planning deprecation of API symbols
+ * with the LIBBPF_DEPRECATED_SINCE macro.
+ */
+#if __LIBBPF_VERSION_CHECK(0, 6)
+#define __LIBBPF_MARK_DEPRECATED_0_6(X) X

tools/lib/bpf/libbpf_common.h +2 -0

+#endif /* LIBBPF_DEPRECATED_SINCE */
+
 /* Helper macro to declare and initialize libbpf options struct
  *
  * This dance with uninitialized declaration, followed by memset to zero,

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")

So I've been looking into this, and it's not _that_ simple to do. Unless I missed something about preprocessing macros, I cannot bake a "#if" in a "#define", to have the attribute printed if and only if the current version is >= 0.6 in this example.

Quoted 6 lines

I've come up with something, but it is not optimal because I have to write a check and macros for each version number used with the LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part I guess we could generate a header with those macros from the Makefile and include it in libbpf_common.h, but that does not really look much cleaner to me.

Yeah, let's not add unnecessary code generation. It sucks, of course, that we can't do #ifdef inside a macro :(

So it's either do something like what you did with defining version-specific macros, which is actually not too bad, because it's not like we have tons of those versions anyways.

LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead") LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);

Alternatively, we can go with:

#if LIBBPF_AT_OR_NEWER(0, 6) LIBBPF_DEPRECATED("use btf__load_from_kernel_by_id instead") #endif
LIBBPF API int btf__get_from_id(__u32 id, struct btf **btf);

I don't really dislike the second variant too much either, but LIBBPF_DEPRECATED_SINCE() reads nicer. Let's go with that. See some comments below about implementation.

Quoted 12 lines

Here's my current code, below - does it correspond to what you had in mind? Or did you think of something else?

------

tools/lib/bpf/Makefile +0 -0

index ec14aa725bb0..095d5dc30d50 100644

tools/lib/bpf/Makefile LIBBPF_VERSION := $(shell \ · line 11

Quoted 4 lines

tools/lib/bpf/Makefile +1 -0

@@ -8,6 +8,7 @@ LIBBPF_VERSION := $(shell \
        grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
        sort -rV | head -n1 | cut -d'_' -f2)
 LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION)))
+LIBBPF_MINOR_VERSION := $(firstword $(subst ., ,$(subst $(LIBBPF_MAJOR_VERSION)., ,$(LIBBPF_VERSION))))

Given all this is for internal use, I'd instead define something like __LIBBPF_CURVER as an integer that is easy to compare against:

#define __LIBBPF_CURVER (LIBBPF_MAJOR_VERSION * 100 + LIBBPF_MINOR_VERSION) * 100 + LIBBPF_PATCH_VERSION

That will simplify some stuff below and is generally easier to use in code, if we will need this somewhere to use explicitly.

Quoted 11 lines

tools/lib/bpf/Makefile +2 -0

 MAKEFLAGS += --no-print-directory
 

@@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall
 override CFLAGS += $(INCLUDES)
 override CFLAGS += -fvisibility=hidden
 override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
+override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION)
+override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION)
 

tools/lib/bpf/btf.h LIBBPF_API struct btf *btf__parse_raw(const char *path); · line 48

Quoted 12 lines

tools/lib/bpf/Makefile +0 -0

 # flags specific for shared library
 SHLIB_FLAGS := -DSHARED -fPIC

tools/lib/bpf/btf.h +1 -1

index cf8490f95641..8b6b5442dbd8 100644
@@ -45,7 +45,8 @@ LIBBPF_API struct btf *btf__parse_raw(const char *path);
 LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf);
 LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
 LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf);
-LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
+LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")

nit: given how long those deprecations will be, let's keep them at a separate (first) line and keep LIBBPF_API near the function declaration itself

Quoted 11 lines

tools/lib/bpf/btf.h +1 -0

+int btf__get_from_id(__u32 id, struct btf **btf);
 
 LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf);
 LIBBPF_API int btf__load(struct btf *btf);

tools/lib/bpf/libbpf_common.h +0 -0

index 947d8bd8a7bb..9ba9f8135dc8 100644
@@ -17,6 +17,28 @@
 

tools/lib/bpf/libbpf_common.h line 20

Quoted 3 lines

tools/lib/bpf/libbpf_common.h +1 -0

 #define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg)))
 
+#ifndef LIBBPF_DEPRECATED_SINCE

why #ifndef conditional?

tools/lib/bpf/libbpf_common.h line 23

Quoted 4 lines

tools/lib/bpf/libbpf_common.h +3 -0

+#define __LIBBPF_VERSION_CHECK(major, minor) \
+       LIBBPF_MAJOR_VERSION > major || \
+               (LIBBPF_MAJOR_VERSION == major && LIBBPF_MINOR_VERSION >= minor)

so we don't need this if we do __LIBBPF_CURVER

Quoted 12 lines

tools/lib/bpf/libbpf_common.h +11 -0

+
+/* Add checks for other versions below when planning deprecation of API symbols
+ * with the LIBBPF_DEPRECATED_SINCE macro.
+ */
+#if __LIBBPF_VERSION_CHECK(0, 6)
+#define __LIBBPF_MARK_DEPRECATED_0_6(X) X
+#else
+#define __LIBBPF_MARK_DEPRECATED_0_6(X)
+#endif
+
+#define __LIBBPF_DEPRECATED_SINCE(major, minor, msg) \

tools/lib/bpf/libbpf_common.h line 39

Quoted 5 lines

tools/lib/bpf/libbpf_common.h +5 -0

+       __LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor (LIBBPF_DEPRECATED("v" # major "." # minor "+, " msg))
+
+/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */
+#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
+       __LIBBPF_DEPRECATED_SINCE(major, minor, msg)

Is it needed for some macro value concatenation magic to have this nested __LIBBPF_DEPRECATED_SINCE?

Quoted 27 lines · Andrii Nakryiko

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

LIBBPF_DEPRECATED_SINCE(0, 6, "API that will be marked deprecated in v0.6")

So I've been looking into this, and it's not _that_ simple to do. Unless I missed something about preprocessing macros, I cannot bake a "#if" in a "#define", to have the attribute printed if and only if the current version is >= 0.6 in this example.

Quoted 6 lines

I've come up with something, but it is not optimal because I have to write a check and macros for each version number used with the LIBBPF_DEPRECATED_SINCE macro. If we really wanted to automate that part I guess we could generate a header with those macros from the Makefile and include it in libbpf_common.h, but that does not really look much cleaner to me.

Yeah, let's not add unnecessary code generation. It sucks, of course, that we can't do #ifdef inside a macro :(

So it's either do something like what you did with defining version-specific macros, which is actually not too bad, because it's not like we have tons of those versions anyways.

LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead") LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);

Alternatively, we can go with:

Quoted 8 lines

#if LIBBPF_AT_OR_NEWER(0, 6) LIBBPF_DEPRECATED("use btf__load_from_kernel_by_id instead") #endif
LIBBPF API int btf__get_from_id(__u32 id, struct btf **btf);

I don't really dislike the second variant too much either, but LIBBPF_DEPRECATED_SINCE() reads nicer. Let's go with that. See some comments below about implementation.

Ok.

Quoted 20 lines

Quoted 12 lines

Here's my current code, below - does it correspond to what you had in mind? Or did you think of something else?

------

tools/lib/bpf/Makefile +0 -0

index ec14aa725bb0..095d5dc30d50 100644

tools/lib/bpf/Makefile LIBBPF_VERSION := $(shell \ · line 11

Quoted 4 lines

tools/lib/bpf/Makefile +1 -0

@@ -8,6 +8,7 @@ LIBBPF_VERSION := $(shell \
        grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
        sort -rV | head -n1 | cut -d'_' -f2)
 LIBBPF_MAJOR_VERSION := $(firstword $(subst ., ,$(LIBBPF_VERSION)))
+LIBBPF_MINOR_VERSION := $(firstword $(subst ., ,$(subst $(LIBBPF_MAJOR_VERSION)., ,$(LIBBPF_VERSION))))

Given all this is for internal use, I'd instead define something like __LIBBPF_CURVER as an integer that is easy to compare against:

Quoted 5 lines

#define __LIBBPF_CURVER (LIBBPF_MAJOR_VERSION * 100 + LIBBPF_MINOR_VERSION) * 100 + LIBBPF_PATCH_VERSION

That will simplify some stuff below and is generally easier to use in code, if we will need this somewhere to use explicitly.

Did you mean computing __LIBBPF_CURVER in the Makefile, or in the header?

I can do that if you want, although I'm not convinced it will simplify much. Instead of having one long-ish condition, we'll have to compute the integer for the current version, as well as for each of the versions that we list for deprecating functions. I suppose I can add another dedicated macro.

Do you actually want the patch version? I chose to leave it aside because 1) I thought it would not be relevant for deprecating symbols, and 2) if anything like a -rc1 suffix is ever appended to the version, it makes it more complex to parse from the version string.

Quoted 12 lines

Quoted 12 lines

tools/lib/bpf/Makefile +2 -0

 MAKEFLAGS += --no-print-directory
 

@@ -86,6 +87,8 @@ override CFLAGS += -Werror -Wall
 override CFLAGS += $(INCLUDES)
 override CFLAGS += -fvisibility=hidden
 override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
+override CFLAGS += -DLIBBPF_MAJOR_VERSION=$(LIBBPF_MAJOR_VERSION)
+override CFLAGS += -DLIBBPF_MINOR_VERSION=$(LIBBPF_MINOR_VERSION)
 

Quoted 16 lines

tools/lib/bpf/btf.h LIBBPF_API struct btf *btf__parse_raw(const char *path); · line 48

Quoted 12 lines

tools/lib/bpf/Makefile +0 -0

 # flags specific for shared library
 SHLIB_FLAGS := -DSHARED -fPIC

tools/lib/bpf/btf.h +1 -1

index cf8490f95641..8b6b5442dbd8 100644
@@ -45,7 +45,8 @@ LIBBPF_API struct btf *btf__parse_raw(const char *path);
 LIBBPF_API struct btf *btf__parse_raw_split(const char *path, struct btf *base_btf);
 LIBBPF_API struct btf *btf__load_from_kernel_by_id(__u32 id);
 LIBBPF_API struct btf *btf__load_from_kernel_by_id_split(__u32 id, struct btf *base_btf);
-LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
+LIBBPF_API LIBBPF_DEPRECATED_SINCE(0, 6, "use btf__load_from_kernel_by_id instead")

nit: given how long those deprecations will be, let's keep them at a separate (first) line and keep LIBBPF_API near the function declaration itself

I thought having the LIBBPF_API on a separate line would slightly reduce the risk, when moving lines around, to move the function prototype but not the deprecation attribute. But ok, fine.

Quoted 12 lines

Quoted 12 lines

tools/lib/bpf/btf.h +1 -0

+int btf__get_from_id(__u32 id, struct btf **btf);
 
 LIBBPF_API int btf__finalize_data(struct bpf_object *obj, struct btf *btf);
 LIBBPF_API int btf__load(struct btf *btf);

tools/lib/bpf/libbpf_common.h +0 -0

index 947d8bd8a7bb..9ba9f8135dc8 100644
@@ -17,6 +17,28 @@
 

Quoted 5 lines

tools/lib/bpf/libbpf_common.h line 20

Quoted 3 lines

tools/lib/bpf/libbpf_common.h +1 -0

 #define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg)))
 
+#ifndef LIBBPF_DEPRECATED_SINCE

why #ifndef conditional?

Right, we don't expect to have the macro defined elsewhere. I'll remove it.

Quoted 5 lines

Quoted 4 lines

tools/lib/bpf/libbpf_common.h +3 -0

+#define __LIBBPF_VERSION_CHECK(major, minor) \
+       LIBBPF_MAJOR_VERSION > major || \
+               (LIBBPF_MAJOR_VERSION == major && LIBBPF_MINOR_VERSION >= minor)

Quoted 1 line

so we don't need this if we do __LIBBPF_CURVER

Right, but we do need to compute an integer for each of the versions listed below (0.6 for now). I'll see if I can come up with something short.

Quoted 13 lines

Quoted 13 lines

tools/lib/bpf/libbpf_common.h +11 -0

+
+/* Add checks for other versions below when planning deprecation of API symbols
+ * with the LIBBPF_DEPRECATED_SINCE macro.
+ */
+#if __LIBBPF_VERSION_CHECK(0, 6)
+#define __LIBBPF_MARK_DEPRECATED_0_6(X) X
+#else
+#define __LIBBPF_MARK_DEPRECATED_0_6(X)
+#endif
+
+#define __LIBBPF_DEPRECATED_SINCE(major, minor, msg) \

Quoted 8 lines

tools/lib/bpf/libbpf_common.h line 39

Quoted 5 lines

tools/lib/bpf/libbpf_common.h +5 -0

+       __LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor (LIBBPF_DEPRECATED("v" # major "." # minor "+, " msg))
+
+/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */
+#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
+       __LIBBPF_DEPRECATED_SINCE(major, minor, msg)

Is it needed for some macro value concatenation magic to have this nested __LIBBPF_DEPRECATED_SINCE?

I double-checked (I needed to, anyway), and it seems not. It's a leftover from an earlier version of my code, I'll clean it up before the proper submission.

Thanks!
Quentin