Re: [PATCH bpf-next v9 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 7/10/25 06:33, Andrii Nakryiko wrote:
> On Tue, Sep 30, 2025 at 8:40 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>
>> ---

[...]

>>  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;
>> @@ -198,12 +198,18 @@ int bpf_percpu_cgroup_storage_copy(struct bpf_map *_map, void *key,
>>          * access 'value_size' of them, so copying rounded areas
>>          * will not leak any kernel data
>>          */
>> +       if (map_flags & BPF_F_CPU) {
>> +               cpu = map_flags >> 32;
>> +               memcpy(value, per_cpu_ptr(storage->percpu_buf, cpu), _map->value_size);
> 
> this is so far ok, because we don't seem to allow special fields for
> PERCPU_CGROUP_STORAGE, but it's best to switch this one to
> copy_map_value()
> 

Agreed to use copy_map_value() here.

>> +               goto unlock;
>> +       }
>>         size = round_up(_map->value_size, 8);
>>         for_each_possible_cpu(cpu) {
>>                 bpf_long_memcpy(value + off,
>>                                 per_cpu_ptr(storage->percpu_buf, cpu), size);
> 
> and let's switch this to copy_map_value_long() to future-proof this:
> copy_map_value[_long]() should work correctly with any type of map and
> will take care of all existing and future special fields
> 
> (but maybe have it as a separate patch with just that change to make it obvious)
> 

Ack.

>>                 off += size;
>>         }
>> +unlock:
>>         rcu_read_unlock();
>>         return 0;
>>  }
>> @@ -213,10 +219,11 @@ int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *key,
>>  {
>>         struct bpf_cgroup_storage_map *map = map_to_storage(_map);
>>         struct bpf_cgroup_storage *storage;
>> -       int cpu, off = 0;
>> +       void *ptr;
>>         u32 size;
>> +       int cpu;
>>
>> -       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();
>> @@ -232,12 +239,18 @@ int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *key,
>>          * returned or zeros which were zero-filled by percpu_alloc,
>>          * so no kernel data leaks possible
>>          */
>> -       size = round_up(_map->value_size, 8);
>> +       size = (map_flags & (BPF_F_CPU | BPF_F_ALL_CPUS)) ? _map->value_size :
>> +               round_up(_map->value_size, 8);
>> +       if (map_flags & BPF_F_CPU) {
>> +               cpu = map_flags >> 32;
>> +               memcpy(per_cpu_ptr(storage->percpu_buf, cpu), value, size);
>> +               goto unlock;
>> +       }
>>         for_each_possible_cpu(cpu) {
>> -               bpf_long_memcpy(per_cpu_ptr(storage->percpu_buf, cpu),
>> -                               value + off, size);
>> -               off += size;
>> +               ptr = (map_flags & BPF_F_ALL_CPUS) ? value : value + size * cpu;
>> +               memcpy(per_cpu_ptr(storage->percpu_buf, cpu), ptr, size);
Switch memcpy() to copy_map_value[_long](), too.

Thanks,
Leon

[...]