Message-ID: <CAGxU2F6KYsAdedHVpopNz1vCMeduiy570_7LLysdKeBb74d9iw@mail.gmail.com>
Date: Tue, 11 Feb 2025 09:35:20 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: junnan01.wu@...sung.com
Cc: "leonardi@...hat.com" <leonardi@...hat.com>, 
	"stefanha@...hat.com" <stefanha@...hat.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>, 
	"virtualization@...ts.linux-foundation.org" <virtualization@...ts.linux-foundation.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
	赵民栋 <mindong.zhao@...sung.com>, 黄青 <q1.huang@...sung.com>, 
	高英 <ying01.gao@...sung.com>, 许莹 <ying123.xu@...sung.com>, 
	王磊 <lei19.wang@...sung.com>
Subject: Re: Re: [PATCH 2/2] vsock/virtio: Don't reset the created SOCKET
 during s2r

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, 吴俊南 <junnan01.wu@...sung.com> 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 <sgarzare@...hat.com>
>
> 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@...hat.com 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@...sung.com>
>
> >>
>
> >>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@...sung.com>
>
> >Missing Co-developed-by?
>
> >>Signed-off-by: Junnan Wu <junnan01.wu@...sung.com>
>
> >
>
> >
>
> >>---
>
> >>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
>
> >
>
>
>  
>
>  
>
>

Powered by blists - more mailing lists