On Mon, Feb 10, 2025 at 12:48:03PM +0100, leonardi@xxxxxxxxxx wrote:
Like for the other patch, some maintainers have not been CCd.
Yes, please use `scripts/get_maintainer.pl`.
On Fri, Feb 07, 2025 at 01:20:33PM +0800, Junnan Wu wrote:
From: Ying Gao <ying01.gao@xxxxxxxxxxx>
If suspend is executed during vsock communication and the
socket is reset, the original socket will be unusable after resume.
Why? (I mean for a good commit description)
Judge the value of vdev->priv in function virtio_vsock_vqs_del,
only when the function is invoked by virtio_vsock_remove,
all vsock connections will be reset.
The second part of the commit message is not that clear, do you mind
rephrasing it?
+1 on that
Also in this case, why checking `vdev->priv` fixes the issue?
Signed-off-by: Ying Gao <ying01.gao@xxxxxxxxxxx>
Missing Co-developed-by?
Signed-off-by: Junnan Wu <junnan01.wu@xxxxxxxxxxx>
---
net/vmw_vsock/virtio_transport.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
index 9eefd0fba92b..9df609581755 100644
--- a/net/vmw_vsock/virtio_transport.c
+++ b/net/vmw_vsock/virtio_transport.c
@@ -717,8 +717,10 @@ static void virtio_vsock_vqs_del(struct virtio_vsock *vsock)
struct sk_buff *skb;
/* Reset all connected sockets when the VQs disappear */
- vsock_for_each_connected_socket(&virtio_transport.transport,
- virtio_vsock_reset_sock);
I would add a comment explaining why you are adding this check.
Yes, please.
+ if (!vdev->priv) {
+ vsock_for_each_connected_socket(&virtio_transport.transport,
+ virtio_vsock_reset_sock);
+ }
Okay, after looking at the code I understood why, but please write it
into the commit next time!
virtio_vsock_vqs_del() is called in 2 cases:
1 - in virtio_vsock_remove() after setting `vdev->priv` to null since
the drive is about to be unloaded because the device is for example
removed (hot-unplug)
2 - in virtio_vsock_freeze() when suspending, but in this case
`vdev->priv` is not touched.
I don't think is a good idea using that because in the future it could
change. So better to add a parameter to virtio_vsock_vqs_del() to
differentiate the 2 use cases.
That said, I think this patch is wrong:
We are deallocating virtqueues, so all packets that are "in flight" will
be completely discarded. Our transport (virtqueues) has no mechanism to
retransmit them, so those packets would be lost forever. So we cannot
guarantee the reliability of SOCK_STREAM sockets for example.
In any case, after a suspension, many connections will be expired in the
host anyway, so does it make sense to keep them open in the guest?
If you want to support this use case, you must first provide a way to
keep those packets somewhere (e.g. avoiding to remove the virtqueues?),
but I honestly don't understand the use case.
To be clear, this behavior is intended, and it's for example the same as
when suspending the VM is the hypervisor directly, which after that, it
sends an event to the guest, just to close all connections because it's
complicated to keep them active.
Thanks,
Stefano
/* Stop all work handlers to make sure no one is accessing the device,
* so we can safely call virtio_reset_device().
--
2.34.1
I am not familiar with freeze/resume, but I don't see any problems
with this patch.
Thank you,
Luigi
