On Mon, Feb 10, 2025 at 10:21:38AM -0600, Yan Zhai wrote:
Hi Brian, Jiri
thanks for the comments.
On Mon, Feb 10, 2025 at 8:47 AM Brian Vazquez brianvv@google.com wrote:
On Mon, Feb 10, 2025 at 4:19 AM Jiri Olsa olsajiri@gmail.com wrote:
On Sun, Feb 09, 2025 at 11:22:35PM -0800, Yan Zhai wrote:
The generic_map_lookup_batch currently returns EINTR if it fails with ENOENT and retries several times on bpf_map_copy_value. The next batch would start from the same location, presuming it's a transient issue. This is incorrect if a map can actually have "holes", i.e. "get_next_key" can return a key that does not point to a valid value. At least the array of maps type may contain such holes legitly. Right now these holes show up, generic batch lookup cannot proceed any more. It will always fail with EINTR errors.
Rather, do not retry in generic_map_lookup_batch. If it finds a non existing element, skip to the next key. This simple solution comes with a price that transient errors may not be recovered, and the iteration might cycle back to the first key under parallel deletion. For example,
probably stupid question, but why not keep the retry logic and when it fails then instead of returning EINTR just jump to the next key
jirka
+1, keeping the retry logic but moving to the next key on error sounds like a sensible approach.
I made the trade off since retry would consistently fail for the array of maps, so it is merely wasting cycles to ever do so. It is already pretty slow to read these maps today from userspace (for us we read them for accounting/monitoring purposes), so it is nice to save a few cycles especially for sparse maps. E.g. We use inner maps to store protocol specific actions in an array of maps with 256 slots, but usually only a few common protocols like TCP/UDP/ICMP are populated, leaving most "holes". On the other hand, I personally feel it is really "fragile" if users rely heavily on this logic to survive concurrent lookup and deletion. Would it make more sense to provide concurrency guarantee with map specific ops like hash map?
Brian, any details on the EINTR path? is that just to survive concurent batch-lookup and delete?
if that's important use case I guess the map specific function would be possible, because it's broken for maps with holes as you described
thanks, jirka