viona: only reset in-range queues #1047
Conversation
45af0f7 was too eager to reset rings. by resetting all rings regardless of previous SET_PAIRS, any RING_RESET beyond that point would EINVAL. Propolis then tracks that ring state as VRingState::Fatal, at which point operations like VqChange::Addresss immediately give up. I don't actaully understand what is different about the Ubuntu 24.04 I'm using versus the one we saw this issue on. By tracing viona operations with viona.d it's quite clear that something in the image on mb-0 is resetting the device before (during?) Linux booting, and that is not at all happening locally. In both cases the kernels are 6.8, but the patch levels are slightly different: the problematic image is 6.8.0-94-generic versus my 6.8.0-88-generic. Without this tracking of which rings are eligible *for* reset, that first device reset effectively bricks the NIC for the guest. None of my Ubuntu 24.04, 22.04, or Debian 13 images have this behavior, which is upsetting in its own right. For good measure, I've checked this change on FreeBSD 14.1 and a relatively recent helios as well. At least as far as my (apparently well-behaved??) images go, the viona NIC functions at boot, after a reboot, can seemingly do TCP without issue, etc. My test Windows image has not had functional networking since the VirtIO/multiqueue work landed, and is at least not *less* functional now. It's not clear if that's a my-image problem or a Propolis problem.
iximeow
added
bug
lib/propolis/src/hw/virtio/queue.rs
Outdated
| } | ||
| self.len.store(len, Ordering::Release); | ||
| let mut peak = self.peak.load(Ordering::Acquire); | ||
| loop { |
There was a problem hiding this comment.
careful attention to the cas-loop-under-duress would be appreciated...
There was a problem hiding this comment.
So just FYI, I think that atomicity here was used mainly as a proxy for interior mutability, rather than for concurrency.
There was a problem hiding this comment.
yeah, and len specifically wants to be outside a mutex so we don't need to lock it for each VirtQueues::get
lib/propolis/src/hw/virtio/queue.rs
Outdated
| loop { | ||
| if len <= peak { | ||
| break; | ||
| } |
There was a problem hiding this comment.
is this not traditionally spelt
| loop { | |
| if len <= peak { | |
| break; | |
| } | |
| while len > peak { |
There was a problem hiding this comment.
while i do love saying "eh" to tradition, uhh, you're right lol
| /// VirtQueues may be lower than a previous high watermark, but in cases | ||
| /// like device reset and teardown we must manage all viona rings | ||
| /// corresponding to ever-active VirtQueues. | ||
| pub fn iter_all(&self) -> impl std::iter::Iterator<Item = &Arc<VirtQueue>> { |
There was a problem hiding this comment.
I wonder about only resetting those queues that are not in the initial/reset state themselves? That is, can we examine the state before issuing the reset?
There was a problem hiding this comment.
my read is that that truly is allowed-but-pointless, versus the current behavior of resetting queues beyond USE_PAIRS that gets us the EINVAL and is really not
answer: nothing interesting. my local configs all have a without this patch that reset when getting out of OVMF and into Linux would reset the other 20 rings and render the NIC unusable. removing the boot order entry immediately reproduces both the issue and fix. the bug manifests slightly differently on Debian 13/kernel 6.12 where the NIC is listed in |
3 participants
45af0f7 was too eager to reset rings. by resetting all rings regardless of previous SET_PAIRS, any RING_RESET beyond that point would EINVAL. Propolis then tracks that ring state as VRingState::Fatal, at which point operations like VqChange::Addresss immediately give up. I don't follow exactly how this turns into a non-functional NIC from the guest perspective, but Propolis is clearly operating viona incorrectly here.
I don't understand what is different about the Ubuntu 24.04 I'm using versus the one we saw this issue on. By tracing viona operations with viona.d it's quite clear that something in the image on mb-0 is resetting the device before (during?) Linux booting, and that is not at all happening locally. In both cases the kernels are 6.8, but the patch levels are slightly different: the problematic image is 6.8.0-94-generic versus my 6.8.0-88-generic.
Without this tracking of which rings are eligible for reset, that first device reset effectively bricks the NIC for the guest.
None of my Ubuntu 24.04, 22.04, or Debian 13 images have this behavior, which is upsetting in its own right. For good measure, I've checked this change on FreeBSD 14.1 and a relatively recent helios as well. At least as far as my (apparently well-behaved??) images go, the viona NIC functions at boot, after a reboot, can seemingly do TCP without issue, etc.
My test Windows image has not had functional networking since the VirtIO/multiqueue work landed, and is at least not less functional now. It's not clear if that's a my-image problem or a Propolis problem.