Please read the links we already shared with you!!!

No MIME, no links, no compression, no attachments. Just plain text

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#no-mime-no-links-no-compression-no-attachments-just-plain-text


On Tue, 11 Feb 2025 at 06:24, 吴俊南 [off-list ref] wrote:

Hello leonardi  and  stefanha：

    Thanks for your review. And I will add other maintainers CCd in 
    next push. And I want to discuss more about the second patch.

Why are you sending a v2 if we didn't reach an agreement on the second 
patch?

    Firstly, we think our scenarios are quite different.

These are the information that should be put in the commit description.
We are not oracles imagining scenarios....

Our scenario is  virtio-vsock deployed in embeded environment, and 
suspend to ram is for order to allow system run at low power 
consumption. In this scenario, the AF_VSOCK socket is created by Guest 
upper application and don't close after driver freeze. Once restore, 
the connection which are communicating before will be failed. It will 
cause that upper application based on vsock connect failed. In this 
mode, guest haven't received the event to close all connections.  
That's difference with you metioned.

I mentioned the second scenario just as an example.

    Secondly, refer to socket based on virtio-net device, they don't 
    close connected during freeze.

    Here we did a test that:

   Start iperf server based on virtio-net in Host.
   Start iperf client based on virtio-net in Guest and keep 
   communicating with server.
   Suspend Guest
   Resume Guest.

    Here in virtio-net, the iperf communication is still working after 
    these steps. But iperf based on vsock will fail. We think it 
    should keep same reaction with virtio-net

I agree that it would be cool, but this patch is not the right way as I 
explained in the previous email.

virtio-net can easily discard packets because it's an ethernet device.
As I already explained, virtio-vsock guarantees ordering and delivery of 
packets via virtqueues, if these disappear, you have to add something on 
top that keeps track of undelivered packets.

    Thirdly, accroding to virtio-spec, vsock facilitates data transfer 
    between the guest and device without using the Ethernet or IP 
    protocols.

What does this have to do with packet loss?
It simply says that vsock does not need a classic TCP/IP stack, but 
directly connects guest and host sockets via virtqueues.

    Therefore we think packets lost is acceptable for it, and it is 
    not necessary to keep those packet during suspend flow.

Where did you read that packet loss is acceptable?

From https://docs.oasis-open.org/virtio/virtio/v1.3/csd01/virtio-v1.3-csd01.html#x1-4800006

  5.10.6.2 Addressing

  ...

  Currently stream and seqpacket sockets are supported. type is 1
  (VIRTIO_VSOCK_TYPE_STREAM) for stream socket types, and 2
  (VIRTIO_VSOCK_TYPE_SEQPACKET) for seqpacket socket types.

  #define VIRTIO_VSOCK_TYPE_STREAM    1
  #define VIRTIO_VSOCK_TYPE_SEQPACKET 2

  Stream sockets provide in-order, guaranteed, connection-oriented
  delivery without message boundaries. Seqpacket sockets provide
  in-order, guaranteed, connection-oriented delivery with message and
  record boundaries.


Please explain in the commit description how this change ensures the 
requirements of the specification: "in-order, guaranteed, 
connection-oriented delivery".

Thanks,
Stefano

 Best Wish

--------- Original Message ---------

Sender : Stefano Garzarella [off-list ref]

Date : 2025-02-11 00:52 (GMT+8)

Title : Re: [PATCH 2/2] vsock/virtio: Don't reset the created SOCKET during s2r

 

On Mon, Feb 10, 2025 at 12:48:03PM +0100, leonardi@redhat.com wrote:

quoted

Like for the other patch, some maintainers have not been CCd.

Yes, please use `scripts/get_maintainer.pl`.

quoted

quoted

On Fri, Feb 07, 2025 at 01:20:33PM +0800, Junnan Wu wrote:

quoted

quoted

From: Ying Gao <redacted>

quoted

quoted

quoted

quoted

If suspend is executed during vsock communication and the

quoted

quoted

socket is reset, the original socket will be unusable after resume.

Why? (I mean for a good commit description)

quoted

quoted

quoted

quoted

Judge the value of vdev->priv in function virtio_vsock_vqs_del,

quoted

quoted

only when the function is invoked by virtio_vsock_remove,

quoted

quoted

all vsock connections will be reset.

quoted

quoted

quoted

The second part of the commit message is not that clear, do you mind

quoted

rephrasing it?

+1 on that


Also in this case, why checking `vdev->priv` fixes the issue?

quoted

quoted

quoted

Signed-off-by: Ying Gao <redacted>

quoted

Missing Co-developed-by?

quoted

quoted

Signed-off-by: Junnan Wu <redacted>

quoted

quoted

quoted

quoted

---

quoted

quoted

net/vmw_vsock/virtio_transport.c | 6 ++++--

quoted

quoted

1 file changed, 4 insertions(+), 2 deletions(-)

quoted

quoted

quoted

quoted

diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c

quoted

quoted

index 9eefd0fba92b..9df609581755 100644

quoted

quoted

--- a/net/vmw_vsock/virtio_transport.c

quoted

quoted

+++ b/net/vmw_vsock/virtio_transport.c

quoted

quoted

@@ -717,8 +717,10 @@ static void virtio_vsock_vqs_del(struct virtio_vsock *vsock)

quoted

quoted

       struct sk_buff *skb;

quoted

quoted

quoted

quoted

       /* Reset all connected sockets when the VQs disappear */

quoted

quoted

-        vsock_for_each_connected_socket(&virtio_transport.transport,

quoted

quoted

-                                        virtio_vsock_reset_sock);

quoted

I would add a comment explaining why you are adding this check.

Yes, please.

quoted

quoted

+        if (!vdev->priv) {

quoted

quoted

+                vsock_for_each_connected_socket(&virtio_transport.transport,

quoted

quoted

+                                                virtio_vsock_reset_sock);

quoted

quoted

+        }

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

quoted

quoted

quoted

quoted

       /* Stop all work handlers to make sure no one is accessing the device,

quoted

quoted

        * so we can safely call virtio_reset_device().

quoted

quoted

--

quoted

quoted

2.34.1

quoted

quoted

quoted

quoted

I am not familiar with freeze/resume, but I don't see any problems

quoted

with this patch.

quoted

quoted

Thank you,

quoted

Luigi

quoted