Re: [PATCH bpf-next v4 2/7] bpf: Introduce BPF_F_CPU and BPF_F_ALL_CPUS flags

[Leon Hwang <leon.hwang@linux.dev>]

On 4/9/25 07:53, Andrii Nakryiko wrote:
> On Wed, Sep 3, 2025 at 7:27 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>> On Thu Aug 28, 2025 at 7:18 AM +08, Andrii Nakryiko wrote:
>>> On Wed, Aug 27, 2025 at 9:45 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>>>
>>
>> [...]
>>
>>>>
>>>> +#ifdef CONFIG_BPF_SYSCALL
>>>> +static inline void bpf_percpu_copy_to_user(struct bpf_map *map, void __percpu *pptr, void *value,
>>>> +                                          u32 size, u64 flags)
>>>> +{
>>>> +       int current_cpu = raw_smp_processor_id();
>>>> +       int cpu, off = 0;
>>>> +
>>>> +       if (flags & BPF_F_CPU) {
>>>> +               cpu = flags >> 32;
>>>> +               copy_map_value_long(map, value, cpu != current_cpu ? per_cpu_ptr(pptr, cpu) :
>>>> +                                   this_cpu_ptr(pptr));
>>>> +               check_and_init_map_value(map, value);
>>>
>>> I'm not sure it's the question to you, but why would we
>>> "check_and_init_map_value" when copying data to user space?... this is
>>> so confusing...
>>>
>>
>> After reading its code, I think it's to hide some kernel details from
>> user space, e.g. refcount, list nodes, rb nodes.
> 
> we don't copy those details, so there is nothing to hide, so no, I
> think it's just weird that we do this, unless there is some
> non-obvious reasoning behind this
> 

Ack.

check_and_init_map_value() is useless here.

>>
>>>> +       } else {
>>>> +               for_each_possible_cpu(cpu) {
>>>> +                       copy_map_value_long(map, value + off, per_cpu_ptr(pptr, cpu));
>>>> +                       check_and_init_map_value(map, value + off);
>>>> +                       off += size;
>>>> +               }
>>>> +       }
>>>> +}
>>>> +
>>>> +void bpf_obj_free_fields(const struct btf_record *rec, void *obj);
>>>> +
>>>> +static inline void bpf_percpu_copy_from_user(struct bpf_map *map, void __percpu *pptr, void *value,
>>>> +                                            u32 size, u64 flags)
>>>> +{
>>
>> [...]
>>
>>>> +}
>>>> +#endif
>>>
>>> hm... these helpers are just here with no way to validate that they
>>> generalize existing logic correctly... Do a separate patch where you
>>> introduce this helper before adding per-CPU flags *and* make use of
>>> them in existing code? Then we can check that you didn't introduce any
>>> subtle differences? Then in this patch you can adjust helpers to
>>> handle BPF_F_CPU and BPF_F_ALL_CPUS?
>>>
>>
>> Get it.
>>
>> I'll send a separate patch later.
> 
> separate patch as part of the patch set to show the value of this refactoring :)
> 

Sorry for my misunderstanding. :/

Thanks,
Leon