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:
>>
[...]
>> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
>> index 71f9931ac64cd..031a74c1b7fd7 100644
>> --- a/kernel/bpf/hashtab.c
>> +++ b/kernel/bpf/hashtab.c
>> @@ -937,24 +937,39 @@ static void free_htab_elem(struct bpf_htab *htab, struct htab_elem *l)
>> }
>>
>> static void pcpu_copy_value(struct bpf_htab *htab, void __percpu *pptr,
>> - void *value, bool onallcpus)
>> + void *value, bool onallcpus, u64 map_flags)
>> {
>> + int cpu = map_flags & BPF_F_CPU ? map_flags >> 32 : 0;
>> + int current_cpu = raw_smp_processor_id();
>> +
>> if (!onallcpus) {
>> /* copy true value_size bytes */
>> - copy_map_value(&htab->map, this_cpu_ptr(pptr), value);
>> + copy_map_value(&htab->map, (map_flags & BPF_F_CPU) && cpu != current_cpu ?
>> + per_cpu_ptr(pptr, cpu) : this_cpu_ptr(pptr), value);
>
> FWIW, I still feel like this_cpu_ptr() micro-optimization is
> unnecessary and is just a distraction. This code is called when
> user-space updates/looks up per-CPU value, it's not a hot path by any
> means where this_cpu_ptr() vs per_cpu_ptr() makes any measurable
> difference
>
OK.
I'll remove it in next revision.
>> } else {
>> u32 size = round_up(htab->map.value_size, 8);
>> - int off = 0, cpu;
>> + int off = 0;
>> +
>> + if (map_flags & BPF_F_CPU) {
>> + copy_map_value_long(&htab->map, cpu != current_cpu ?
>> + per_cpu_ptr(pptr, cpu) : this_cpu_ptr(pptr), value);
>> + return;
>> + }
>>
>> for_each_possible_cpu(cpu) {
>> copy_map_value_long(&htab->map, per_cpu_ptr(pptr, cpu), value + off);
>> - 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;
>> }
>> }
>
> this couldn't have been replaced by bpf_percpu_copy_to_user?.. (and I
> just want to emphasize how hard did you make it to review all this by
> putting those bpf_percpu_copy_to_user and bpf_percpu_copy_from_user on
> its own in patch #2, instead of having a proper refactoring patch...)
>
Sorry about this.
I will use 'bpf_percpu_copy_from_user()' here in next revision.
Thanks,
Leon
