On 4/11/21 11:47 PM, Denis Salopek wrote:
On Sun, Apr 04, 2021 at 09:47:30PM -0700, Yonghong Song wrote:
On 3/29/21 5:57 AM, Denis Salopek wrote:
Extend the existing bpf_map_lookup_and_delete_elem() functionality to
hashtab maps, in addition to stacks and queues.
Add bpf_map_lookup_and_delete_elem_flags() libbpf API in order to use
the BPF_F_LOCK flag.
Create a new hashtab bpf_map_ops function that does lookup and deletion
of the element under the same bucket lock and add the created map_ops to
bpf.h.
Add the appropriate test cases to 'maps' and 'lru_map' selftests
accompanied with new test_progs.
Cc: Juraj Vijtiuk <juraj.vijtiuk@xxxxxxxxxx>
Cc: Luka Oreskovic <luka.oreskovic@xxxxxxxxxx>
Cc: Luka Perkov <luka.perkov@xxxxxxxxxx>
Signed-off-by: Denis Salopek <denis.salopek@xxxxxxxxxx>
---
v2: Add functionality for LRU/per-CPU, add test_progs tests.
v3: Add bpf_map_lookup_and_delete_elem_flags() and enable BPF_F_LOCK
flag, change CHECKs to ASSERT_OKs, initialize variables to 0.
v4: Fix the return value for unsupported map types.
---
include/linux/bpf.h | 2 +
kernel/bpf/hashtab.c | 97 ++++++
kernel/bpf/syscall.c | 27 +-
tools/lib/bpf/bpf.c | 13 +
tools/lib/bpf/bpf.h | 2 +
tools/lib/bpf/libbpf.map | 1 +
.../bpf/prog_tests/lookup_and_delete.c | 279 ++++++++++++++++++
.../bpf/progs/test_lookup_and_delete.c | 26 ++
tools/testing/selftests/bpf/test_lru_map.c | 8 +
tools/testing/selftests/bpf/test_maps.c | 19 +-
10 files changed, 469 insertions(+), 5 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/lookup_and_delete.c
create mode 100644 tools/testing/selftests/bpf/progs/test_lookup_and_delete.c
Since another revision is needed, could you break the patch to several
commits which will make it easy to review?
Patch 1: kernel + uapi header:
include/linux/bpf.h
kernel/bpf/hashtab.c
kernel/bpf/syscall.c
include/uapi/linux/bpf.h and tools/include/uapi/linux/bpf.h (see below)
Patch 2: libbpf change
tools/lib/bpf/bpf.{c,h}, libbpf.map
Patch 3: selftests/bpf change
tools/testing/selftests/bpf/...
Sure, I'll do that.
[...]
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 9603de81811a..e3851bafb603 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1468,7 +1468,7 @@ int generic_map_lookup_batch(struct bpf_map *map,
return err;
}
-#define BPF_MAP_LOOKUP_AND_DELETE_ELEM_LAST_FIELD value
+#define BPF_MAP_LOOKUP_AND_DELETE_ELEM_LAST_FIELD flags
static int map_lookup_and_delete_elem(union bpf_attr *attr)
{
@@ -1484,6 +1484,9 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr)
if (CHECK_ATTR(BPF_MAP_LOOKUP_AND_DELETE_ELEM))
return -EINVAL;
+ if (attr->flags & ~BPF_F_LOCK)
+ return -EINVAL; > +
f = fdget(ufd);
map = __bpf_map_get(f);
if (IS_ERR(map))
@@ -1494,24 +1497,40 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr)
goto err_put;
}
Previously, for QUEUE and STACK map, flags are not allowed, and libbpf
sets it as 0. Let us enforce attr->flags with 0 here for QUEUE and STACK.
Ok, so just to make this clear: if map_type is QUEUE or STACK, and if
attr->flags != 0, return -EINVAL?
Yes.
+ if ((attr->flags & BPF_F_LOCK) &&
+ !map_value_has_spin_lock(map)) {
+ err = -EINVAL;
+ goto err_put;
+ }
+
key = __bpf_copy_key(ukey, map->key_size);
if (IS_ERR(key)) {
err = PTR_ERR(key);
goto err_put;
}
- value_size = map->value_size;
+ value_size = bpf_map_value_size(map);
err = -ENOMEM;
value = kmalloc(value_size, GFP_USER | __GFP_NOWARN);
if (!value)
goto free_key;
+ err = -ENOTSUPP;
if (map->map_type == BPF_MAP_TYPE_QUEUE ||
map->map_type == BPF_MAP_TYPE_STACK) {
err = map->ops->map_pop_elem(map, value);
- } else {
- err = -ENOTSUPP;
+ } else if (map->map_type == BPF_MAP_TYPE_HASH ||
+ map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
+ map->map_type == BPF_MAP_TYPE_LRU_HASH ||
+ map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH) {
Since we added four new map support for lookup_and_delete, we should
update documentation in uapi bpf.h.
Currently, it is:
* BPF_MAP_LOOKUP_AND_DELETE_ELEM
* Description
* Look up an element with the given *key* in the map referred
to
* by the file descriptor *fd*, and if found, delete the
element.
*
* The **BPF_MAP_TYPE_QUEUE** and **BPF_MAP_TYPE_STACK** map
types
* implement this command as a "pop" operation, deleting the
top
* element rather than one corresponding to *key*.
* The *key* and *key_len* parameters should be zeroed when
* issuing this operation for these map types.
*
* This command is only valid for the following map types:
* * **BPF_MAP_TYPE_QUEUE**
* * **BPF_MAP_TYPE_STACK**
*
* Return
* Returns zero on success. On error, -1 is returned and
*errno*
* is set appropriately.
Please remember to sync updated linux/include/uapi/linux/bpf.h to
linux/tools/include/uapi/linux/bpf.h.
The description looks ok to me as it is, I should just extend the list
of valid map types and add the description for the flags argument (needs
to be 0 for QUEUE and STACK, can be BPF_F_LOCK for rest)?
Yes.
+ if (!bpf_map_is_dev_bound(map)) {
+ bpf_disable_instrumentation();
+ rcu_read_lock();
+ err = map->ops->map_lookup_and_delete_elem(map, key, value, attr->flags);
+ rcu_read_unlock();
+ bpf_enable_instrumentation();
+ }
}
if (err)
[...]
