On Wed, Feb 5, 2025 at 10:17 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 2/6/2025 11:01 AM, Yan Zhai wrote:
> > On Wed, Feb 5, 2025 at 6:46 PM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >> On 2/6/2025 12:27 AM, Yan Zhai wrote:
> >>> On Wed, Feb 5, 2025 at 3:56 AM Alexei Starovoitov
> >>> <alexei.starovoitov@xxxxxxxxx> wrote:
> >>>> Let's not invent new magic return values.
> >>>>
> >>>> But stepping back... why do we have this EINTR case at all?
> >>>> Can we always goto next_key for all map types?
> >>>> The command returns and a set of (key, value) pairs.
> >>>> It's always better to skip then get stuck in EINTR,
> >>>> since EINTR implies that the user space should retry and it
> >>>> might be successful next time.
> >>>> While here it's not the case.
> >>>> I don't see any selftests for EINTR, so I suspect it was added
> >>>> as escape path in case retry count exceeds 3 and author assumed
> >>>> that it should never happen in practice, so EINTR was expected
> >>>> to be 'never happens'. Clearly that's not the case.
> >>> It makes more sense to me if we just goto the next key for all types.
> >>> At least for current users of generic batch lookup, arrays and
> >>> lpm_trie, I didn't notice in any case retry would help.
> >> I think it will break lpm_trie. In lpm_trie, if tries to find the next
> >> key of a non-existent key, it will restart from the left-mode node.
> > I am not sure how lpm trie would break if we always skip to the next
> > key. Current retry logic does not change prev_key, so the lookup key
> > will always be the same. It would make sense if searching with the
> > same key could temporarily fail, but it does not seem so for both
> > lpm_tire and array based maps.
>
> Retry logic does change prev_key, please see "swap(prev_key, key);"
> below the next_key tag, otherwise the lookup_batch procedure will loop
> forever for array map.
>
We are probably not on the same page. Let me clarify:
By "retry logic" I mean this code snippet:
if (err == -ENOENT) {
if (retry) {
retry--;
continue;
}
err = -EINTR;
break;
}
It wouldn't execute the swap when ENOENT is returned from bpf_map_copy_value.
And by "skipping to the next key", it's simply
if (err == -ENOENT)
goto next_key;
Note the "next_key" label was not in the current codebase. It is only
in my posted patch. I don't think this would break lpm_trie unless I
missed something.
Yan
