> On Jun 5, 2019, at 4:54 AM, Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 06/05/2019 02:18 AM, Hechao Li wrote:
>> 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
>
>> 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
>>
>>
>
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
