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 Wed Sep 17, 2025 at 7:44 AM +08, Andrii Nakryiko wrote:
> On Wed, Sep 10, 2025 at 9:28 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>> Introduce BPF_F_ALL_CPUS flag support for percpu_hash and lru_percpu_hash
>> maps to allow updating values for all CPUs with a single value for both
>> update_elem and update_batch APIs.
>>
>> Introduce BPF_F_CPU flag support for percpu_hash and lru_percpu_hash
>> maps to allow:
>>
>> * update value for specified CPU for both update_elem and update_batch
>> APIs.
>> * lookup value for specified CPU for both lookup_elem and lookup_batch
>> APIs.
>>
>> The BPF_F_CPU flag is passed via:
>>
>> * map_flags along with embedded cpu info.
>> * elem_flags along with embedded cpu info.
>>
>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>> ---
>>  include/linux/bpf.h  |  4 ++-
>>  kernel/bpf/hashtab.c | 77 +++++++++++++++++++++++++++++++-------------
>>  kernel/bpf/syscall.c |  2 +-
>>  3 files changed, 58 insertions(+), 25 deletions(-)
>>
>
> [...]
>
>> @@ -1147,7 +1158,7 @@ static long htab_map_update_elem(struct bpf_map *map, void *key, void *value,
>>         }
>>
>>         l_new = alloc_htab_elem(htab, key, value, key_size, hash, false, false,
>> -                               l_old);
>> +                               l_old, map_flags);
>>         if (IS_ERR(l_new)) {
>>                 /* all pre-allocated elements are in use or memory exhausted */
>>                 ret = PTR_ERR(l_new);
>> @@ -1263,7 +1274,7 @@ static long htab_map_update_elem_in_place(struct bpf_map *map, void *key,
>>         u32 key_size, hash;
>>         int ret;
>>
>> -       if (unlikely(map_flags > BPF_EXIST))
>> +       if (unlikely(!onallcpus && map_flags > BPF_EXIST))
>
> BPF_F_LOCK shouldn't be let through
>

Ack.

>>                 /* unknown flags */
>>                 return -EINVAL;
>>
>
> [...]
>
>> @@ -1698,9 +1709,16 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
>>         int ret = 0;
>>
>>         elem_map_flags = attr->batch.elem_flags;
>> -       if ((elem_map_flags & ~BPF_F_LOCK) ||
>> -           ((elem_map_flags & BPF_F_LOCK) && !btf_record_has_field(map->record, BPF_SPIN_LOCK)))
>> -               return -EINVAL;
>> +       if (!do_delete && is_percpu) {
>> +               ret = bpf_map_check_op_flags(map, elem_map_flags, BPF_F_LOCK | BPF_F_CPU);
>> +               if (ret)
>> +                       return ret;
>> +       } else {
>> +               if ((elem_map_flags & ~BPF_F_LOCK) ||
>> +                   ((elem_map_flags & BPF_F_LOCK) &&
>> +                    !btf_record_has_field(map->record, BPF_SPIN_LOCK)))
>> +                       return -EINVAL;
>> +       }
>
> partially open-coded bpf_map_check_op_flags() if `do_delete ||
> !is_percpu`, right? Have you considered
>
> u32 allowed_flags = 0;
>
> ...
>
> allowed_flags = BPF_F_LOCK | BPF_F_CPU;
> if (do_delete || !is_percpu)
>     allowed_flags ~= BPF_F_CPU;
> err = bpf_map_check_op_flags(map, elem_map_flags, allowed_flags);
>
>
> This reads way more natural (in my head...), and no open-coding the
> helper you just so painstakingly extracted and extended to check all
> these conditions.
>

My intention was to call bpf_map_check_op_flags() only for lookup_batch
on *percpu* hash maps, while excluding lookup_batch on non-percpu hash
maps and the lookup_and_delete_batch API.

I don’t think we should be checking op flags for non-percpu hash maps or
for lookup_and_delete_batch cases.

>>
>>         map_flags = attr->batch.flags;
>>         if (map_flags)
>> @@ -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();
>
> ?
>

Right, good catch.

Thanks,
Leon