Skip to content

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

Merged
merged 3 commits into from
Nov 28, 2024

Conversation

tommyp1ckles
Copy link
Contributor

@tommyp1ckles tommyp1ckles commented Sep 29, 2024

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.

// NewBatchIterator returns a typed wrapper for *Map that allows for
// iterating (i.e. "dumping") the map using the bpf batch api.
//
// Unlike the general *Map dump functions, this must reads into slices of
// a concrete type (such as struct, or other scalar type), rather than into
// pointers. This is because there is currently no safe way to allocate a
// contiguous slice of key/value elements from either the underlying generic
// pointer types or from MapKey/MapValue interface types.
//
// This means that attempting to use the pointer types that implement
// MapKey/MapValue will fail upon iteration (via the Err() function).
//
// Example usage:
//
//	m := NewMap("cilium_test",
//		ebpf.Hash,
//		&TestKey{}, 	// *TestKey implements MapKey.
//		&TestValue{}, 	// *TestValue implements MapValue.
//		mapSize,
//		BPF_F_NO_PREALLOC,
//	)
//
//	// Note: TestKey & TestValue are not pointer types!
//	iter := NewBatchIterator[TestKey, TestValue](m)
//	for k, v := range iter {
//		// ...
//	}
//
// Following iteration, any unresolved errors encountered when iterating
// the bpf map can be accessed via the Err() function.

Design considerations

  1. Using Generics: Most Cilium functions that abstract bpf map operations rely on the MapKey and MapValue interfaces, in particular the respective New() 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 with func (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.

  2. 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.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 29, 2024
@tommyp1ckles tommyp1ckles added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Sep 29, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 29, 2024
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/batch-ctmap branch 8 times, most recently from 2fc88c3 to 1ea5195 Compare October 4, 2024 04:52
@tommyp1ckles tommyp1ckles marked this pull request as ready for review October 4, 2024 04:53
@tommyp1ckles tommyp1ckles requested review from a team as code owners October 4, 2024 04:53
@tommyp1ckles tommyp1ckles requested review from markpash and rgo3 October 4, 2024 04:53
@tommyp1ckles tommyp1ckles requested a review from ti-mo October 4, 2024 04:58
@tommyp1ckles
Copy link
Contributor Author

Need to fix some test failures - moving this back to draft for a little while.

@tommyp1ckles tommyp1ckles marked this pull request as draft October 7, 2024 05:00
@tommyp1ckles tommyp1ckles added the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Oct 7, 2024
@tommyp1ckles tommyp1ckles force-pushed the pr/tp/batch-ctmap branch 3 times, most recently from de33043 to 4c30a15 Compare October 7, 2024 21:05
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles force-pushed the pr/tp/batch-ctmap branch 2 times, most recently from 57e98b4 to 74090c2 Compare October 7, 2024 22:55
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles marked this pull request as ready for review October 7, 2024 23:17
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles tommyp1ckles marked this pull request as ready for review October 22, 2024 22:15
@tommyp1ckles
Copy link
Contributor Author

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.

@tommyp1ckles tommyp1ckles changed the title Use batched iteration for CT map GC to improve performance. Add new batched iterator type in pkg/bpf: Oct 22, 2024
@tommyp1ckles
Copy link
Contributor Author

Per my last comment, I think I just need reviews from @cilium/loader and @cilium/sig-datapath.

@markpash @rgo3 Think you two where assigned for datapath & loader. Gentle ping for review 😃

@tommyp1ckles tommyp1ckles removed the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Oct 29, 2024
@julianwiedmann julianwiedmann self-requested a review October 30, 2024 17:20
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@julianwiedmann julianwiedmann removed the request for review from markpash November 5, 2024 08:33
Copy link
Member

@dylandreimerink dylandreimerink left a 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!

@tommyp1ckles
Copy link
Contributor Author

/test

Copy link
Contributor

@rgo3 rgo3 left a 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!

@joestringer joestringer changed the title Add new batched iterator type in pkg/bpf: Add new batched iterator type in pkg/bpf Nov 25, 2024
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles
Copy link
Contributor Author

/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>
@tommyp1ckles
Copy link
Contributor Author

/test

@tommyp1ckles
Copy link
Contributor Author

Hitting this one on clustermesh e2e upgrade: #35865

@tommyp1ckles tommyp1ckles added this pull request to the merge queue Nov 28, 2024
Merged via the queue into cilium:main with commit f1746da Nov 28, 2024
64 checks passed
@tommyp1ckles tommyp1ckles deleted the pr/tp/batch-ctmap branch November 28, 2024 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/conntrack release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants