Re: [RFC v2] virtio-vsock: add description for datagram type

From: Stefano Garzarella <sgarzare@redhat.com>
To: "Jiang Wang ." <jiang.wang@bytedance.com>
Cc: cong.wang@bytedance.com,
	Xiongchun Duan <duanxiongchun@bytedance.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	cohuck@redhat.com, virtualization@lists.linux-foundation.org,
	xieyongji@bytedance.com, Stefan Hajnoczi <stefanha@redhat.com>,
	asias@redhat.com, Arseny Krasnov <arseny.krasnov@kaspersky.com>
Subject: Re: [RFC v2] virtio-vsock: add description for datagram type
Date: Tue, 4 May 2021 18:16:51 +0200	[thread overview]
Message-ID: <20210504161651.3b6fhi64d7g3jui4@steredhat> (raw)
In-Reply-To: <CAP_N_Z9xPgyf=au4VD+fXF8iorZHHk7Q4-CbJCOVUeupG4v4sw@mail.gmail.com>

Hi Jiang,

On Mon, May 03, 2021 at 08:40:46PM -0700, Jiang Wang . wrote:
>Hi Stefano,
>
>I checked the VIRTIO_NET_F_MRG_RXBUF feature bit and I think vsock
>dgram can use that feature too.

Cool, thanks for checking!

>Do we want to make this feature a must-have or optional? One idea is
>to make it optional. When not

I think optional is fine, and we should support it for all kind of 
traffic (stream, dgram, seqpacket).

>supported, dgram rx buf is 16 KB which should be good in most cases.

Why not 4 KB like for stream? Or we could make it configurable.

>When VIRTIO_NET_F_MRG_RXBUF is supported, the rx buf is 4K and the max
>packet size is 64 KB.
>
>Also, just to make sure we are on the same page, the current vsock
>stream code can also split a
>big packet to multiple buffers and the receive side can assemble them
>together.

Yes, sort of. Being a stream, there's no concept of a boundary.

> But dgram cannot
>use that code because the dgram may drop a buffer in the driver code
>(if there is not enough space).
>That means dgram may drop some buffers at the beginning, in the end or in the
>middle of a pkt. And a packet may
>not be received as a complete one. Therefore, we need something like
>VIRTIO_NET_F_MRG_RXBUF.

Yep.

>
>If we want to leverage current stream code without using 
>VIRTIO_NET_F_MRG_RXBUF,
>we could add a total_len and offset to the virtio_vsock_hdr. Then when sending
>packet, the device split the big packet to multiple small ones and
>each has a header. They will have the
>same total_len, but different offsets. On the driver side, the driver
>can check the total_len before
>enqueueing the big packet for the one with offset 0.
>If there is enough space, all the remaining packets will be received.
>If not, the remaining packets will be dropped.
>I feel this implementation might be easier than using
>VIRTIO_NET_F_MRG_RXBUF. But either one is fine with me.
>Any preference? Thanks.

This is very similar to what we discussed with Michael. He pointed out 
that it could be complicated and we could have several problems.

For example, we should also provide an ID to prevent different fragments 
from overlapping. Also we might have problems handling different flows 
at the same time.

Mergable buffers allow us to avoid these problems and also bring 
advantages for the other types of traffic (stream, seqpacket).

It also allows us to use a single header for the packet and all its 
fragments.

So IMHO, if there are no significant issues, the best way would be to 
implement mergeable buffers in vsock,
I think there are only advantages to using this feature.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization