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>]

>> >> @@ -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.
>
> Can you elaborate on why?
>

I’ve reconsidered your suggestion, and I agree.

With your approach, CPU flags and the CPU number won’t be checked when
'(do_delete || !is_percpu)', which makes sense.

I’d like to update the code as follows:

allowed_flags = BPF_F_LOCK;
if (!do_delete && is_percpu)
    allowed_flags |= BPF_F_CPU;
err = bpf_map_check_op_flags(map, elem_map_flags, allowed_flags);

This way, CPU flags and the CPU number are only validated for the
lookup_batch API on percpu hash maps.

Thanks,
Leon