Hi
On 12/10/2023 10:11 AM, Alexei Starovoitov wrote:
> On Fri, Dec 8, 2023 at 2:55 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>>
>> On 12/8/23 2:23 AM, Hou Tao wrote:
>>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>>
>>> There is no need to call maybe_wait_bpf_programs() if all operations in
>>> batched update, deletion, or lookup_and_deletion fail. So only call
>>> maybe_wait_bpf_programs() if at least one map operation succeeds.
>>>
SNIP
>>> + attr->batch.count = 0;
>>> if (put_user(0, &uattr->batch.count))
>>> return -EFAULT;
>>>
>>> @@ -1903,6 +1908,7 @@ int generic_map_lookup_batch(struct bpf_map *map,
>>> if (err == -EFAULT)
>>> goto free_buf;
>>>
>>> + attr->batch.count = cp;
>> You don't need to change generic_map_lookup_batch() here. It won't trigger
>> maybe_wait_bpf_programs().
>>
>>> if ((copy_to_user(&uattr->batch.count, &cp, sizeof(cp)) ||
>>> (cp && copy_to_user(uobatch, prev_key, map->key_size))))
>>> err = -EFAULT;
>>> @@ -4926,7 +4932,7 @@ static int bpf_task_fd_query(const union bpf_attr *attr,
>>> err = fn(__VA_ARGS__); \
>>> } while (0)
>>>
>>> -static int bpf_map_do_batch(const union bpf_attr *attr,
>>> +static int bpf_map_do_batch(union bpf_attr *attr,
>>> union bpf_attr __user *uattr,
>>> int cmd)
>>> {
>>> @@ -4966,7 +4972,8 @@ static int bpf_map_do_batch(const union bpf_attr *attr,
>>> BPF_DO_BATCH(map->ops->map_delete_batch, map, attr, uattr);
>>> err_put:
>>> if (has_write) {
>>> - maybe_wait_bpf_programs(map);
>>> + if (attr->batch.count)
>>> + maybe_wait_bpf_programs(map);
>> Your code logic sounds correct but I feel you are optimizing for extreme
>> corner cases. In really esp production environment, a fault with something
>> like copy_to_user() should be extremely rare. So in my opinion, this optimization
>> is not needed.
> +1
> the code is fine as-is.
Thanks for suggestions. It is indeed an over-optimization for a rare
scenario. Will drop it.
