finish to add support of subset in items_*_batch() #3440

egobillot · 2021-05-18T22:17:12Z

Hello,
Here is a partial PR in order to :

add items_lookup_batch() on subset keys
add items_lookup_and_delete_batch() on subset keys
add docstring on items_*_batch()
update the reference_guide.md
This is a partial PT because I am stuck with items_lookup_batch and items_lookup_and_delete_batch on a subset. Sometimes tests are Ok and sometimes they fails. See the output of tests.

python git:(add_subset_for_batch_ops) ✗ sudo python3 ./test_map_batch_ops.py
...F....
======================================================================
FAIL: test_lookup_and_delete_batch_subset (__main__.TestMapBatch)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./test_map_batch_ops.py", line 94, in test_lookup_and_delete_batch_subset
    self.assertEqual(count, self.SUBSET_SIZE)
AssertionError: 31 != 32

----------------------------------------------------------------------
Ran 8 tests in 2.635s

FAILED (failures=1)
➜  python git:(add_subset_for_batch_ops) ✗ sudo python3 ./test_map_batch_ops.py
........
----------------------------------------------------------------------
Ran 8 tests in 2.613s

OK

Do I need to call the lib.bpf_lookup_and_delete_batch() several times until the sum of the nb of elements returned matches the subset size ? For example, If my subset size is 32 and get only 31 elem on the first call, do I need to call it again for the last element ? 🤔

- add items_lookup_batch() on subset keys - add items_lookup_and_delete_batch() on subset keys - add docstring on items_*_batch() - update the reference_guide.md

yonghong-song · 2021-05-19T19:00:23Z

@egobillot This is the original kernel patch introduced for lookup_batch/lookup_and_delete_batch/update_batch/delete_batch.
https://lore.kernel.org/bpf/20200115184308.162644-5-brianvv@google.com

The quote from the commit message:

in_batch and out_batch are only used for lookup and lookup_and_delete since
those are the only two operations that attempt to traverse the map.

update/delete batch ops should provide the keys/values that user wants
to modify.

Basically, lookup/lookup_and_delete does not accept a list of keys as the input,
while update/delete needs a list of keys as the input.

So we do not need to provide lookup/lookup_and_delete API which accepts
keys as input.

For you test failure, the reason is:

suppose hash table max size is 1024
we want to traverse the first 32 elements and provided a buffer with size 32 elements.
Say bucket 0/1 total 31 elements and bucket 2 has 3 elements.
So kernel will only be able to give back the first 31 elements. since if it tries to
include all data from bucket 2, it will be 34 elements exceeding the buffer size.
Note that kernel has to either include all data from one bucket or none of them. Otherwise,
things could be change between syscalls for that bucket.

I think you do not need to have subset tests from lookup, lookup_and_delete as they are not really supported.

egobillot · 2021-05-21T13:38:33Z

Thank you @yonghong-song . It was not clear to me that subset batch was not supported for lookup and lookup_and_delete by the kernel. So I remove the code regarding subset for lookup_*.

…rt batch on a subset of key: update code accordingly.

yonghong-song · 2021-05-22T00:31:16Z

[buildbot, test this please]

yonghong-song

Added some comments. Most importantly for the robust implementation, I suggest to have a loop for lookup_batch and lookup_and_delete_batch, could you take a look?

yonghong-song · 2021-05-23T22:57:33Z

src/python/bcc/table.py

+            alloc_v (bool): True to allocate values array, False otherwise.
+            Default is False.
+            count (int): number of elements in the array(s) to allocate. If
+            count is None then it allocates the maximum nb of elements i.e


nb => number

yonghong-song · 2021-05-23T22:58:23Z

src/python/bcc/table.py

+            tuple: (count, keys, values). Where count is ct.c_uint32,
+            and keys and values an instance of ct.Array
+        Raises:
+            ValueError: If count is not < 1 or > self.max_entries


ValueError: If count is less than 1 or greater than self.max_entries?

yonghong-song · 2021-05-23T23:00:26Z

src/python/bcc/table.py

+        Returns:
+            ct.c_uint32 : the size of the array(s)
+        Raises:
+            ValueError: If length of arrays is < 1 or > self.max_entries, or


is less than 1 or greater than self.max_entries?

yonghong-song · 2021-05-23T23:06:59Z

src/python/bcc/table.py

+            tuple: The tuple of (key,value) for every entries that have
+            been looked up.
+        Raises:
+            Exception: If bpf syscall has failed or is invalid


How about Exception: if bpf syscall return value indicates an error

yonghong-song · 2021-05-23T23:19:21Z

src/python/bcc/table.py

-                                   ct.byref(keys),
-                                   ct.byref(values),
-                                   ct.byref(count)
+                                   ct.byref(ct_cnt),


To avoid future confusion, let us just use a separate variable out_batch here by not overloading ct_cnt.

yonghong-song · 2021-05-23T23:33:22Z

src/python/bcc/table.py

+            tuple: The tuple of (key,value) for every entries that have
+            been deleted.
+        Raises:
+            Exception: If bpf syscall has failed or is invalid


if bpf syscall returns an error.

yonghong-song · 2021-05-23T23:35:17Z

src/python/bcc/table.py

-                                       ct.byref(keys),
-                                       ct.byref(count)
+                                       ct.byref(ct_keys),
+                                       ct.byref(ct_cnt)
                                       )
            errcode = ct.get_errno()
            if (errcode == errno.EINVAL):


The error chacking here and below is not quite right. The delete_batch and update_batch use kernel internal function generic_map_{delete,update}_batch function, they return 0 to indicate a success and non-0 as an error code. Just check errcode is 0 or not is enough.

yonghong-song · 2021-05-23T23:36:41Z

src/python/bcc/table.py

-                                   ct.byref(count)
+                                   ct.byref(ct_keys),
+                                   ct.byref(ct_values),
+                                   ct.byref(ct_cnt)
                                   )

        errcode = ct.get_errno()


similar to delete_batch, just check whether errcode is 0 or not is enough below.

yonghong-song · 2021-05-23T23:37:20Z

src/python/bcc/table.py

+            tuple: The tuple of (key,value) for every entries that have
+            been looked up and deleted.
+        Raises:
+            Exception: If bpf syscall has failed or is invalid


if bpf syscall return code indicates an error.

yonghong-song · 2021-05-23T23:38:01Z

src/python/bcc/table.py

+                                              ct.byref(ct_cnt),
+                                              ct.byref(ct_keys),
+                                              ct.byref(ct_values),
+                                              ct.byref(ct_cnt)


similar to lookup_batch, we may need a loop here.

egobillot · 2021-05-25T20:26:32Z

Yes, I will have a look at your comments.

…ake it more robust.

egobillot · 2021-05-27T22:08:01Z

@yonghong-song It should be better now with 7cbf766 . I have factorized the code for items_lookup_and_delete_batch() and items_lookup_batch() since it was almost the same.
I have also took into account all your comments. I hope I did not missed one.

yonghong-song · 2021-06-01T22:03:54Z

[buildbot, test this please]

src/python/bcc/table.py

yonghong-song · 2021-06-01T22:57:50Z

@egobillot Thanks for the update! Much better. I just added one more comment and I think we should be fine after that.

yonghong-song · 2021-06-01T22:58:06Z

[buildbot, ok to test]

yonghong-song · 2021-06-06T05:43:15Z

LGTM, Thanks!

finish to add support of subset in items_*_batch() - rewrite items_lookup_batch() and items_lookup_and_delete_batch() to make it more robust. - add docstring on items_*_batch() - update the reference_guide.md

[WIP] finish to add support of subset in items_*_batch()

48ff56c

- add items_lookup_batch() on subset keys - add items_lookup_and_delete_batch() on subset keys - add docstring on items_*_batch() - update the reference_guide.md

egobillot requested review from brendangregg, drzaeus77, goldshtn and yonghong-song as code owners May 18, 2021 22:17

BPF_MAP_LOOKUP_BATCH and BPF_MAP_LOOKUP_AND_DELETE_BATCH do not suppo…

6f2fa01

…rt batch on a subset of key: update code accordingly.

yonghong-song reviewed May 23, 2021

View reviewed changes

rewrite items_lookup_batch() and items_lookup_and_delete_batch() to m…

7cbf766

…ake it more robust.

egobillot changed the title ~~[WIP] finish to add support of subset in items_*_batch()~~ finish to add support of subset in items_*_batch() May 27, 2021

yonghong-song reviewed Jun 1, 2021

View reviewed changes

src/python/bcc/table.py Show resolved Hide resolved

improve logic in _items_lookup_and_optionally_delete_batch

59394fb

yonghong-song merged commit f77c1a6 into iovisor:master Jun 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

finish to add support of subset in items_*_batch() #3440

finish to add support of subset in items_*_batch() #3440

egobillot commented May 18, 2021

yonghong-song commented May 19, 2021

egobillot commented May 21, 2021

yonghong-song commented May 22, 2021

yonghong-song left a comment

yonghong-song May 23, 2021

yonghong-song May 23, 2021

yonghong-song May 23, 2021

yonghong-song May 23, 2021

yonghong-song May 23, 2021

yonghong-song May 23, 2021

yonghong-song May 23, 2021

yonghong-song May 23, 2021

yonghong-song May 23, 2021

yonghong-song May 23, 2021

egobillot commented May 25, 2021

egobillot commented May 27, 2021

yonghong-song commented Jun 1, 2021

yonghong-song commented Jun 1, 2021

yonghong-song commented Jun 1, 2021

yonghong-song commented Jun 6, 2021

finish to add support of subset in items_*_batch() #3440

finish to add support of subset in items_*_batch() #3440

Conversation

egobillot commented May 18, 2021

yonghong-song commented May 19, 2021

egobillot commented May 21, 2021

yonghong-song commented May 22, 2021

yonghong-song left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egobillot commented May 25, 2021

egobillot commented May 27, 2021

yonghong-song commented Jun 1, 2021

yonghong-song commented Jun 1, 2021

yonghong-song commented Jun 1, 2021

yonghong-song commented Jun 6, 2021