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

["Jiang Wang ." <jiang.wang@bytedance.com>]

On Tue, May 4, 2021 at 9:16 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> 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!

NP.

> >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).

Got it. I was thinking only for dgram originally, but I think it should be fine
for stream and seqpacket too.

Btw, I have a small implementation question. For now, the vsock
allocates rx buffers with two scatterlist. One for header and one for the
payload. After we enable VIRTIO_NET_F_MRG_RXBUF feature,
do we still want to allocate buffers like that? Or could we just use
one big scatterlist for the whole packet? I think using the same allocation
method is fine, but it just may not line up with the real packets well since
we will skip headers for the big packets except the first buffer.

> >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.

OK. sure. 4 KB is fine with me. I mentioned 16 KB because I was thinking
jumbo frames in the ethernet world. But  I just found out the jumbo frame
is about 8 KB or 9 KB only.

If we make it configurable, what kind of interface to use to configure it?
In linux, we could use something like the sysfs interface. I guess we don't
need to specify that detail in the spec though. I will just put the size should
be configurable in the spec.

> >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.

Sure. Got it. I was thinking only about dgram, which is simpler than
stream and seqpacket. For those two, they will have issues as you
just mentioned.

Also, just to make sure. For steam and seqpacket, supporting
mergeable buffers is mainly for performance improvements,
right? Or to save memory? I think functionally, they will be the
same with or without
mergeable buffers. For dgram, the maximum supported packet size
is increased when using MRG_RXBUF if the rx buf size is fixed,
and it can save lots of memory.

I am a little bit confused about the motivation to support mergeable
buffers for stream and seqpacket. Could you remind me again? Sorry
that if it was already mentioned in the old emails.

We could only support it on dgram since dgram has its own virtqueues.

btw, my company email system automatically adds [External] to
these emails, and I meant to remove it manually when I reply,
but forgot to do that sometimes, so the email threading may not
be very accurate.
Sorry about that.


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