On 4/9/25 07:53, Andrii Nakryiko wrote:
> On Wed, Sep 3, 2025 at 7:27 AM Leon Hwang <leon.hwang@xxxxxxxxx> 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@xxxxxxxxx> 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
