netdev: add queue-get and queue-create with queue leasing - #1195
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Linux netdev generic-netlink support for queue contracts, queue retrieval, queue creation, nested attribute parsing, and kernel integration tests for leases and network namespace IDs. ChangesLinux Netdev Queue Management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Handle
participant GenericNetlink
participant LinuxNetdev
Caller->>Handle: NetDevQueueCreate(request)
Handle->>GenericNetlink: send queue-create attributes
GenericNetlink->>LinuxNetdev: execute netdev request
LinuxNetdev-->>GenericNetlink: ACK and queue response
GenericNetlink-->>Handle: return created queue ID
Handle-->>Caller: return queue ID
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netdev_linux_test.go`:
- Line 68: The test uses fixed interface names ("nltestnk0" and "nltestnk1")
which can collide with stale interfaces and cause flaky LinkAdd failures; change
the constants to generate unique names per test (e.g., append a test-specific
suffix using t.Name(), testing.T.TempDir() hash, process id or time-based/random
suffix) so each run creates distinct interface names referenced where
"nltestnk0"/"nltestnk1" are used (search for the constants and LinkAdd calls in
netdev_linux_test.go, including the other occurrence around line 118) and ensure
cleanup still targets the generated names.
- Around line 44-47: The current checks call t.Skipf for any non-nil err (the
"queue-get on lo rx-0" block and the similar block at lines 101-105), which can
mask real encoding/decoding bugs; change these to only skip when the error
clearly indicates the queue/feature is genuinely absent (e.g. match the specific
expected sentinel or syscall error like ENOENT/ENOTSUP/ENXIO returned by the
netlink call), and otherwise call t.Fatalf or t.Errorf to fail the test; update
the two occurrences that use t.Skipf (the "queue-get on lo rx-0" check and the
later analogous check) to perform a targeted error-match then skip, else fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fcf3b7e-a293-4c06-85bd-80c0e4cc26a2
📒 Files selected for processing (3)
netdev_linux.gonetdev_linux_test.gonl/netdev_linux.go
6f8d9bb to
6edaf35
Compare
6edaf35 to
6f8d8fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netdev_linux_test.go`:
- Around line 1-14: Update handle_retry_linux_test.go to stop calling
RetryInterrupted as a method; access it as the Handle.option boolean field when
configuring or asserting dumpHandle, so the netlink test package compiles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 80f9fa3d-4cc0-46ca-8138-34b2f09023eb
📒 Files selected for processing (3)
netdev_linux.gonetdev_linux_test.gonl/netdev_linux.go
🚧 Files skipped from review as they are similar to previous changes (1)
- nl/netdev_linux.go
6f8d8fd to
3723edf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netdev_linux_test.go`:
- Around line 21-24: Update the setup error handling around GenlFamilyGet and
the other referenced test setup paths to skip only when the error explicitly
indicates the netdev generic-netlink family or feature is unavailable; call
t.Fatalf for all unexpected errors. For new_device ID collisions, retry setup
with a new ID rather than skipping, preserving skips only for genuinely
unavailable environments.
In `@netdev_linux.go`:
- Around line 178-232: Validate req.Lease.NetNSID in Handle.NetDevQueueCreate
before encoding NETDEV_A_LEASE_NETNS_ID; when NetNSIDSet is true, reject
negative values by returning a descriptive error. Preserve the existing
attribute encoding for non-negative IDs and the omission behavior when
NetNSIDSet is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 70535091-47db-46fe-b058-6e4de6ff4db0
📒 Files selected for processing (4)
handle_retry_linux_test.gonetdev_linux.gonetdev_linux_test.gonl/netdev_linux.go
🚧 Files skipped from review as they are similar to previous changes (1)
- nl/netdev_linux.go
3723edf to
a1c53b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netdev_linux.go`:
- Around line 84-152: Validate that each relevant attribute value has at least
four bytes before calling native.Uint32 in parseNetDevQueueID,
parseNetDevQueueLease, and parseNetDevQueue, covering all queue ID, type,
ifindex, netns ID, and NAPI ID fields. Return an appropriate Go error for short
values so malformed replies never reach the decoder or panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 404d5e61-6040-441e-b8f2-35479cc830c1
📒 Files selected for processing (4)
handle_retry_linux_test.gonetdev_linux.gonetdev_linux_test.gonl/netdev_linux.go
🚧 Files skipped from review as they are similar to previous changes (2)
- handle_retry_linux_test.go
- nl/netdev_linux.go
a1c53b3 to
f24113e
Compare
Add the "netdev" generic netlink family with NetDevQueueGet (decodes the nested lease attribute) and NetDevQueueCreate (creates an rx queue on a virtual device and leases it to a real queue on a physical device). Queue leasing lets a virtual netdev proxy a physical NIC rx queue so io_uring zero-copy and AF_XDP can address it by (ifindex, queue-id). The lease nest is encoded with NLA_F_NESTED and may carry an optional netns-id. Mirrors include/uapi/linux/netdev.h. Tests skip unless the kernel supports the commands. Tests use netdevsim as a hardware-free lease source (it implements the kernel's queue management ops): TestNetDevQueueLeaseRoundTrip leases a netdevsim rx queue to a netkit peer and reads it back, asserting the decoded lease matches exactly. It requires root and is skipped when netdevsim or the queue commands are unavailable. Signed-off-by: Aaron Campbell <aaron@monkey.org>
f24113e to
7f9257a
Compare
Add the "netdev" generic netlink family with NetDevQueueGet (decodes the nested lease attribute) and NetDevQueueCreate (creates an rx queue on a virtual device and leases it to a real queue on a physical device).
Queue leasing lets a virtual netdev proxy a physical NIC rx queue so io_uring zero-copy and AF_XDP can address it by (ifindex, queue-id). The lease nest is encoded with NLA_F_NESTED and may carry an optional netns-id. Mirrors include/uapi/linux/netdev.h. Tests skip unless the kernel supports the commands.
Tests use netdevsim as a hardware-free lease source (it implements the kernel's queue management ops): TestNetDevQueueLeaseRoundTrip leases a netdevsim rx queue to a netkit peer and reads it back, asserting the decoded lease matches exactly. It requires root and is skipped when netdevsim or the queue commands are unavailable.
Also performed a manual test on arm64 by creating a netdevsim interface w/ 2 queues:
And then exercising the new APIs:
Test program source:
These changes will help Cilium take advantage of the zero-copy networking features added in Linux 7.1—see the upstream queue-leasing merge.
Summary by CodeRabbit
Summary by CodeRabbit