[bpf-next 0/2] bpf: Add flag for batch operation

[bpf-next 0/2] bpf: Add flag for batch operation

[Florian Lehner]

Introduce a new flag for batch operations that allows the deletion process
to continue even if certain keys are missing. This simplifies map flushing
by eliminating the requirement to maintain a separate list of keys and
makes sure maps can be flushed with a single batch delete operation.

Florian Lehner (2):
  bpf: Add flag to continue batch operation
  selftests/bpf: Add a test for batch operation flag

 include/uapi/linux/bpf.h                      |  5 +++++
 kernel/bpf/syscall.c                          | 14 ++++++++++---
 tools/include/uapi/linux/bpf.h                |  5 +++++
 .../bpf/map_tests/htab_map_batch_ops.c        | 20 ++++++++++++++++++-
 4 files changed, 40 insertions(+), 4 deletions(-)

-- 
2.47.0

[bpf-next 1/2] bpf: Add flag to continue batch operation

[Florian Lehner]

Introduce flag for batch operations to continue batch deletes when missing
keys are encountered. This allows to flush maps at once without the need to
keep track of the keys in a map.

Signed-off-by: Florian Lehner <dev@der-flo.net>
---
 include/uapi/linux/bpf.h       |  5 +++++
 kernel/bpf/syscall.c           | 14 +++++++++++---
 tools/include/uapi/linux/bpf.h |  5 +++++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 4162afc6b5d0..b38884cf6fe3 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1423,6 +1423,11 @@ enum {
  */
 #define BPF_F_QUERY_EFFECTIVE	(1U << 0)
 
+/* Flags for batch operations. */
+
+/* If set, continue with batch operation even if a key is missing. */
+#define BPF_F_BATCH_IGNORE_MISSING_KEY		(1U << 1)
+
 /* Flags for BPF_PROG_TEST_RUN */
 
 /* If set, run the test on the cpu specified by bpf_attr.test.cpu */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 58190ca724a2..860d6dc0c6d9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1851,7 +1851,7 @@ int generic_map_delete_batch(struct bpf_map *map,
 			     union bpf_attr __user *uattr)
 {
 	void __user *keys = u64_to_user_ptr(attr->batch.keys);
-	u32 cp, max_count;
+	u32 count, cp, max_count;
 	int err = 0;
 	void *key;
 
@@ -1874,6 +1874,7 @@ int generic_map_delete_batch(struct bpf_map *map,
 	if (!key)
 		return -ENOMEM;
 
+	count = 0;
 	for (cp = 0; cp < max_count; cp++) {
 		err = -EFAULT;
 		if (copy_from_user(key, keys + cp * map->key_size,
@@ -1890,11 +1891,18 @@ int generic_map_delete_batch(struct bpf_map *map,
 		err = map->ops->map_delete_elem(map, key);
 		rcu_read_unlock();
 		bpf_enable_instrumentation();
-		if (err)
+		if (err) {
+			if (err == -ENOENT &&
+			    (attr->batch.flags & BPF_F_BATCH_IGNORE_MISSING_KEY)) {
+				cond_resched();
+				continue;
+			}
 			break;
+		}
 		cond_resched();
+		count++;
 	}
-	if (copy_to_user(&uattr->batch.count, &cp, sizeof(cp)))
+	if (copy_to_user(&uattr->batch.count, &count, sizeof(count)))
 		err = -EFAULT;
 
 	kvfree(key);
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 4162afc6b5d0..ea03e7e8272b 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1423,6 +1423,11 @@ enum {
  */
 #define BPF_F_QUERY_EFFECTIVE	(1U << 0)
 
+/* Flags for batch operations. */
+
+/* If set, continue with batch operation even if a key is missing. */
+#define BPF_F_BATCH_IGNORE_MISSING_KEY          (1U << 1)
+
 /* Flags for BPF_PROG_TEST_RUN */
 
 /* If set, run the test on the cpu specified by bpf_attr.test.cpu */
-- 
2.47.0

[bpf-next 2/2] selftests/bpf: Add a test for batch operation flag

[Florian Lehner]

Add a test that verifies the batch operation continues if
BPF_F_BATCH_IGNORE_MISSING_KEY is set.

Signed-off-by: Florian Lehner <dev@der-flo.net>
---
 .../bpf/map_tests/htab_map_batch_ops.c        | 20 ++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/map_tests/htab_map_batch_ops.c b/tools/testing/selftests/bpf/map_tests/htab_map_batch_ops.c
index 5da493b94ae2..76df196f5223 100644
--- a/tools/testing/selftests/bpf/map_tests/htab_map_batch_ops.c
+++ b/tools/testing/selftests/bpf/map_tests/htab_map_batch_ops.c
@@ -136,7 +136,25 @@ void __test_map_lookup_and_delete_batch(bool is_pcpu)
 	err = bpf_map_get_next_key(map_fd, NULL, &key);
 	CHECK(!err, "bpf_map_get_next_key()", "error: %s\n", strerror(errno));
 
-	/* test 4: lookup/delete in a loop with various steps. */
+	/* test 4: batch delete with missing key */
+	map_batch_update(map_fd, max_entries, keys, values, is_pcpu);
+
+	key = 2;
+	err = bpf_map_delete_elem(map_fd, &key);
+	CHECK(err, "bpf_map_delete_elem()", "error: %s\n", strerror(errno));
+
+	DECLARE_LIBBPF_OPTS(bpf_map_batch_opts, ignore_opts,
+		.elem_flags = 0,
+		.flags = BPF_F_BATCH_IGNORE_MISSING_KEY,
+	);
+
+	err = bpf_map_delete_batch(map_fd, keys, &count,
+	       &ignore_opts);
+	CHECK(err, "batch delete with missing key", "error: %s\n", strerror(errno));
+	CHECK(count != max_entries-1, "count != max_entries-1",
+	      "count = %u, max_entries = %u\n", count, max_entries);
+
+	/* test 5: lookup/delete in a loop with various steps. */
 	total_success = 0;
 	for (step = 1; step < max_entries; step++) {
 		map_batch_update(map_fd, max_entries, keys, values, is_pcpu);
-- 
2.47.0

Re: [bpf-next 0/2] bpf: Add flag for batch operation

[Hou Tao]

On 11/10/2024 7:29 PM, Florian Lehner wrote:
> Introduce a new flag for batch operations that allows the deletion process
> to continue even if certain keys are missing. This simplifies map flushing
> by eliminating the requirement to maintain a separate list of keys and
> makes sure maps can be flushed with a single batch delete operation.

Is it expensive to close and recreate a new map instead ? If it is
expensive, does it make more sense to add a new command to delete all
elements in the map ? Because reusing the deletion logic will make each
deletion involve an unnecessary lookup operation.
>
> Florian Lehner (2):
>   bpf: Add flag to continue batch operation
>   selftests/bpf: Add a test for batch operation flag
>
>  include/uapi/linux/bpf.h                      |  5 +++++
>  kernel/bpf/syscall.c                          | 14 ++++++++++---
>  tools/include/uapi/linux/bpf.h                |  5 +++++
>  .../bpf/map_tests/htab_map_batch_ops.c        | 20 ++++++++++++++++++-
>  4 files changed, 40 insertions(+), 4 deletions(-)
>

Re: [bpf-next 0/2] bpf: Add flag for batch operation

[Alexei Starovoitov]

On Mon, Nov 11, 2024 at 6:15 AM Hou Tao <houtao@huaweicloud.com> wrote:
>
>
>
> On 11/10/2024 7:29 PM, Florian Lehner wrote:
> > Introduce a new flag for batch operations that allows the deletion process
> > to continue even if certain keys are missing. This simplifies map flushing
> > by eliminating the requirement to maintain a separate list of keys and
> > makes sure maps can be flushed with a single batch delete operation.
>
> Is it expensive to close and recreate a new map instead ? If it is
> expensive, does it make more sense to add a new command to delete all
> elements in the map ? Because reusing the deletion logic will make each
> deletion involve an unnecessary lookup operation.

+1 to above questions.

In addition:

What is the use case ?
Are you trying to erase all elements from the map ?

If so you bpf_for_each_map_elem() and delete elems while iterating.

This extra flag looks too specific.

pw-bot: cr

Re: [bpf-next 0/2] bpf: Add flag for batch operation

[dev]

On Mon, Nov 11, 2024 at 07:01:26PM -0800, Alexei Starovoitov wrote:
> On Mon, Nov 11, 2024 at 6:15 AM Hou Tao <houtao@huaweicloud.com> wrote:
> >
> >
> >
> > On 11/10/2024 7:29 PM, Florian Lehner wrote:
> > > Introduce a new flag for batch operations that allows the deletion process
> > > to continue even if certain keys are missing. This simplifies map flushing
> > > by eliminating the requirement to maintain a separate list of keys and
> > > makes sure maps can be flushed with a single batch delete operation.
> >
> > Is it expensive to close and recreate a new map instead ? If it is
> > expensive, does it make more sense to add a new command to delete all
> > elements in the map ? Because reusing the deletion logic will make each
> > deletion involve an unnecessary lookup operation.
> 
> +1 to above questions.

There is an eBPF map, that a variable number of eBPF programs use, to access
common states for a variable number of connections. On predefined events, a set
of keys is deleted from this map. This set can either be all keys or just a
subset of all keys - but it is not guaranteed that this set of keys still exists
in this eBPF map.
The current work around is to use bpf_map_lookup_and_delete_batch(), as this
operation continues on missing keys and clears all requested keys from the eBPF
map. The noticeable downside of bpf_map_lookup_and_delete_batch() is the memory
requirement that comes with the lookup and allocation for the values.

> > [..] If it is
> > expensive, does it make more sense to add a new command to delete all
> > elements in the map ?

It felt like bpf_map_delete_batch() was introduced for this use case. So adding
a new command was not considered.

> 
> In addition:
> 
> What is the use case ?
> Are you trying to erase all elements from the map ?
> 
> If so you bpf_for_each_map_elem() and delete elems while iterating.

bpf_for_each_map_elem() could be an option if the map should be flushed
completley, but in most cases only a subset of keys should be removed from the
map.

> 
> This extra flag looks too specific.

Sure, the proposed flag is focused on the delete operation. What could be the
requirement to make it less specific?

> 
> pw-bot: cr

Re: [bpf-next 0/2] bpf: Add flag for batch operation

[Alexei Starovoitov]

On Tue, Nov 12, 2024 at 11:13 AM <dev@der-flo.net> wrote:
>
> On Mon, Nov 11, 2024 at 07:01:26PM -0800, Alexei Starovoitov wrote:
> > On Mon, Nov 11, 2024 at 6:15 AM Hou Tao <houtao@huaweicloud.com> wrote:
> > >
> > >
> > >
> > > On 11/10/2024 7:29 PM, Florian Lehner wrote:
> > > > Introduce a new flag for batch operations that allows the deletion process
> > > > to continue even if certain keys are missing. This simplifies map flushing
> > > > by eliminating the requirement to maintain a separate list of keys and
> > > > makes sure maps can be flushed with a single batch delete operation.
> > >
> > > Is it expensive to close and recreate a new map instead ? If it is
> > > expensive, does it make more sense to add a new command to delete all
> > > elements in the map ? Because reusing the deletion logic will make each
> > > deletion involve an unnecessary lookup operation.
> >
> > +1 to above questions.
>
> There is an eBPF map, that a variable number of eBPF programs use, to access
> common states for a variable number of connections. On predefined events, a set
> of keys is deleted from this map. This set can either be all keys or just a
> subset of all keys - but it is not guaranteed that this set of keys still exists
> in this eBPF map.
> The current work around is to use bpf_map_lookup_and_delete_batch(), as this
> operation continues on missing keys and clears all requested keys from the eBPF
> map.

Hmm.
bpf_map_lookup_and_delete_batch() deletes all elements and
returns key/values.
It doesn't do any key comparisons.
There is no concept of skipping keys or missing keys.
It deletes all.

This cannot be a workaround.

> The noticeable downside of bpf_map_lookup_and_delete_batch() is the memory
> requirement that comes with the lookup and allocation for the values.

Sure. That's why I suggest for_each+delete that clears the map
without returning key/value back to user space.

> > > [..] If it is
> > > expensive, does it make more sense to add a new command to delete all
> > > elements in the map ?
>
> It felt like bpf_map_delete_batch() was introduced for this use case. So adding
> a new command was not considered.
>
> >
> > In addition:
> >
> > What is the use case ?
> > Are you trying to erase all elements from the map ?
> >
> > If so you bpf_for_each_map_elem() and delete elems while iterating.
>
> bpf_for_each_map_elem() could be an option if the map should be flushed
> completley, but in most cases only a subset of keys should be removed from the
> map.
>
> >
> > This extra flag looks too specific.
>
> Sure, the proposed flag is focused on the delete operation. What could be the
> requirement to make it less specific?

I feel that letting map_delete_batch() ignore keys make it a footgun.

Sounds like in your case the key is a network connection tuple.
So by ignoring unknown tuple you're saying 'delete connection info
if it's there'. I assuming because bpf prog and user space may race
to delete it.
In such case user space can trim key list and retry generic_map_delete_batch.
It's another syscall. A bit more overhead, but
try to speed up this very specific use cases seems wrong.
It's not going to be much faster. The set of keys is probably limited.