-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add new batched iterator type in pkg/bpf #35079
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2fc88c3
to
1ea5195
Compare
1ea5195
to
05c6622
Compare
Need to fix some test failures - moving this back to draft for a little while. |
de33043
to
4c30a15
Compare
/test |
57e98b4
to
74090c2
Compare
/test |
/test |
For reviewers - I'm moving this back out of draft. Per my last comment; I've dropped all commits related to ctmap in favor of trying to get the initial pkg/bpf functions and nat map changes merged first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Nice work!
d326756
to
16ff3c1
Compare
/test |
16ff3c1
to
60d3abe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, sorry for the long wait!
60d3abe
to
0a0ec30
Compare
/test |
0a0ec30
to
6a0c499
Compare
/test |
Adds new BatchIterator[KT,VT] type which provides a typed wrapper for *Map which allows for iteration using the iter.Seq2 type. This allows for seamless iteration, while abstracting away concerns regarding iterating batches & inner-loops as well as managing/allocating batch sizes, managing cursors and handling errors. In future commits, this will be used to DRY up similar internal implementations in pkg/maps/{ctmap,nat} and instead use the general purpose BatchIterator approach. Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
As well, rename the function from Apply... to Dump... as it better describes the purpose of the function. Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
As previous commits have switched the DumpBatch{4,6} funcs to use the bpf.DumpBatchReliably function, we no longer need the nat specific internal implementation for batch dumping so we can remove it. Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
6a0c499
to
d4b0eb0
Compare
/test |
Hitting this one on clustermesh e2e upgrade: #35865 |
See commit comments for details.
Add new batched iterator type in pkg/bpf:
This is intended to provide a general approach to iterating maps using the "batched" API, and replace existing bespoke implementations in ctmap/nat to use the one generic approach.
Follow up work
This PR will not change
pkg/maps/ctmap
to use this as @ysksuzuki is currently working on refactoring the ctmap gc code, which would likely cause large conflicts - so that will be a follow up to that work being done.Design considerations
Using Generics: Most Cilium functions that abstract bpf map operations rely on the
MapKey
andMapValue
interfaces, in particular the respectiveNew() MapKey
&New() MapValue
interface functions which allow allocation of a key/value pointer type in an opaque manner. For batched operations this doesn't work as we need to be able to allocate contiguous, non-indirect, slices of objects which are passed to the batched bpf syscalls for processing. Thus this is done with a generic wrapper type, which is able to do such an allocations (i.e.make([]T, batchSize)
). The tradeoff is that, going back to the first problem, the generic type must not be a pointer/interface type, so even if the implentation of Map{Key,Value} is using the pointer handle for a type (i.e. is implemented withfunc (k *someKeyType) Map() bpf.MapKey {...}
), we still need to use the serial struct type in this situation (i.e.NewBatchIterator[someKeyType, someValueType]
). Failing to do so will result in a runtime error.Error Handling: The benefits of returning a seq.Iter2 type is that this will compose nicely with loops and other functional list operations in the future. The downside is that batched iteration can fail, thus we need some kind of error collection mechanism. With this I've opted to collect Errors with the
Err()
function. Practically, this means that we need to actually use a iterator handle instead of just doing it purely functionally.Use batched iteration for ctmap GC to improve performance
note: I'm using pkg/bpf/ as a staging ground for hammering out a good API for a batched iterator, a follow up will be to open a PR to add this functionality in cilium/ebpf and then use that.