Support reset for net device
#4389
Conversation
@kalyazin @sudanl0 👋 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4389 +/- ##
==========================================
- Coverage 81.52% 81.51% -0.01%
==========================================
Files 241 241
Lines 29296 29308 +12
==========================================
+ Hits 23883 23891 +8
- Misses 5413 5417 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Rebased. At least some of the build failures were from the branch being stale, I think. The rest ("failed to run custom build command") look like they might be related to seccomp, but there's not much output. |
|
Hi @acj,
I think this is because there is no However, my team talked about this internally and I think we can avoid adding new syscalls. The callsite of |
Signed-off-by: Adam Jensen <[email protected]>
Signed-off-by: Adam Jensen <[email protected]>
Signed-off-by: Adam Jensen <[email protected]>
|
@roypat Hi! No worries, and thanks for the feedback. Changing the reset signature sounds good. I went with |
There was a problem hiding this comment.
Hi Adam,
Thanks a lot for your contribution to Firecracker!
This looks a lot like what I had in mind. One thing that I'd like to understand better is what we should do with the TAP device. I've considered two main alternatives:
- Leave everything as it is. It should probably work.
- Close the open end we are holding during reset and re-create it once we get activated. In fact, we could always defer the opening of the TAP device during activation. This would express better our intentions I think, but there are three problems that I see:
- We will need to change the TAP type we are holding to be an
Option, which is not ideal - We would need to handle the (de)registration of the TAP device with the event loop which might not be trivial
- Opening the TAP device now, adds an extra system call to the boot time which is in the hot path.
- We will need to change the TAP type we are holding to be an
One final thing that bothers me is testing this through integration tests. We can't use FreeBSD to do this. It is not feasible for us to maintain FreeBSD artifacts for testing, so it would need to be done using Linux drivers. However, I haven't found a way to trigger a reset through Linux.
| self.device_state = DeviceState::Inactive; | ||
| self.rx_bytes_read = 0; | ||
| self.rx_deferred_frame = false; | ||
| self.rx_frame_buf = [0u8; MAX_BUFFER_SIZE]; | ||
| self.metrics = NetMetricsPerDevice::alloc(self.id.clone()); |
There was a problem hiding this comment.
We definitely need to reset the acked_features here, too.
Also, @sudanl0 do you think it is ok to reset the metrics here? I think it's more of a philosophical question, on whether we want our metrics to reflect the fact that the device has been reset by zeroing them out. Maybe, it would make sense to add a reset counter metric as well.
| // Test corresponding queues events. | ||
| assert_eq!(net.queue_events().len(), NET_QUEUE_SIZES.len()); | ||
| // Test reset. | ||
| let mut net = net; |
There was a problem hiding this comment.
I guess you can just mark as mut the net device at line 2025?
| fn reset(&mut self) -> Result<(), ResetError> { | ||
| Err(ResetError::NotImplemented) |
There was a problem hiding this comment.
Yes, that's what I had in mind honestly. I assume the initial idea of giving back the EventFds was to "ensure" through the type system that they can't be used, but that is a very dubious choice, IMHO.
|
@bchalios Thanks, that's very helpful. I'll take a closer look at the TAP handling.
I think this might be an issue on the Firecracker side. My understanding of Linux's virtio implementation is crude at best, but it looks like it should reset the device during registration. I added logging code and verified that the Linux driver sets status to 0 during boot, prior to activation. But Firecracker ignores this because it only calls |
|
Hey @acj. Really sorry for silence on this. We got context switched in other topics.
It is. We don't support reset :) I had seen this reset there when I looked at resetting some time ago, but I'm not sure it is enough for testing that reset works (I might be convinced otherwise :)). Ideally, what we could do is load/unload the virtio-net module in the guest. But we explicitly don't want to use modules in the guest for safety reasons. |
|
Hi @acj, just wanted to touch base in order to see if you're still working on this or have plans to do in the near future. |
|
@bchalios 👋 Yes, sorry for the delay on my end. I've been busy with other projects but would still like to land this. I'll find time soon to rebase and understand what's changed. If your team needs this sooner, though, don't let me hold you up. :) |
Changes
This PR implements support for resetting the
netdevice. Resolves #3074.Reason
The VirtIO spec says in section 3.1.1:
Operating systems like FreeBSD and NetBSD reset the device as part of initializing it, which currently causes Firecracker to put the device into a
FAILEDstate. As noted in #3074, some guest OSes continue to have functional networking because they're unaware that the device is in a failed state, but we shouldn't rely on this behavior.Notes
reset()returns a copy of the interrupt FD and queue event FDs, but the MMIO driver does not use them. It's unclear to me how they should be used. Is that handshake only needed for specific drivers? If there were a way to signal that reset had succeeded without needing to clone the FDs, then I could remove the seccomp changes. Perhaps the return type could beResult<Option<...>>instead.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following
Developer Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
CHANGELOG.md.TODOs link to an issue.rust-vmm.