| Message ID | 20241110112905.64616-1-dev@der-flo.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | bpf: Add flag for batch operation | expand |
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(-) >
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
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
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.