Re: [PATCH bpf-next v7 4/7] bpf: Add BPF_F_CPU and BPF_F_ALL_CPUS flags support for percpu_hash and lru_percpu_hash maps

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

On 23/9/25 00:13, Andrii Nakryiko wrote:
> On Mon, Sep 22, 2025 at 7:50 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>> On Sat Sep 20, 2025 at 6:31 AM +08, Andrii Nakryiko wrote:
>>> On Thu, Sep 18, 2025 at 10:25 PM Leon Hwang <leon.hwang@linux.dev> wrote:
>>>>
>>>>
>>>>
>>>>>> @@ -1724,7 +1742,7 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
>>>>>>         value_size = htab->map.value_size;
>>>>>>         size = round_up(value_size, 8);
>>>>>>         if (is_percpu)
>>>>>> -               value_size = size * num_possible_cpus();
>>>>>> +               value_size = (elem_map_flags & BPF_F_CPU) ? size : size * num_possible_cpus();
>>>>>
>>>>> if (is_percpu && !(elem_map_flags & BPF_F_CPU))
>>>>>     value_size = size * num_possible_cpus();
>>>>>
>>>>> ?
>>>>>
>>>>
>>>> After looking at it again, I’d like to keep my approach.
>>>>
>>>> When 'elem_map_flags & BPF_F_CPU' is set, 'value_size' has to be
>>>> assigned to 'size' ('round_up(value_size, 8)') instead of keeping
>>>> 'htab->map.value_size'.
>>>>
>>>
>>> isn't that what will happen here as well? There is
>>>
>>> size = round_up(value_size, 8);
>>>
>>> right before that if
>>>
>>
>> As for percpu maps, both 'size' and 'value_size' need to be 8-byte
>> aligned here, because 'map.value_size' itself is not guarenteed to be
>> aligned.
>>
>> In 'htab_map_alloc_check()', there is no alignment check for percpu
>> maps.
>>
>> So 'map.value_size' can be unaligned.
>>
>> Let's look at how 'value_size' is used:
>>
>> values = kvmalloc_array(value_size, bucket_size, GFP_USER | __GFP_NOWARN);
>> dst_val = values;
>> hlist_nulls_for_each_entry_safe(l, n, head, hash_node) {
>>         if (is_percpu) {
>>                 if (elem_map_flags & BPF_F_CPU) {
>>                         copy_map_value_long(&htab->map, dst_val, per_cpu_ptr(pptr, cpu));
>>                 }
>>         }
>>         dst_val += value_size;
>> }
>> copy_to_user(uvalues + total * value_size, values,
>>              value_size * bucket_cnt)
>>
>> Here, 'value_size' determines how values are laid out and copied.
>>
>
> So in my mind (and maybe it's wrong, tell me), BPF_F_CPU turns a
> per-CPU map lookup into an effectively non-per-cpu one. So I'm not
> sure we need to do 8 byte alignment of value/key sizes when BPF_F_CPU
> is specified.
>
> But if people would like to keep 8 byte alignment anyways for
> BPF_F_CPU, that's fine too, I guess.
>

'value_size' should be 8-byte aligned here.

For example, if 'value_size' is *1* when BPF_F_CPU is specified:

values = kvmalloc_array();  /* 5 bytes (value_size * bucket_size) memory */
copy_map_value_long();      /* copies 8 bytes, writing past the
                               allocated 5 bytes of memory */

To stay consistent with 'copy_map_value_long()', 'value_size' itself
needs to be 8-byte aligned.

That leaves us with two options:

1. Keep 'value_size' unaligned, switch 'copy_map_value_long()' to
   'copy_map_value()'.
2. Require 'value_size' to be 8-byte aligned.

WDYT?

Thanks,
Leon