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.
