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...@samsung.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 <sgarz...@redhat.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, leona...@redhat.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....@samsung.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....@samsung.com> > > >Missing Co-developed-by? > > >>Signed-off-by: Junnan Wu <junnan01...@samsung.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 > > > > > > > > > >
