Re: [PATCH 0/2] Move bpf_num_possible_cpus() to libbpf_util

[Hechao Li <hechaol@xxxxxx>]

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" <daniel@xxxxxxxxxxxxx> 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" <songliubraving@xxxxxx> wrote:
    > 
    >     
    >     > On Jun 4, 2019, at 3:38 PM, Hechao Li <hechaol@xxxxxx> 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