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

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

[Song Liu]

> On Jun 4, 2019, at 3:38 PM, Hechao Li <hechaol@fb.com> 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? 

Thanks,
Song

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

[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

On 6/4/19, 4:40 PM, "Song Liu" <songliubraving@fb.com> wrote:

    
    > On Jun 4, 2019, at 3:38 PM, Hechao Li <hechaol@fb.com> 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? 
    
    Thanks,
    Song
    
    
    
    

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

[Daniel Borkmann]

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@fb.com> wrote:
> 
>     
>     > On Jun 4, 2019, at 3:38 PM, Hechao Li <hechaol@fb.com> 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

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

[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.

Thanks,
Hechao

On 6/4/19, 5:08 PM, "Daniel Borkmann" <daniel@iogearbox.net> 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@fb.com> wrote:
    > 
    >     
    >     > On Jun 4, 2019, at 3:38 PM, Hechao Li <hechaol@fb.com> 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
    

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

[Daniel Borkmann]

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@iogearbox.net> 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@fb.com> wrote:
>     > 
>     >     
>     >     > On Jun 4, 2019, at 3:38 PM, Hechao Li <hechaol@fb.com> 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
>     
> 

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

[Hechao Li]

> On Jun 5, 2019, at 4:54 AM, Daniel Borkmann <daniel@iogearbox.net> 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@iogearbox.net> 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@fb.com> wrote:
>>>
>>>
>>>> On Jun 4, 2019, at 3:38 PM, Hechao Li <hechaol@fb.com> 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

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

[Andrii Nakryiko]

On Wed, Jun 5, 2019 at 10:41 AM Hechao Li <hechaol@fb.com> wrote:
>
>
> > On Jun 5, 2019, at 4:54 AM, Daniel Borkmann <daniel@iogearbox.net> 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@iogearbox.net> 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@fb.com> wrote:
> >>>
> >>>
> >>>> On Jun 4, 2019, at 3:38 PM, Hechao Li <hechaol@fb.com> 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?
>

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.

> Thanks,
> Hechao
>

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

[Daniel Borkmann]

On 06/05/2019 08:55 PM, Andrii Nakryiko wrote:
> On Wed, Jun 5, 2019 at 10:41 AM Hechao Li <hechaol@fb.com> wrote:
[...]
>> 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?
> 
> 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.

That hack is basically for the case where a map value is not a multiple of
8 bytes, meaning without padding the lookup could corrupt the application; I
don't think users are aware of this w/o looking at the kernel implementation,
but I'm fine with leaving it as is, we could always migrate it later if needed.

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

Yes, definitely, we cannot exit out of the lib.