-
Notifications
You must be signed in to change notification settings - Fork 5
Initial flow table data structure #817
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
977302e
to
5e9796b
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.
Pull Request Overview
This PR introduces the initial flow table data structure for the networking stack, providing a foundation for tracking network flows with timeout capabilities. The implementation uses a combination of DashMap for concurrent hash table operations and thread-local priority queues for managing flow expiration.
- Adds FlowTable with concurrent insert/lookup/remove operations and automatic timeout handling
- Implements FlowKey with support for both unidirectional and bidirectional flows
- Provides thread-local priority queues for efficient timeout management using weak references
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
pkt-meta/src/lib.rs | Adds flow_table module export |
pkt-meta/src/flow_table/mod.rs | Module structure and public API exports |
pkt-meta/src/flow_table/flow_key.rs | Flow key data structures with bidirectional/unidirectional support |
pkt-meta/src/flow_table/flow_info.rs | Flow information storage with atomic expiration times |
pkt-meta/src/flow_table/atomic_instant.rs | Wrapper for atomic Instant operations |
pkt-meta/src/flow_table/thread_local_pq.rs | Thread-local priority queue implementation for timeout management |
pkt-meta/src/flow_table/table.rs | Main FlowTable implementation with concurrent operations |
pkt-meta/src/flow_table/README.md | Documentation of design decisions and future optimizations |
pkt-meta/Cargo.toml | Adds required dependencies for flow table functionality |
net/src/udp/mod.rs | Exports UdpPort type for flow key usage |
Cargo.toml | Adds workspace dependencies for flow table implementation |
Comments suppressed due to low confidence (1)
pkt-meta/src/flow_table/thread_local_pq.rs:1
- The
#[allow(unused_must_use)]
attribute is unnecessary here sinceexpires_at()
returnsInstant
which doesn't have#[must_use]
. Remove this attribute to clean up the code.
// SPDX-License-Identifier: Apache-2.0
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
5e9796b
to
9bcd4ec
Compare
bd7466e
to
b900948
Compare
b900948
to
03f48ed
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.
This all looks good to me. Minor comments made, but nothing blocking.
I really like the effort which went into testing this code (it isn't easy code to test by any means).
I also appreciate the docs.
I'm slightly concerned about the performance we will get given the number of allocations we can expect, but that requires measurement before we change anything.
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.
Great work!
Looks good overall, I don't see anything obvious that would be “missing”, although it's always hard to tell as long as we haven't integrated yet with e.g. NAT.
I've got some minor nits and comments.
More importantly, I may have spotted a couple bugs in flow_keys.rs, please double-check.
7d63588
to
4cc4008
Compare
Signed-off-by: Manish Vachharajani <[email protected]>
Signed-off-by: Manish Vachharajani <[email protected]>
Signed-off-by: Manish Vachharajani <[email protected]>
Signed-off-by: Manish Vachharajani <[email protected]>
Signed-off-by: Manish Vachharajani <[email protected]>
Signed-off-by: Manish Vachharajani <[email protected]>
Signed-off-by: Manish Vachharajani <[email protected]>
29e73af
to
9738e3f
Compare
This is the first cut for a flow table. There is more work to be done, but this is a good checkpoint. Interfaces and organization may change as we start using this with the stateful NAT code. Signed-off-by: Manish Vachharajani <[email protected]>
9738e3f
to
09e3f3c
Compare
I've addressed all the requested changes but due to timezone, I'm going to dismiss the review so we can merge now.
This PR adds the initial flow table data structure, but not the corresponding pipeline stages.
The appropriate pipeline stages will be added as part of NAT integration and the interface for the flow table updated for anything needed that was not anticipated here.
We also need a way to run the shuttle tests in CI, but that is beyond the scope of this PR.
Fixes #783