Move bpf_num_possible_cpus() to libbpf_util

Getting number of possible CPUs is commonly used for per-CPU BPF maps and perf_event_maps. Putting it into a common place can avoid duplicate implementations.

Hechao Li (2):
Add bpf_num_possible_cpus to libbpf_util Use bpf_num_possible_cpus in bpftool and selftests

tools/bpf/bpftool/common.c                    | 53 ++--------------
tools/lib/bpf/Build                           |  2 +-
tools/lib/bpf/libbpf_util.c                   | 61 +++++++++++++++++++
tools/lib/bpf/libbpf_util.h                   |  7 +++
tools/testing/selftests/bpf/bpf_util.h        | 42 +++----------
.../selftests/bpf/prog_tests/l4lb_all.c       |  2 +-
.../selftests/bpf/prog_tests/xdp_noinline.c   |  2 +-
tools/testing/selftests/bpf/test_btf.c        |  2 +-
tools/testing/selftests/bpf/test_lru_map.c    |  2 +-
tools/testing/selftests/bpf/test_maps.c       |  6 +-
10 files changed, 88 insertions(+), 91 deletions(-)

create mode 100644 tools/lib/bpf/libbpf_util.c

Signature · 1 line

2.17.1

On Jun 4, 2019, at 3:38 PM, Hechao Li <[email protected]> wrote:

Getting number of possible CPUs is commonly used for per-CPU BPF maps and perf_event_maps. Putting it into a common place can avoid duplicate implementations.

Hechao Li (2):
Add bpf_num_possible_cpus to libbpf_util Use bpf_num_possible_cpus in bpftool and selftests

tools/bpf/bpftool/common.c                    | 53 ++--------------
tools/lib/bpf/Build                           |  2 +-
tools/lib/bpf/libbpf_util.c                   | 61 +++++++++++++++++++
tools/lib/bpf/libbpf_util.h                   |  7 +++
tools/testing/selftests/bpf/bpf_util.h        | 42 +++----------
.../selftests/bpf/prog_tests/l4lb_all.c       |  2 +-
.../selftests/bpf/prog_tests/xdp_noinline.c   |  2 +-
tools/testing/selftests/bpf/test_btf.c        |  2 +-
tools/testing/selftests/bpf/test_lru_map.c    |  2 +-
tools/testing/selftests/bpf/test_maps.c       |  6 +-
10 files changed, 88 insertions(+), 91 deletions(-)

create mode 100644 tools/lib/bpf/libbpf_util.c

Signature · 1 line

2.17.1

I put the implementation in libbpf_util.c mainly because it depends on pr_warning defined in libbpf_internal.h. If including libbpf_internal.h in libbpf_util.h, then the internal stuff will be exposed to whoever include libbpf_util.h. But let me know if there is a better way to print the error messages other than depending on libbpf_internal.

Thanks,
Hechao

Quoted 21 lines · Song Liu

On Jun 4, 2019, at 3:38 PM, Hechao Li <[email protected]> wrote:

Getting number of possible CPUs is commonly used for per-CPU BPF maps and perf_event_maps. Putting it into a common place can avoid duplicate implementations.

Hechao Li (2):
Add bpf_num_possible_cpus to libbpf_util Use bpf_num_possible_cpus in bpftool and selftests

tools/bpf/bpftool/common.c                    | 53 ++--------------
tools/lib/bpf/Build                           |  2 +-
tools/lib/bpf/libbpf_util.c                   | 61 +++++++++++++++++++
tools/lib/bpf/libbpf_util.h                   |  7 +++
tools/testing/selftests/bpf/bpf_util.h        | 42 +++----------
.../selftests/bpf/prog_tests/l4lb_all.c       |  2 +-
.../selftests/bpf/prog_tests/xdp_noinline.c   |  2 +-
tools/testing/selftests/bpf/test_btf.c        |  2 +-
tools/testing/selftests/bpf/test_lru_map.c    |  2 +-
tools/testing/selftests/bpf/test_maps.c       |  6 +-
10 files changed, 88 insertions(+), 91 deletions(-)

Quoted 5 lines

create mode 100644 tools/lib/bpf/libbpf_util.c

Signature · 1 line

2.17.1

The change is mostly straightforward. However, I am not sure whether they should be added to libbpf_util.h. Maybe libbpf.h is a better place?

Daniel and Alexei, what's your recommendation here?

I looked into current public APIs in libbpf.h and bpf.h. Most of them seem to be directly related to bpf object/program/map. But this function, bpf_num_possible_cpus(), is just a utility used while looking up per-CPU maps. I am not sure if it is appropriate to make it an official API. Yonghong, the author of libbpf_util.h, also asked me to put it into libbpf_util. But I am fine with either way. I can move it to libbpf.h/.c if you all agree.

Thanks,
Hechao

On 6/4/19, 5:08 PM, "Daniel Borkmann" <[email protected]> wrote:

    On 06/05/2019 01:54 AM, Hechao Li wrote:
    > I put the implementation in libbpf_util.c mainly because it depends on pr_warning defined in libbpf_internal.h. If including libbpf_internal.h in libbpf_util.h, then the internal stuff will be exposed to whoever include libbpf_util.h. But let me know if there is a better way to print the error messages other than depending on libbpf_internal. 
    > 
    > Thanks,
    > Hechao
    > 
    > On 6/4/19, 4:40 PM, "Song Liu" <[email protected]> wrote:
    > 
    >     
    >     > On Jun 4, 2019, at 3:38 PM, Hechao Li <[email protected]> wrote:
    >     > 
    >     > Getting number of possible CPUs is commonly used for per-CPU BPF maps 
    >     > and perf_event_maps. Putting it into a common place can avoid duplicate 
    >     > implementations.
    >     > 
    >     > Hechao Li (2):
    >     >  Add bpf_num_possible_cpus to libbpf_util
    >     >  Use bpf_num_possible_cpus in bpftool and selftests
    >     > 
    >     > tools/bpf/bpftool/common.c                    | 53 ++--------------
    >     > tools/lib/bpf/Build                           |  2 +-
    >     > tools/lib/bpf/libbpf_util.c                   | 61 +++++++++++++++++++
    >     > tools/lib/bpf/libbpf_util.h                   |  7 +++
    >     > tools/testing/selftests/bpf/bpf_util.h        | 42 +++----------
    >     > .../selftests/bpf/prog_tests/l4lb_all.c       |  2 +-
    >     > .../selftests/bpf/prog_tests/xdp_noinline.c   |  2 +-
    >     > tools/testing/selftests/bpf/test_btf.c        |  2 +-
    >     > tools/testing/selftests/bpf/test_lru_map.c    |  2 +-
    >     > tools/testing/selftests/bpf/test_maps.c       |  6 +-
    >     > 10 files changed, 88 insertions(+), 91 deletions(-)
    >     > create mode 100644 tools/lib/bpf/libbpf_util.c
    >     > 
    >     > -- 
    >     > 2.17.1
    >     > 
    >     
    >     The change is mostly straightforward. However, I am not sure whether
    >     they should be added to libbpf_util.h. Maybe libbpf.h is a better 
    >     place?
    >     
    >     Daniel and Alexei, what's your recommendation here? 
    
    Hm, looks like the patch did not make it to the list (yet?). Agree it makes
    sense to move it into libbpf given common use for per-CPU/perf-event maps.
    Given from the diff stat it's not added to libbpf.map, is there a reason to
    not add it to, say, tools/lib/bpf/libbpf.c and expose it as official API?
    
    Thanks,
    Daniel

Quoted 1 line · Hechao Li

I looked into current public APIs in libbpf.h and bpf.h. Most of them seem to be directly related to bpf object/program/map. But this function, bpf_num_possible_cpus(), is just a utility used while looking up per-CPU maps. I am not sure if it is appropriate to make it an official API. Yonghong, the author of libbpf_util.h, also asked me to put it into libbpf_util. But I am fine with either way. I can move it to libbpf.h/.c if you all agree.

(please avoid top-posting)

It's a good question, I think it depends how much we want to aide users consuming libbpf that are using per-CPU maps, for example. If we only want to reuse it for in-tree selftests, it's fine to keep it in an unexposed internal header that selftests would include. Other option could be to expose and prefix as libbpf_num_possible_cpus() to denote it's a misc helper and perhaps also move f3515b5d0b71 ("bpf: provide a generic macro for percpu values for selftests") into libbpf. I'd be fine either way, my preference is to add it as an libbpf_ API given users would need something along these lines when walking the value anyway. See e00c7b216f34 ("bpf: fix multiple issues in selftest suite and samples") for context on why this helper was added and sysconf(_SC_NPROCESSORS_CONF) use would be broken in this context.

Thanks,
Daniel

Quoted 56 lines

Thanks,
Hechao

On 6/4/19, 5:08 PM, "Daniel Borkmann" <[email protected]> wrote:

Quoted 35 lines · Hechao Li

I put the implementation in libbpf_util.c mainly because it depends on pr_warning defined in libbpf_internal.h. If including libbpf_internal.h in libbpf_util.h, then the internal stuff will be exposed to whoever include libbpf_util.h. But let me know if there is a better way to print the error messages other than depending on libbpf_internal.

Thanks,
Hechao

Quoted 21 lines · Song Liu

On Jun 4, 2019, at 3:38 PM, Hechao Li <[email protected]> wrote:

Getting number of possible CPUs is commonly used for per-CPU BPF maps and perf_event_maps. Putting it into a common place can avoid duplicate implementations.

Hechao Li (2):
Add bpf_num_possible_cpus to libbpf_util Use bpf_num_possible_cpus in bpftool and selftests

tools/bpf/bpftool/common.c                    | 53 ++--------------
tools/lib/bpf/Build                           |  2 +-
tools/lib/bpf/libbpf_util.c                   | 61 +++++++++++++++++++
tools/lib/bpf/libbpf_util.h                   |  7 +++
tools/testing/selftests/bpf/bpf_util.h        | 42 +++----------
.../selftests/bpf/prog_tests/l4lb_all.c       |  2 +-
.../selftests/bpf/prog_tests/xdp_noinline.c   |  2 +-
tools/testing/selftests/bpf/test_btf.c        |  2 +-
tools/testing/selftests/bpf/test_lru_map.c    |  2 +-
tools/testing/selftests/bpf/test_maps.c       |  6 +-
10 files changed, 88 insertions(+), 91 deletions(-)

Quoted 5 lines

create mode 100644 tools/lib/bpf/libbpf_util.c

--
2.17.1

Quoted 5 lines

The change is mostly straightforward. However, I am not sure whether they should be added to libbpf_util.h. Maybe libbpf.h is a better place?

Daniel and Alexei, what's your recommendation here?

Hm, looks like the patch did not make it to the list (yet?). Agree it makes sense to move it into libbpf given common use for per-CPU/perf-event maps. Given from the diff stat it's not added to libbpf.map, is there a reason to not add it to, say, tools/lib/bpf/libbpf.c and expose it as official API?

Thanks,
Daniel

Quoted 20 lines · Daniel Borkmann

Quoted 1 line · Hechao Li

I looked into current public APIs in libbpf.h and bpf.h. Most of them seem to be directly related to bpf object/program/map. But this function, bpf_num_possible_cpus(), is just a utility used while looking up per-CPU maps. I am not sure if it is appropriate to make it an official API. Yonghong, the author of libbpf_util.h, also asked me to put it into libbpf_util. But I am fine with either way. I can move it to libbpf.h/.c if you all agree.

(please avoid top-posting)

It's a good question, I think it depends how much we want to aide users consuming libbpf that are using per-CPU maps, for example. If we only want to reuse it for in-tree selftests, it's fine to keep it in an unexposed internal header that selftests would include. Other option could be to expose and prefix as libbpf_num_possible_cpus() to denote it's a misc helper and perhaps also move f3515b5d0b71 ("bpf: provide a generic macro for percpu values for selftests") into libbpf. I'd be fine either way, my preference is to add it as an libbpf_ API given users would need something along these lines when walking the value anyway. See e00c7b216f34 ("bpf: fix multiple issues in selftest suite and samples") for context on why this helper was added and sysconf(_SC_NPROCESSORS_CONF) use would be broken in this context.

Quoted 60 lines

Thanks,
Daniel

Quoted 56 lines

Thanks,
Hechao

On 6/4/19, 5:08 PM, "Daniel Borkmann" <[email protected]> wrote:

Quoted 35 lines · Hechao Li

I put the implementation in libbpf_util.c mainly because it depends on pr_warning defined in libbpf_internal.h. If including libbpf_internal.h in libbpf_util.h, then the internal stuff will be exposed to whoever include libbpf_util.h. But let me know if there is a better way to print the error messages other than depending on libbpf_internal.

Thanks,
Hechao

Quoted 21 lines · Song Liu

On Jun 4, 2019, at 3:38 PM, Hechao Li <[email protected]> wrote:

Getting number of possible CPUs is commonly used for per-CPU BPF maps and perf_event_maps. Putting it into a common place can avoid duplicate implementations.

Hechao Li (2):
Add bpf_num_possible_cpus to libbpf_util Use bpf_num_possible_cpus in bpftool and selftests

tools/bpf/bpftool/common.c                    | 53 ++--------------
tools/lib/bpf/Build                           |  2 +-
tools/lib/bpf/libbpf_util.c                   | 61 +++++++++++++++++++
tools/lib/bpf/libbpf_util.h                   |  7 +++
tools/testing/selftests/bpf/bpf_util.h        | 42 +++----------
.../selftests/bpf/prog_tests/l4lb_all.c       |  2 +-
.../selftests/bpf/prog_tests/xdp_noinline.c   |  2 +-
tools/testing/selftests/bpf/test_btf.c        |  2 +-
tools/testing/selftests/bpf/test_lru_map.c    |  2 +-
tools/testing/selftests/bpf/test_maps.c       |  6 +-
10 files changed, 88 insertions(+), 91 deletions(-)

Quoted 5 lines

create mode 100644 tools/lib/bpf/libbpf_util.c

--
2.17.1

Quoted 5 lines

The change is mostly straightforward. However, I am not sure whether they should be added to libbpf_util.h. Maybe libbpf.h is a better place?

Daniel and Alexei, what's your recommendation here?

Hm, looks like the patch did not make it to the list (yet?). Agree it makes sense to move it into libbpf given common use for per-CPU/perf-event maps. Given from the diff stat it's not added to libbpf.map, is there a reason to not add it to, say, tools/lib/bpf/libbpf.c and expose it as official API?

Thanks,
Daniel

Thanks a lot for the detailed explanation, Daniel. And sorry for the reply format. Sure, I will add it as a libbpf_ API instead. Moving the macro BPF_DECLARE_PERCPU in selftest util to libbpf also makes sense to me. However, since bpf_num_possible_cpus in selftest exits the process in case of failures, which is not good for a user facing API, how about making #CPU a param and define it as

#define BPF_DECLARE_PERCPU(type, name, ncpu) \
struct { type v; } __bpf_percpu_val_align name[ncpu]

And the user should do
int ncpu = libbpf_num_possible_cpus();
// error handling if ncpu <=0
BPF_DECLARE_PERCPU(long, value, ncpu)

The problem of this method is, the user may still pass sysconf(_SC_NPROCESSORS_CONF) as ncpu. I think this can be avoided by putting some comments around this macro. Does it make sense?

Thanks,
Hechao

On Wed, Jun 5, 2019 at 10:41 AM Hechao Li <[email protected]> wrote:

Quoted 13 lines

Thanks a lot for the detailed explanation, Daniel. And sorry for the reply format. Sure, I will add it as a libbpf_ API instead. Moving the macro BPF_DECLARE_PERCPU in selftest util to libbpf also makes sense to me. However, since bpf_num_possible_cpus in selftest exits the process in case of failures, which is not good for a user facing API, how about making #CPU a param and define it as

#define BPF_DECLARE_PERCPU(type, name, ncpu) \
struct { type v; } __bpf_percpu_val_align name[ncpu]

And the user should do
int ncpu = libbpf_num_possible_cpus();
// error handling if ncpu <=0
BPF_DECLARE_PERCPU(long, value, ncpu)

Quoted 2 lines

The problem of this method is, the user may still pass sysconf(_SC_NPROCESSORS_CONF) as ncpu. I think this can be avoided by putting some comments around this macro. Does it make sense?

BPF_DECLARE_PERCPU doesn't do anything fancy and is used only from single selftest, so I'd keep it where it is right now. There is no need to pollute libbpf_internal.h with lots of stuff, if it's not widely used.

For libbpf_num_possible_cpus() - definitely change it to return int with <0 on error.