Re: [PATCH v2 bpf-next 2/9] bpf: add generic support for lookup and lookup_and_delete batch ops

[Yonghong Song]

On 11/21/19 1:36 PM, Brian Vazquez wrote:
> Hi Yonghong,
> thanks for reviewing the patch, I will fix all the direct returns and
> small fixes in next version.
> 
> On Thu, Nov 21, 2019 at 9:36 AM Yonghong Song <yhs@xxxxxx> wrote:
>>
>>
>>
>> On 11/19/19 11:30 AM, Brian Vazquez wrote:
>>> This commit introduces generic support for the bpf_map_lookup_batch and
>>> bpf_map_lookup_and_delete_batch ops. This implementation can be used by
>>> almost all the bpf maps since its core implementation is relying on the
>>> existing map_get_next_key, map_lookup_elem and map_delete_elem
>>> functions. The bpf syscall subcommands introduced are:
>>>
[...]
>>> +     for (cp = 0; cp < max_count; cp++) {
>>> +             if (cp || first_key) {
>>> +                     rcu_read_lock();
>>> +                     err = map->ops->map_get_next_key(map, prev_key, key);
>>> +                     rcu_read_unlock();
>>> +                     if (err)
>>> +                             break;
>>> +             }
>>> +             err = bpf_map_copy_value(map, key, value,
>>> +                                      attr->batch.elem_flags, do_delete);
>>> +
>>> +             if (err == -ENOENT) {
>>> +                     if (retry) {
>>> +                             retry--;
>>
>> What is the 'retry' semantics here? After 'continue', cp++ is executed.
> 
> Good catch, I'll move cp++ to a proper place. retry is used to prevent
> the cases where the map is doing many concurrent additions and
> deletions, this could result in map_get_next_key succeeding but
> bpf_map_copy_value failing, in which case I think it'd be better to
> try and find a next elem, but we don't want to do this for more than 3
> times.
> 
>>
>>> +                             continue;
>>> +                     }
>>> +                     err = -EINTR;
>>
>> Why returning -EINTR?
> 
> I thought that this is the err more appropriate for the behaviour I
> describe above. Should I handle that case? WDYT?

I see. We do not want to use -ENOENT since get_next_key may return 
-ENOENT. I think -EINTR is okay here to indicate we have key, but 
key/value entry is gone right before the attempted access.