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