Re: [PATCH bpf-next v8 5/7] bpf: Add BPF_F_CPU and BPF_F_ALL_CPUS flags support for percpu_cgroup_storage maps

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

On 2025/9/28 10:42, Andrii Nakryiko wrote:
> On Thu, Sep 25, 2025 at 8:38 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>> Introduce BPF_F_ALL_CPUS flag support for percpu_cgroup_storage maps to
>> allow updating values for all CPUs with a single value for update_elem
>> API.
>>
>> Introduce BPF_F_CPU flag support for percpu_cgroup_storage maps to
>> allow:
>>
>> * update value for specified CPU for update_elem API.
>> * lookup value for specified CPU for lookup_elem API.
>>
>> The BPF_F_CPU flag is passed via map_flags along with embedded cpu info.
>>
>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>> ---
>>  include/linux/bpf-cgroup.h |  4 ++--
>>  include/linux/bpf.h        |  1 +
>>  kernel/bpf/local_storage.c | 22 +++++++++++++++++++---
>>  kernel/bpf/syscall.c       |  2 +-
>>  4 files changed, 23 insertions(+), 6 deletions(-)
>>
>
> [...]
>
>>  int bpf_percpu_cgroup_storage_copy(struct bpf_map *_map, void *key,
>> -                                  void *value)
>> +                                  void *value, u64 map_flags)
>>  {
>>         struct bpf_cgroup_storage_map *map = map_to_storage(_map);
>>         struct bpf_cgroup_storage *storage;
>> @@ -199,11 +199,17 @@ int bpf_percpu_cgroup_storage_copy(struct bpf_map *_map, void *key,
>>          * will not leak any kernel data
>>          */
>>         size = round_up(_map->value_size, 8);
>
> um... same issue with rounding up value_size when BPF_F_CPU is set, no?
>
>> +       if (map_flags & BPF_F_CPU) {
>> +               cpu = map_flags >> 32;
>> +               bpf_long_memcpy(value, per_cpu_ptr(storage->percpu_buf, cpu), size);
>> +               goto unlock;
>> +       }
>>         for_each_possible_cpu(cpu) {
>>                 bpf_long_memcpy(value + off,
>>                                 per_cpu_ptr(storage->percpu_buf, cpu), size);
>>                 off += size;
>>         }
>> +unlock:
>>         rcu_read_unlock();
>>         return 0;
>>  }
>> @@ -216,7 +222,7 @@ int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *key,
>>         int cpu, off = 0;
>>         u32 size;
>>
>> -       if (map_flags != BPF_ANY && map_flags != BPF_EXIST)
>> +       if ((u32)map_flags & ~(BPF_ANY | BPF_EXIST | BPF_F_CPU | BPF_F_ALL_CPUS))
>>                 return -EINVAL;
>>
>>         rcu_read_lock();
>> @@ -233,11 +239,21 @@ int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *key,
>>          * so no kernel data leaks possible
>>          */
>>         size = round_up(_map->value_size, 8);
>> +       if (map_flags & BPF_F_CPU) {
>> +               cpu = map_flags >> 32;
>> +               bpf_long_memcpy(per_cpu_ptr(storage->percpu_buf, cpu), value, size);
>
> ditto
>
>> +               goto unlock;
>> +       }
>>         for_each_possible_cpu(cpu) {
>>                 bpf_long_memcpy(per_cpu_ptr(storage->percpu_buf, cpu),
>>                                 value + off, size);
>> -               off += size;
>> +               /* same user-provided value is used if BPF_F_ALL_CPUS is
>> +                * specified, otherwise value is an array of per-CPU values.
>> +                */
>> +               if (!(map_flags & BPF_F_ALL_CPUS))
>> +                       off += size;
>
> btw, given we'll need another revision to fix up all those round_up()
> issues, what do you think about make this offset logic completely
> stateless (and, in my opinion, more obvious):
>
> for_each_possible_cpu(cpu) {
>     p = (map_flags & BPF_F_ALL_CPUS) ? value : value + size * cpu;
>     memcpy(per_cpu_ptr(storage->percpu_buf, cpu), p, size);
> }
>
> seems more straightforward to me

lgtm.

But I think the correct memcpy() should look like this:

memcpy(per_cpu_ptr(storage->percpu_buf, cpu), p,
       (map_flags & BPF_F_ALL_CPUS) ? _map->value_size : size);

because 'size' is 8-byte aligned and can’t be used directly when
'map_flags & BPF_F_ALL_CPUS' is set.

So the more accurate version would be:

for_each_possible_cpu(cpu) {
    p = (map_flags & BPF_F_ALL_CPUS) ? value : value + size * cpu;
    s = (map_flags & BPF_F_ALL_CPUS) ? _map->value_size : size;
    memcpy(per_cpu_ptr(storage->percpu_buf, cpu), p, s);
}

Isn’t this the correct approach?

Thanks,
Leon

[...]