virtualization.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
* vsock virtio: questions about supporting DGRAM type
@ 2021-02-12  6:04 Jiang Wang .
  2021-02-12  9:02 ` Stefano Garzarella
  2021-02-23  9:53 ` Michael S. Tsirkin
  0 siblings, 2 replies; 14+ messages in thread
From: Jiang Wang . @ 2021-02-12  6:04 UTC (permalink / raw)
  To: virtualization, asias, imbrenda, stefanha, mst, sgarzare
  Cc: xieyongji, cong.wang, Xiongchun Duan

Hi guys,

I am working on supporting DGRAM type for virtio/vhost vsock. I
already did some work and a draft code is here (which passed my tests,
but still need some cleanup and only works from host to guest as of
now, will add host to guest soon):
https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
qemu changes are here:
https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb

Today, I just noticed that the Asias had an old version of virtio
which had both dgram and stream support, see this link:
https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1

But somehow, the dgram part seems never merged to upstream linux (the
stream part is merged). If so, does anyone know what is the reason for
this? Did we drop dgram support for some specific reason or the code
needs some improvement?

My current code differs from Asias' code in some ways. It does not use
credit and does not support fragmentation.  It basically adds two virt
queues and re-uses the existing functions for tx and rx ( there is
somewhat duplicate code for now, but I will try to make common
functions to reduce it). If we still want to support dgram in upstream
linux, which way do you guys recommend? If necessary, I can try to
base on Asias' old code and continue working on it. If there is
anything unclear, just let me know. Thanks.

Regards,

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: vsock virtio: questions about supporting DGRAM type
  2021-02-12  6:04 vsock virtio: questions about supporting DGRAM type Jiang Wang .
@ 2021-02-12  9:02 ` Stefano Garzarella
  2021-02-13 23:26   ` [External] " Jiang Wang .
       [not found]   ` <e62051f4-e65d-4967-da5c-50ea76f2c783@kaspersky.com>
  2021-02-23  9:53 ` Michael S. Tsirkin
  1 sibling, 2 replies; 14+ messages in thread
From: Stefano Garzarella @ 2021-02-12  9:02 UTC (permalink / raw)
  To: Jiang Wang .
  Cc: cong.wang, Xiongchun Duan, mst, imbrenda, xieyongji, stefanha,
	asias, virtualization, Arseny Krasnov

Hi Jiang,

CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock 
[1].

On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
>Hi guys,
>
>I am working on supporting DGRAM type for virtio/vhost vsock. I
>already did some work and a draft code is here (which passed my tests,
>but still need some cleanup and only works from host to guest as of
>now, will add host to guest soon):
>https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
>qemu changes are here:
>https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
>
>Today, I just noticed that the Asias had an old version of virtio
>which had both dgram and stream support, see this link:
>https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
>
>But somehow, the dgram part seems never merged to upstream linux (the
>stream part is merged). If so, does anyone know what is the reason for
>this? Did we drop dgram support for some specific reason or the code
>needs some improvement?

I wasn't involved yet in virtio-vsock development when Asias posted that 
patches, so I don't know the exact reason.

Maybe could be related on how to handle the credit mechanism for a 
connection-less sockets and how to manage the packet queue, if for 
example no one is listening.

>
>My current code differs from Asias' code in some ways. It does not use
>credit and does not support fragmentation.  It basically adds two virt

If you don't use credit, do you have some threshold when you start to 
drop packets on the RX side?

>queues and re-uses the existing functions for tx and rx ( there is

This make sense, some time ago I was thinking about this and also came 
to the conclusion that 2 new virtqueues were needed to handle DGRAM 
traffic.

>somewhat duplicate code for now, but I will try to make common
>functions to reduce it). If we still want to support dgram in upstream
>linux, which way do you guys recommend? If necessary, I can try to
>base on Asias' old code and continue working on it. If there is
>anything unclear, just let me know. Thanks.

A problem I see is how to handle multiple transports to support nested 
VMs. Since the sockets are not connected, we can't assign them to a 
single transport.

Arseny is working on SOCK_SEQPACKET [1], it's similar to DGRAM, but it 
is connection oriented, so we can reuse most of the STREAM stuff and 
also the credit mechanism.

Maybe you can reuse some of the Arseny's stuff to handle the 
fragmentation.

For the channel type (lossless) I think SEQPACKET makes more sense, but 
if you have any use-cases for DGRAM and want to support it, you are 
welcome to send patches and I will be happy to review them.

Thanks,
Stefano

[1] 
https://lore.kernel.org/lkml/20210207151451.804498-1-arseny.krasnov@kaspersky.com/T/#m81d3ee4ccd54cd301b92c00b3b1bca6e7bc91fc9

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [External] Re: vsock virtio: questions about supporting DGRAM type
  2021-02-12  9:02 ` Stefano Garzarella
@ 2021-02-13 23:26   ` Jiang Wang .
  2021-02-15  8:31     ` Stefano Garzarella
       [not found]   ` <e62051f4-e65d-4967-da5c-50ea76f2c783@kaspersky.com>
  1 sibling, 1 reply; 14+ messages in thread
From: Jiang Wang . @ 2021-02-13 23:26 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: cong.wang, Xiongchun Duan, mst, imbrenda, xieyongji, stefanha,
	asias, virtualization, Arseny Krasnov

On Fri, Feb 12, 2021 at 1:02 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Hi Jiang,
>
> CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock
> [1].
>
> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
> >Hi guys,
> >
> >I am working on supporting DGRAM type for virtio/vhost vsock. I
> >already did some work and a draft code is here (which passed my tests,
> >but still need some cleanup and only works from host to guest as of
> >now, will add host to guest soon):
> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
> >qemu changes are here:
> >https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
> >
> >Today, I just noticed that the Asias had an old version of virtio
> >which had both dgram and stream support, see this link:
> >https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
> >
> >But somehow, the dgram part seems never merged to upstream linux (the
> >stream part is merged). If so, does anyone know what is the reason for
> >this? Did we drop dgram support for some specific reason or the code
> >needs some improvement?
>
> I wasn't involved yet in virtio-vsock development when Asias posted that
> patches, so I don't know the exact reason.
>
> Maybe could be related on how to handle the credit mechanism for a
> connection-less sockets and how to manage the packet queue, if for
> example no one is listening.
>

I see. thanks.

> >
> >My current code differs from Asias' code in some ways. It does not use
> >credit and does not support fragmentation.  It basically adds two virt
>
> If you don't use credit, do you have some threshold when you start to
> drop packets on the RX side?
>

As of now, I don't have any threshold to drop packets on RX side. In my
view, DGRAM is like UDP and is a best effort service. If the virtual queue
is full on TX (or the available buffer size is less than the packet size),
I drop the packets on the TX side.

> >queues and re-uses the existing functions for tx and rx ( there is
>
> This make sense, some time ago I was thinking about this and also came
> to the conclusion that 2 new virtqueues were needed to handle DGRAM
> traffic.
>
Good to know.

> >somewhat duplicate code for now, but I will try to make common
> >functions to reduce it). If we still want to support dgram in upstream
> >linux, which way do you guys recommend? If necessary, I can try to
> >base on Asias' old code and continue working on it. If there is
> >anything unclear, just let me know. Thanks.
>
> A problem I see is how to handle multiple transports to support nested
> VMs. Since the sockets are not connected, we can't assign them to a
> single transport.
>

I did not consider the nested VMs when I started working on this. I
just checked your
nested VM support patch, and I agree it is harder to support it for DGRAM. One
idea is that we can also prepare two transport layers for DGRAM (
similar to STREAM)
and assign transport for every DGRAM packet. The downside is that it will
introduce some overhead. I will think about it more.

> Arseny is working on SOCK_SEQPACKET [1], it's similar to DGRAM, but it
> is connection oriented, so we can reuse most of the STREAM stuff and
> also the credit mechanism.
>
> Maybe you can reuse some of the Arseny's stuff to handle the
> fragmentation.

Sure. I will check Arseny's patch.

> For the channel type (lossless) I think SEQPACKET makes more sense, but
> if you have any use-cases for DGRAM and want to support it, you are
> welcome to send patches and I will be happy to review them.
>
Got it. Thanks. We have some use cases for DGRAM. Basically, we send some
kind of non-critical logging data from guest to the host. It is one way
communication and does not require reliable delivery.  I will continue
working on the patch and send it out for review when it is ready.
Thanks again for all the inputs.

> Thanks,
> Stefano
>
> [1]
> https://lore.kernel.org/lkml/20210207151451.804498-1-arseny.krasnov@kaspersky.com/T/#m81d3ee4ccd54cd301b92c00b3b1bca6e7bc91fc9
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [External] Re: vsock virtio: questions about supporting DGRAM type
       [not found]   ` <e62051f4-e65d-4967-da5c-50ea76f2c783@kaspersky.com>
@ 2021-02-13 23:46     ` Jiang Wang .
       [not found]       ` <f6dd1500-53cf-afb0-ec3c-47de57f8490f@kaspersky.com>
  0 siblings, 1 reply; 14+ messages in thread
From: Jiang Wang . @ 2021-02-13 23:46 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: cong.wang, Xiongchun Duan, mst, virtualization, xieyongji,
	stefanha, asias, imbrenda

On Fri, Feb 12, 2021 at 7:19 AM Arseny Krasnov
<arseny.krasnov@kaspersky.com> wrote:
>
>
> On 12.02.2021 12:02, Stefano Garzarella wrote:
> > Hi Jiang,
> >
> > CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock
> > [1].
> >
> > On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
> >> Hi guys,
> >>
> >> I am working on supporting DGRAM type for virtio/vhost vsock. I
> >> already did some work and a draft code is here (which passed my tests,
> >> but still need some cleanup and only works from host to guest as of
> >> now, will add host to guest soon):
> >> https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
> >> qemu changes are here:
> >> https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
> >>
> >> Today, I just noticed that the Asias had an old version of virtio
> >> which had both dgram and stream support, see this link:
> >> https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
> >>
> >> But somehow, the dgram part seems never merged to upstream linux (the
> >> stream part is merged). If so, does anyone know what is the reason for
> >> this? Did we drop dgram support for some specific reason or the code
> >> needs some improvement?
> > I wasn't involved yet in virtio-vsock development when Asias posted that
> > patches, so I don't know the exact reason.
> >
> > Maybe could be related on how to handle the credit mechanism for a
> > connection-less sockets and how to manage the packet queue, if for
> > example no one is listening.
> >
> >> My current code differs from Asias' code in some ways. It does not use
> >> credit and does not support fragmentation.  It basically adds two virt
> > If you don't use credit, do you have some threshold when you start to
> > drop packets on the RX side?
> >
> >> queues and re-uses the existing functions for tx and rx ( there is
> > This make sense, some time ago I was thinking about this and also came
> > to the conclusion that 2 new virtqueues were needed to handle DGRAM
> > traffic.
>
> Sorry, but what is purpose of two new virt queues?
>
The two new virt queues will be dedicated for the DGRAM packets. Current
queues are used by STREAM (and your SEQPACKET) and the credit
mechanism is bound up with current queues. For DGRAM, the packets can be lost
and no need to use the credit based mechanism. Allocating two new queues
will make this simpler and DGRAM will not interfere with the STREAM
or SEQPACKET.

> >
> >> somewhat duplicate code for now, but I will try to make common
> >> functions to reduce it). If we still want to support dgram in upstream
> >> linux, which way do you guys recommend? If necessary, I can try to
> >> base on Asias' old code and continue working on it. If there is
> >> anything unclear, just let me know. Thanks.
> > A problem I see is how to handle multiple transports to support nested
> > VMs. Since the sockets are not connected, we can't assign them to a
> > single transport.
> >
> > Arseny is working on SOCK_SEQPACKET [1], it's similar to DGRAM, but it
> > is connection oriented, so we can reuse most of the STREAM stuff and
> > also the credit mechanism.
> >
> > Maybe you can reuse some of the Arseny's stuff to handle the
> > fragmentation.
>
> Yes, just apply patchset to latest kernel. I've solved problem
>
> with fragmentation at transport layer.
>

Got it. Thanks. I will check it out.

> >
> > For the channel type (lossless) I think SEQPACKET makes more sense, but
> > if you have any use-cases for DGRAM and want to support it, you are
> > welcome to send patches and I will be happy to review them.
> >
> > Thanks,
> > Stefano
> >
> > [1]
> > https://lore.kernel.org/lkml/20210207151451.804498-1-arseny.krasnov@kaspersky.com/T/#m81d3ee4ccd54cd301b92c00b3b1bca6e7bc91fc9
> >
> >
>
> Thanks, Arseny
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [External] Re: vsock virtio: questions about supporting DGRAM type
  2021-02-13 23:26   ` [External] " Jiang Wang .
@ 2021-02-15  8:31     ` Stefano Garzarella
  2021-02-16  4:50       ` Jiang Wang .
  0 siblings, 1 reply; 14+ messages in thread
From: Stefano Garzarella @ 2021-02-15  8:31 UTC (permalink / raw)
  To: Jiang Wang .
  Cc: cong.wang, Xiongchun Duan, mst, imbrenda, xieyongji, stefanha,
	asias, virtualization, Arseny Krasnov

On Sat, Feb 13, 2021 at 03:26:18PM -0800, Jiang Wang . wrote:
>On Fri, Feb 12, 2021 at 1:02 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> Hi Jiang,
>>
>> CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock
>> [1].
>>
>> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
>> >Hi guys,
>> >
>> >I am working on supporting DGRAM type for virtio/vhost vsock. I
>> >already did some work and a draft code is here (which passed my tests,
>> >but still need some cleanup and only works from host to guest as of
>> >now, will add host to guest soon):
>> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
>> >qemu changes are here:
>> >https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
>> >
>> >Today, I just noticed that the Asias had an old version of virtio
>> >which had both dgram and stream support, see this link:
>> >https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
>> >
>> >But somehow, the dgram part seems never merged to upstream linux (the
>> >stream part is merged). If so, does anyone know what is the reason for
>> >this? Did we drop dgram support for some specific reason or the code
>> >needs some improvement?
>>
>> I wasn't involved yet in virtio-vsock development when Asias posted that
>> patches, so I don't know the exact reason.
>>
>> Maybe could be related on how to handle the credit mechanism for a
>> connection-less sockets and how to manage the packet queue, if for
>> example no one is listening.
>>
>
>I see. thanks.
>
>> >
>> >My current code differs from Asias' code in some ways. It does not use
>> >credit and does not support fragmentation.  It basically adds two virt
>>
>> If you don't use credit, do you have some threshold when you start to
>> drop packets on the RX side?
>>
>
>As of now, I don't have any threshold to drop packets on RX side. In my
>view, DGRAM is like UDP and is a best effort service. If the virtual 
>queue
>is full on TX (or the available buffer size is less than the packet size),
>I drop the packets on the TX side.

I have to check the code, my concern is we should have a limit for the 
allocation, for example if the user space doesn't consume RX packets.

>
>> >queues and re-uses the existing functions for tx and rx ( there is
>>
>> This make sense, some time ago I was thinking about this and also came
>> to the conclusion that 2 new virtqueues were needed to handle DGRAM
>> traffic.
>>
>Good to know.
>
>> >somewhat duplicate code for now, but I will try to make common
>> >functions to reduce it). If we still want to support dgram in upstream
>> >linux, which way do you guys recommend? If necessary, I can try to
>> >base on Asias' old code and continue working on it. If there is
>> >anything unclear, just let me know. Thanks.
>>
>> A problem I see is how to handle multiple transports to support nested
>> VMs. Since the sockets are not connected, we can't assign them to a
>> single transport.
>>
>
>I did not consider the nested VMs when I started working on this. I
>just checked your
>nested VM support patch, and I agree it is harder to support it for DGRAM. One
>idea is that we can also prepare two transport layers for DGRAM (
>similar to STREAM)

Yeah, or just remove the VSOCK_TRANSPORT_F_DGRAM and use the same flags 
used for STREAM also for DGRAM. If the transport does not support DGRAM, 
we return an error.

>and assign transport for every DGRAM packet. The downside is that it 
>will
>introduce some overhead. I will think about it more.

I agree that maybe we should check the right transport for every DGRAM 
packet.

>
>> Arseny is working on SOCK_SEQPACKET [1], it's similar to DGRAM, but it
>> is connection oriented, so we can reuse most of the STREAM stuff and
>> also the credit mechanism.
>>
>> Maybe you can reuse some of the Arseny's stuff to handle the
>> fragmentation.
>
>Sure. I will check Arseny's patch.
>
>> For the channel type (lossless) I think SEQPACKET makes more sense, but
>> if you have any use-cases for DGRAM and want to support it, you are
>> welcome to send patches and I will be happy to review them.
>>
>Got it. Thanks. We have some use cases for DGRAM. Basically, we send some
>kind of non-critical logging data from guest to the host. It is one way
>communication and does not require reliable delivery.  I will continue
>working on the patch and send it out for review when it is ready.
>Thanks again for all the inputs.

I see, in fact DGRAM might make sense when we have a single receiver 
(host) and many transmitters (guests).

Thanks,
Stefano

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [External] Re: vsock virtio: questions about supporting DGRAM type
  2021-02-15  8:31     ` Stefano Garzarella
@ 2021-02-16  4:50       ` Jiang Wang .
  2021-02-16  8:09         ` Stefano Garzarella
  0 siblings, 1 reply; 14+ messages in thread
From: Jiang Wang . @ 2021-02-16  4:50 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: cong.wang, Xiongchun Duan, mst, imbrenda, xieyongji, stefanha,
	asias, virtualization, Arseny Krasnov

On Mon, Feb 15, 2021 at 12:31 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Sat, Feb 13, 2021 at 03:26:18PM -0800, Jiang Wang . wrote:
> >On Fri, Feb 12, 2021 at 1:02 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> Hi Jiang,
> >>
> >> CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock
> >> [1].
> >>
> >> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
> >> >Hi guys,
> >> >
> >> >I am working on supporting DGRAM type for virtio/vhost vsock. I
> >> >already did some work and a draft code is here (which passed my tests,
> >> >but still need some cleanup and only works from host to guest as of
> >> >now, will add host to guest soon):
> >> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
> >> >qemu changes are here:
> >> >https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
> >> >
> >> >Today, I just noticed that the Asias had an old version of virtio
> >> >which had both dgram and stream support, see this link:
> >> >https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
> >> >
> >> >But somehow, the dgram part seems never merged to upstream linux (the
> >> >stream part is merged). If so, does anyone know what is the reason for
> >> >this? Did we drop dgram support for some specific reason or the code
> >> >needs some improvement?
> >>
> >> I wasn't involved yet in virtio-vsock development when Asias posted that
> >> patches, so I don't know the exact reason.
> >>
> >> Maybe could be related on how to handle the credit mechanism for a
> >> connection-less sockets and how to manage the packet queue, if for
> >> example no one is listening.
> >>
> >
> >I see. thanks.
> >
> >> >
> >> >My current code differs from Asias' code in some ways. It does not use
> >> >credit and does not support fragmentation.  It basically adds two virt
> >>
> >> If you don't use credit, do you have some threshold when you start to
> >> drop packets on the RX side?
> >>
> >
> >As of now, I don't have any threshold to drop packets on RX side. In my
> >view, DGRAM is like UDP and is a best effort service. If the virtual
> >queue
> >is full on TX (or the available buffer size is less than the packet size),
> >I drop the packets on the TX side.
>
> I have to check the code, my concern is we should have a limit for the
> allocation, for example if the user space doesn't consume RX packets.
>

I think we already have a limit for the allocation. If a DGRAM client
sends packets while no socket is bound on the server side ,
the packet will be dropped when looking for the socket. If the socket is
bound on the server side, but the server program somehow does not
call recv or similar functions, the packets will be dropped when the
buffer is full as in your previous patch at here:
https://lore.kernel.org/patchwork/patch/1141083/
If there are still other cases that we don't have protection, let me know and
I can add some threshold.

> >
> >> >queues and re-uses the existing functions for tx and rx ( there is
> >>
> >> This make sense, some time ago I was thinking about this and also came
> >> to the conclusion that 2 new virtqueues were needed to handle DGRAM
> >> traffic.
> >>
> >Good to know.
> >
> >> >somewhat duplicate code for now, but I will try to make common
> >> >functions to reduce it). If we still want to support dgram in upstream
> >> >linux, which way do you guys recommend? If necessary, I can try to
> >> >base on Asias' old code and continue working on it. If there is
> >> >anything unclear, just let me know. Thanks.
> >>
> >> A problem I see is how to handle multiple transports to support nested
> >> VMs. Since the sockets are not connected, we can't assign them to a
> >> single transport.
> >>
> >
> >I did not consider the nested VMs when I started working on this. I
> >just checked your
> >nested VM support patch, and I agree it is harder to support it for DGRAM. One
> >idea is that we can also prepare two transport layers for DGRAM (
> >similar to STREAM)
>
> Yeah, or just remove the VSOCK_TRANSPORT_F_DGRAM and use the same flags
> used for STREAM also for DGRAM. If the transport does not support DGRAM,
> we return an error.
>
> >and assign transport for every DGRAM packet. The downside is that it
> >will
> >introduce some overhead. I will think about it more.
>
> I agree that maybe we should check the right transport for every DGRAM
> packet.
>
> >
> >> Arseny is working on SOCK_SEQPACKET [1], it's similar to DGRAM, but it
> >> is connection oriented, so we can reuse most of the STREAM stuff and
> >> also the credit mechanism.
> >>
> >> Maybe you can reuse some of the Arseny's stuff to handle the
> >> fragmentation.
> >
> >Sure. I will check Arseny's patch.
> >
> >> For the channel type (lossless) I think SEQPACKET makes more sense, but
> >> if you have any use-cases for DGRAM and want to support it, you are
> >> welcome to send patches and I will be happy to review them.
> >>
> >Got it. Thanks. We have some use cases for DGRAM. Basically, we send some
> >kind of non-critical logging data from guest to the host. It is one way
> >communication and does not require reliable delivery.  I will continue
> >working on the patch and send it out for review when it is ready.
> >Thanks again for all the inputs.
>
> I see, in fact DGRAM might make sense when we have a single receiver
> (host) and many transmitters (guests).
>
> Thanks,
> Stefano
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [External] Re: vsock virtio: questions about supporting DGRAM type
       [not found]       ` <f6dd1500-53cf-afb0-ec3c-47de57f8490f@kaspersky.com>
@ 2021-02-16  5:43         ` Jiang Wang .
  0 siblings, 0 replies; 14+ messages in thread
From: Jiang Wang . @ 2021-02-16  5:43 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: cong.wang, Xiongchun Duan, mst, virtualization, xieyongji,
	stefanha, asias, imbrenda

On Mon, Feb 15, 2021 at 12:53 AM Arseny Krasnov
<arseny.krasnov@kaspersky.com> wrote:
>
>
> On 14.02.2021 02:46, Jiang Wang . wrote:
> > On Fri, Feb 12, 2021 at 7:19 AM Arseny Krasnov
> > <arseny.krasnov@kaspersky.com> wrote:
> >>
> >> On 12.02.2021 12:02, Stefano Garzarella wrote:
> >>> Hi Jiang,
> >>>
> >>> CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock
> >>> [1].
> >>>
> >>> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
> >>>> Hi guys,
> >>>>
> >>>> I am working on supporting DGRAM type for virtio/vhost vsock. I
> >>>> already did some work and a draft code is here (which passed my tests,
> >>>> but still need some cleanup and only works from host to guest as of
> >>>> now, will add host to guest soon):
> >>>> https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
> >>>> qemu changes are here:
> >>>> https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
> >>>>
> >>>> Today, I just noticed that the Asias had an old version of virtio
> >>>> which had both dgram and stream support, see this link:
> >>>> https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
> >>>>
> >>>> But somehow, the dgram part seems never merged to upstream linux (the
> >>>> stream part is merged). If so, does anyone know what is the reason for
> >>>> this? Did we drop dgram support for some specific reason or the code
> >>>> needs some improvement?
> >>> I wasn't involved yet in virtio-vsock development when Asias posted that
> >>> patches, so I don't know the exact reason.
> >>>
> >>> Maybe could be related on how to handle the credit mechanism for a
> >>> connection-less sockets and how to manage the packet queue, if for
> >>> example no one is listening.
> >>>
> >>>> My current code differs from Asias' code in some ways. It does not use
> >>>> credit and does not support fragmentation.  It basically adds two virt
> >>> If you don't use credit, do you have some threshold when you start to
> >>> drop packets on the RX side?
> >>>
> >>>> queues and re-uses the existing functions for tx and rx ( there is
> >>> This make sense, some time ago I was thinking about this and also came
> >>> to the conclusion that 2 new virtqueues were needed to handle DGRAM
> >>> traffic.
> >> Sorry, but what is purpose of two new virt queues?
> >>
> > The two new virt queues will be dedicated for the DGRAM packets. Current
> > queues are used by STREAM (and your SEQPACKET) and the credit
> > mechanism is bound up with current queues. For DGRAM, the packets can be lost
> > and no need to use the credit based mechanism. Allocating two new queues
> > will make this simpler and DGRAM will not interfere with the STREAM
> > or SEQPACKET.
>
> I think that credit mechanism is per socket(e.g. every socket has
>
> its own "free space"). If socket has free space, but queue is full,
>
> packet still inserted in tx list. Otherwise if queue is empty, but
>
> receiver didn't send credit update to make sender happy(free space
>
> is 0) - sender will sleep. I think both of them are independent.

Thanks for the explanation and I agree with your above description.
The data structure for credit is
per socket. But there is a subtle assumption for STREAM or connection
oriented communications. When the client sends the first connect message
to the server, it does not know the credit of the server. Current code
relies on the pkt_len == 0 to send it out, as the following:

/* virtio_transport_get_credit might return less than pkt_len credit */
pkt_len = virtio_transport_get_credit(vvs, pkt_len);

/* Do not send zero length OP_RW pkt */
if (pkt_len == 0 && info->op == VIRTIO_VSOCK_OP_RW)
return pkt_len;

DGRAM will be different. DGRAM does not require a
connect call, and the first message will have a non zero pkt_len.
We could somehow improve the current credit mechanism
and make it work for DGRAM too, but I don't think it is a strict
requirement for DGRAM.

Going back to the original question about why adding two more
new virtqueues, another reason is for better performance and
performance isolation. If DGRAM and STREAM share the same
queues, and both have heavy in-flight data, I think the throughput
of the STREAM will be decreased. Also, if the DGRAM server
somehow does not process packets for a long time, the packets
will be kept in the queue and also interfere with the STREAM
packets. I think adding two more queues without a credit
mechanism is the best option for DGRAM. If there are other
reasons to use credit or not adding two more queues, please
let me know. Thanks


> >
> >>>> somewhat duplicate code for now, but I will try to make common
> >>>> functions to reduce it). If we still want to support dgram in upstream
> >>>> linux, which way do you guys recommend? If necessary, I can try to
> >>>> base on Asias' old code and continue working on it. If there is
> >>>> anything unclear, just let me know. Thanks.
> >>> A problem I see is how to handle multiple transports to support nested
> >>> VMs. Since the sockets are not connected, we can't assign them to a
> >>> single transport.
> >>>
> >>> Arseny is working on SOCK_SEQPACKET [1], it's similar to DGRAM, but it
> >>> is connection oriented, so we can reuse most of the STREAM stuff and
> >>> also the credit mechanism.
> >>>
> >>> Maybe you can reuse some of the Arseny's stuff to handle the
> >>> fragmentation.
> >> Yes, just apply patchset to latest kernel. I've solved problem
> >>
> >> with fragmentation at transport layer.
> >>
> > Got it. Thanks. I will check it out.
> >
> >>> For the channel type (lossless) I think SEQPACKET makes more sense, but
> >>> if you have any use-cases for DGRAM and want to support it, you are
> >>> welcome to send patches and I will be happy to review them.
> >>>
> >>> Thanks,
> >>> Stefano
> >>>
> >>> [1]
> >>> https://lore.kernel.org/lkml/20210207151451.804498-1-arseny.krasnov@kaspersky.com/T/#m81d3ee4ccd54cd301b92c00b3b1bca6e7bc91fc9
> >>>
> >>>
> >> Thanks, Arseny
> >>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [External] Re: vsock virtio: questions about supporting DGRAM type
  2021-02-16  4:50       ` Jiang Wang .
@ 2021-02-16  8:09         ` Stefano Garzarella
  2021-02-16  8:23           ` Jiang Wang .
  0 siblings, 1 reply; 14+ messages in thread
From: Stefano Garzarella @ 2021-02-16  8:09 UTC (permalink / raw)
  To: Jiang Wang .
  Cc: cong.wang, Xiongchun Duan, mst, imbrenda, xieyongji, stefanha,
	asias, virtualization, Arseny Krasnov

On Mon, Feb 15, 2021 at 08:50:36PM -0800, Jiang Wang . wrote:
>On Mon, Feb 15, 2021 at 12:31 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Sat, Feb 13, 2021 at 03:26:18PM -0800, Jiang Wang . wrote:
>> >On Fri, Feb 12, 2021 at 1:02 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >>
>> >> Hi Jiang,
>> >>
>> >> CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock
>> >> [1].
>> >>
>> >> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
>> >> >Hi guys,
>> >> >
>> >> >I am working on supporting DGRAM type for virtio/vhost vsock. I
>> >> >already did some work and a draft code is here (which passed my tests,
>> >> >but still need some cleanup and only works from host to guest as of
>> >> >now, will add host to guest soon):
>> >> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
>> >> >qemu changes are here:
>> >> >https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
>> >> >
>> >> >Today, I just noticed that the Asias had an old version of virtio
>> >> >which had both dgram and stream support, see this link:
>> >> >https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
>> >> >
>> >> >But somehow, the dgram part seems never merged to upstream linux (the
>> >> >stream part is merged). If so, does anyone know what is the reason for
>> >> >this? Did we drop dgram support for some specific reason or the code
>> >> >needs some improvement?
>> >>
>> >> I wasn't involved yet in virtio-vsock development when Asias posted that
>> >> patches, so I don't know the exact reason.
>> >>
>> >> Maybe could be related on how to handle the credit mechanism for a
>> >> connection-less sockets and how to manage the packet queue, if for
>> >> example no one is listening.
>> >>
>> >
>> >I see. thanks.
>> >
>> >> >
>> >> >My current code differs from Asias' code in some ways. It does not use
>> >> >credit and does not support fragmentation.  It basically adds two virt
>> >>
>> >> If you don't use credit, do you have some threshold when you start to
>> >> drop packets on the RX side?
>> >>
>> >
>> >As of now, I don't have any threshold to drop packets on RX side. In my
>> >view, DGRAM is like UDP and is a best effort service. If the virtual
>> >queue
>> >is full on TX (or the available buffer size is less than the packet size),
>> >I drop the packets on the TX side.
>>
>> I have to check the code, my concern is we should have a limit for the
>> allocation, for example if the user space doesn't consume RX packets.
>>
>
>I think we already have a limit for the allocation. If a DGRAM client
>sends packets while no socket is bound on the server side ,
>the packet will be dropped when looking for the socket. If the socket is
>bound on the server side, but the server program somehow does not
>call recv or similar functions, the packets will be dropped when the
>buffer is full as in your previous patch at here:
>https://lore.kernel.org/patchwork/patch/1141083/
>If there are still other cases that we don't have protection, let me know and
>I can add some threshold.

Maybe I misunderstood, but I thought you didn't use credit for DGRAM 
messages.

If you use it to limit buffer occupancy, then it should be okay, but 
again, I still have to look at the code :-)

Thanks,
Stefano

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [External] Re: vsock virtio: questions about supporting DGRAM type
  2021-02-16  8:09         ` Stefano Garzarella
@ 2021-02-16  8:23           ` Jiang Wang .
  2021-02-16  8:50             ` Stefano Garzarella
  0 siblings, 1 reply; 14+ messages in thread
From: Jiang Wang . @ 2021-02-16  8:23 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: cong.wang, Xiongchun Duan, mst, imbrenda, xieyongji, stefanha,
	asias, virtualization, Arseny Krasnov

On Tue, Feb 16, 2021 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Mon, Feb 15, 2021 at 08:50:36PM -0800, Jiang Wang . wrote:
> >On Mon, Feb 15, 2021 at 12:31 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> On Sat, Feb 13, 2021 at 03:26:18PM -0800, Jiang Wang . wrote:
> >> >On Fri, Feb 12, 2021 at 1:02 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >> >>
> >> >> Hi Jiang,
> >> >>
> >> >> CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock
> >> >> [1].
> >> >>
> >> >> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
> >> >> >Hi guys,
> >> >> >
> >> >> >I am working on supporting DGRAM type for virtio/vhost vsock. I
> >> >> >already did some work and a draft code is here (which passed my tests,
> >> >> >but still need some cleanup and only works from host to guest as of
> >> >> >now, will add host to guest soon):
> >> >> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
> >> >> >qemu changes are here:
> >> >> >https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
> >> >> >
> >> >> >Today, I just noticed that the Asias had an old version of virtio
> >> >> >which had both dgram and stream support, see this link:
> >> >> >https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
> >> >> >
> >> >> >But somehow, the dgram part seems never merged to upstream linux (the
> >> >> >stream part is merged). If so, does anyone know what is the reason for
> >> >> >this? Did we drop dgram support for some specific reason or the code
> >> >> >needs some improvement?
> >> >>
> >> >> I wasn't involved yet in virtio-vsock development when Asias posted that
> >> >> patches, so I don't know the exact reason.
> >> >>
> >> >> Maybe could be related on how to handle the credit mechanism for a
> >> >> connection-less sockets and how to manage the packet queue, if for
> >> >> example no one is listening.
> >> >>
> >> >
> >> >I see. thanks.
> >> >
> >> >> >
> >> >> >My current code differs from Asias' code in some ways. It does not use
> >> >> >credit and does not support fragmentation.  It basically adds two virt
> >> >>
> >> >> If you don't use credit, do you have some threshold when you start to
> >> >> drop packets on the RX side?
> >> >>
> >> >
> >> >As of now, I don't have any threshold to drop packets on RX side. In my
> >> >view, DGRAM is like UDP and is a best effort service. If the virtual
> >> >queue
> >> >is full on TX (or the available buffer size is less than the packet size),
> >> >I drop the packets on the TX side.
> >>
> >> I have to check the code, my concern is we should have a limit for the
> >> allocation, for example if the user space doesn't consume RX packets.
> >>
> >
> >I think we already have a limit for the allocation. If a DGRAM client
> >sends packets while no socket is bound on the server side ,
> >the packet will be dropped when looking for the socket. If the socket is
> >bound on the server side, but the server program somehow does not
> >call recv or similar functions, the packets will be dropped when the
> >buffer is full as in your previous patch at here:
> >https://lore.kernel.org/patchwork/patch/1141083/
> >If there are still other cases that we don't have protection, let me know and
> >I can add some threshold.
>
> Maybe I misunderstood, but I thought you didn't use credit for DGRAM
> messages.
>
I don't use credit for DGRAM. But the dropping code I mentioned does not
rely on credit too. Just to make it clear, following is one part of code that
I referred to:

static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
struct virtio_vsock_pkt *pkt)
{
if (vvs->rx_bytes + pkt->len > vvs->buf_alloc)
return false;

vvs->rx_bytes += pkt->len;
return true;
}


> If you use it to limit buffer occupancy, then it should be okay, but
> again, I still have to look at the code :-)

Sure. I think you mean you need to look at my final patches. I will
send it later.  Just a friendly reminder, if you just want to get some
idea of my patches, you could check this link :
https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d

thanks. :)

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [External] Re: vsock virtio: questions about supporting DGRAM type
  2021-02-16  8:23           ` Jiang Wang .
@ 2021-02-16  8:50             ` Stefano Garzarella
  2021-02-16 16:54               ` Jiang Wang .
  0 siblings, 1 reply; 14+ messages in thread
From: Stefano Garzarella @ 2021-02-16  8:50 UTC (permalink / raw)
  To: Jiang Wang .
  Cc: cong.wang, Xiongchun Duan, mst, imbrenda, xieyongji, stefanha,
	asias, virtualization, Arseny Krasnov

On Tue, Feb 16, 2021 at 12:23:19AM -0800, Jiang Wang . wrote:
>On Tue, Feb 16, 2021 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Mon, Feb 15, 2021 at 08:50:36PM -0800, Jiang Wang . wrote:
>> >On Mon, Feb 15, 2021 at 12:31 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >>
>> >> On Sat, Feb 13, 2021 at 03:26:18PM -0800, Jiang Wang . wrote:
>> >> >On Fri, Feb 12, 2021 at 1:02 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >> >>
>> >> >> Hi Jiang,
>> >> >>
>> >> >> CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock
>> >> >> [1].
>> >> >>
>> >> >> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
>> >> >> >Hi guys,
>> >> >> >
>> >> >> >I am working on supporting DGRAM type for virtio/vhost vsock. I
>> >> >> >already did some work and a draft code is here (which passed my tests,
>> >> >> >but still need some cleanup and only works from host to guest as of
>> >> >> >now, will add host to guest soon):
>> >> >> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
>> >> >> >qemu changes are here:
>> >> >> >https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
>> >> >> >
>> >> >> >Today, I just noticed that the Asias had an old version of virtio
>> >> >> >which had both dgram and stream support, see this link:
>> >> >> >https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
>> >> >> >
>> >> >> >But somehow, the dgram part seems never merged to upstream linux (the
>> >> >> >stream part is merged). If so, does anyone know what is the reason for
>> >> >> >this? Did we drop dgram support for some specific reason or the code
>> >> >> >needs some improvement?
>> >> >>
>> >> >> I wasn't involved yet in virtio-vsock development when Asias posted that
>> >> >> patches, so I don't know the exact reason.
>> >> >>
>> >> >> Maybe could be related on how to handle the credit mechanism for a
>> >> >> connection-less sockets and how to manage the packet queue, if for
>> >> >> example no one is listening.
>> >> >>
>> >> >
>> >> >I see. thanks.
>> >> >
>> >> >> >
>> >> >> >My current code differs from Asias' code in some ways. It does not use
>> >> >> >credit and does not support fragmentation.  It basically adds two virt
>> >> >>
>> >> >> If you don't use credit, do you have some threshold when you start to
>> >> >> drop packets on the RX side?
>> >> >>
>> >> >
>> >> >As of now, I don't have any threshold to drop packets on RX side. In my
>> >> >view, DGRAM is like UDP and is a best effort service. If the virtual
>> >> >queue
>> >> >is full on TX (or the available buffer size is less than the 
>> >> >packet size),
>> >> >I drop the packets on the TX side.
>> >>
>> >> I have to check the code, my concern is we should have a limit for the
>> >> allocation, for example if the user space doesn't consume RX packets.
>> >>
>> >
>> >I think we already have a limit for the allocation. If a DGRAM client
>> >sends packets while no socket is bound on the server side ,
>> >the packet will be dropped when looking for the socket. If the socket is
>> >bound on the server side, but the server program somehow does not
>> >call recv or similar functions, the packets will be dropped when the
>> >buffer is full as in your previous patch at here:
>> >https://lore.kernel.org/patchwork/patch/1141083/
>> >If there are still other cases that we don't have protection, let me know and
>> >I can add some threshold.
>>
>> Maybe I misunderstood, but I thought you didn't use credit for DGRAM
>> messages.
>>
>I don't use credit for DGRAM. But the dropping code I mentioned does not
>rely on credit too. Just to make it clear, following is one part of code that
>I referred to:
>
>static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
>struct virtio_vsock_pkt *pkt)
>{
>if (vvs->rx_bytes + pkt->len > vvs->buf_alloc)
>return false;
>
>vvs->rx_bytes += pkt->len;
>return true;
>}

Okay, now it's clear. The transmitter doesn't care about the receiver's 
credit, but the receiver uses that part of the credit mechanism to 
discard packets.

I think that's fine, but when you send the patches, explain it well in 
the cover letter/commit message.

>
>
>> If you use it to limit buffer occupancy, then it should be okay, but
>> again, I still have to look at the code :-)
>
>Sure. I think you mean you need to look at my final patches. I will
>send it later.  Just a friendly reminder, if you just want to get some
>idea of my patches, you could check this link :
>https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d

It's my fault because I didn't have time, I saw the link. :-)

Remember that we should also plan changes to the VIRTIO spec for the 
DGRAM support.

Thanks,
Stefano

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [External] Re: vsock virtio: questions about supporting DGRAM type
  2021-02-16  8:50             ` Stefano Garzarella
@ 2021-02-16 16:54               ` Jiang Wang .
  0 siblings, 0 replies; 14+ messages in thread
From: Jiang Wang . @ 2021-02-16 16:54 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: cong.wang, Xiongchun Duan, mst, imbrenda, xieyongji, stefanha,
	asias, virtualization, Arseny Krasnov

On Tue, Feb 16, 2021 at 12:50 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Tue, Feb 16, 2021 at 12:23:19AM -0800, Jiang Wang . wrote:
> >On Tue, Feb 16, 2021 at 12:09 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> On Mon, Feb 15, 2021 at 08:50:36PM -0800, Jiang Wang . wrote:
> >> >On Mon, Feb 15, 2021 at 12:31 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >> >>
> >> >> On Sat, Feb 13, 2021 at 03:26:18PM -0800, Jiang Wang . wrote:
> >> >> >On Fri, Feb 12, 2021 at 1:02 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >> >> >>
> >> >> >> Hi Jiang,
> >> >> >>
> >> >> >> CCing Arseny who is working on SOCK_SEQPACKET support for virtio-vsock
> >> >> >> [1].
> >> >> >>
> >> >> >> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
> >> >> >> >Hi guys,
> >> >> >> >
> >> >> >> >I am working on supporting DGRAM type for virtio/vhost vsock. I
> >> >> >> >already did some work and a draft code is here (which passed my tests,
> >> >> >> >but still need some cleanup and only works from host to guest as of
> >> >> >> >now, will add host to guest soon):
> >> >> >> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
> >> >> >> >qemu changes are here:
> >> >> >> >https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
> >> >> >> >
> >> >> >> >Today, I just noticed that the Asias had an old version of virtio
> >> >> >> >which had both dgram and stream support, see this link:
> >> >> >> >https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
> >> >> >> >
> >> >> >> >But somehow, the dgram part seems never merged to upstream linux (the
> >> >> >> >stream part is merged). If so, does anyone know what is the reason for
> >> >> >> >this? Did we drop dgram support for some specific reason or the code
> >> >> >> >needs some improvement?
> >> >> >>
> >> >> >> I wasn't involved yet in virtio-vsock development when Asias posted that
> >> >> >> patches, so I don't know the exact reason.
> >> >> >>
> >> >> >> Maybe could be related on how to handle the credit mechanism for a
> >> >> >> connection-less sockets and how to manage the packet queue, if for
> >> >> >> example no one is listening.
> >> >> >>
> >> >> >
> >> >> >I see. thanks.
> >> >> >
> >> >> >> >
> >> >> >> >My current code differs from Asias' code in some ways. It does not use
> >> >> >> >credit and does not support fragmentation.  It basically adds two virt
> >> >> >>
> >> >> >> If you don't use credit, do you have some threshold when you start to
> >> >> >> drop packets on the RX side?
> >> >> >>
> >> >> >
> >> >> >As of now, I don't have any threshold to drop packets on RX side. In my
> >> >> >view, DGRAM is like UDP and is a best effort service. If the virtual
> >> >> >queue
> >> >> >is full on TX (or the available buffer size is less than the
> >> >> >packet size),
> >> >> >I drop the packets on the TX side.
> >> >>
> >> >> I have to check the code, my concern is we should have a limit for the
> >> >> allocation, for example if the user space doesn't consume RX packets.
> >> >>
> >> >
> >> >I think we already have a limit for the allocation. If a DGRAM client
> >> >sends packets while no socket is bound on the server side ,
> >> >the packet will be dropped when looking for the socket. If the socket is
> >> >bound on the server side, but the server program somehow does not
> >> >call recv or similar functions, the packets will be dropped when the
> >> >buffer is full as in your previous patch at here:
> >> >https://lore.kernel.org/patchwork/patch/1141083/
> >> >If there are still other cases that we don't have protection, let me know and
> >> >I can add some threshold.
> >>
> >> Maybe I misunderstood, but I thought you didn't use credit for DGRAM
> >> messages.
> >>
> >I don't use credit for DGRAM. But the dropping code I mentioned does not
> >rely on credit too. Just to make it clear, following is one part of code that
> >I referred to:
> >
> >static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
> >struct virtio_vsock_pkt *pkt)
> >{
> >if (vvs->rx_bytes + pkt->len > vvs->buf_alloc)
> >return false;
> >
> >vvs->rx_bytes += pkt->len;
> >return true;
> >}
>
> Okay, now it's clear. The transmitter doesn't care about the receiver's
> credit, but the receiver uses that part of the credit mechanism to
> discard packets.
>
> I think that's fine, but when you send the patches, explain it well in
> the cover letter/commit message.
>
Sure, thanks.

> >
> >
> >> If you use it to limit buffer occupancy, then it should be okay, but
> >> again, I still have to look at the code :-)
> >
> >Sure. I think you mean you need to look at my final patches. I will
> >send it later.  Just a friendly reminder, if you just want to get some
> >idea of my patches, you could check this link :
> >https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
>
> It's my fault because I didn't have time, I saw the link. :-)
>
Got it. No problem. thanks. :)

> Remember that we should also plan changes to the VIRTIO spec for the
> DGRAM support.
>
Sure.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: vsock virtio: questions about supporting DGRAM type
  2021-02-12  6:04 vsock virtio: questions about supporting DGRAM type Jiang Wang .
  2021-02-12  9:02 ` Stefano Garzarella
@ 2021-02-23  9:53 ` Michael S. Tsirkin
  2021-03-12  4:57   ` [External] " Jiang Wang .
  1 sibling, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2021-02-23  9:53 UTC (permalink / raw)
  To: Jiang Wang .
  Cc: cong.wang, Xiongchun Duan, imbrenda, xieyongji, stefanha, asias,
	virtualization

On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
> Hi guys,
> 
> I am working on supporting DGRAM type for virtio/vhost vsock. I
> already did some work and a draft code is here (which passed my tests,
> but still need some cleanup and only works from host to guest as of
> now, will add host to guest soon):
> https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
> qemu changes are here:
> https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
> 
> Today, I just noticed that the Asias had an old version of virtio
> which had both dgram and stream support, see this link:
> https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
> 
> But somehow, the dgram part seems never merged to upstream linux (the
> stream part is merged). If so, does anyone know what is the reason for
> this? Did we drop dgram support for some specific reason or the code
> needs some improvement?

It's not just code it's the spec that needs work.

See some issues pointed out here:

https://lists.oasis-open.org/archives/virtio-dev/201506/msg00003.html



> My current code differs from Asias' code in some ways. It does not use
> credit and does not support fragmentation.  It basically adds two virt
> queues and re-uses the existing functions for tx and rx ( there is
> somewhat duplicate code for now, but I will try to make common
> functions to reduce it). If we still want to support dgram in upstream
> linux, which way do you guys recommend? If necessary, I can try to
> base on Asias' old code and continue working on it. If there is
> anything unclear, just let me know. Thanks.
> 
> Regards,
> 
> Jiang

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [External] Re: vsock virtio: questions about supporting DGRAM type
  2021-02-23  9:53 ` Michael S. Tsirkin
@ 2021-03-12  4:57   ` Jiang Wang .
  2021-03-14 22:19     ` Michael S. Tsirkin
  0 siblings, 1 reply; 14+ messages in thread
From: Jiang Wang . @ 2021-03-12  4:57 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: cong.wang, Xiongchun Duan, imbrenda, xieyongji, stefanha, asias,
	virtualization

Hi Michael,

Sorry for the late reply. I just saw your email yesterday somehow.

I read the email thread you mentioned, and I think the issue with
dgram is that it may drop packets because the sender cannot track the
tx_cnt with subtracting it from peer_fwd_cnt.

I agree with Stefan that the dgram is a best-effort service and may
drop packets. For the sender, I just add a maximum buffer size to
limit the memory usage. On the receiving side, I reuse the existing
virtio_transport_inc_rx_pkt() that Stefano added a year ago to limit
the memory usage. This will avoid denial of service attack to the
other end (host or guest VM).

For the application of dgram, we will use it for some remote logging
application. The application running in the VM will write some logs to
a server running on the host. This is  one way communication and the
log is not critical.

Regards,

Jiang

On Tue, Feb 23, 2021 at 1:53 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Feb 11, 2021 at 10:04:34PM -0800, Jiang Wang . wrote:
> > Hi guys,
> >
> > I am working on supporting DGRAM type for virtio/vhost vsock. I
> > already did some work and a draft code is here (which passed my tests,
> > but still need some cleanup and only works from host to guest as of
> > now, will add host to guest soon):
> > https://github.com/Jiang1155/linux/commit/4e89736e0bce15496460ff411cb4694b143d1c3d
> > qemu changes are here:
> > https://github.com/Jiang1155/qemu/commit/7ab778801e3e8969ab98e44539943810a2fb03eb
> >
> > Today, I just noticed that the Asias had an old version of virtio
> > which had both dgram and stream support, see this link:
> > https://kvm.vger.kernel.narkive.com/BMvH9eEr/rfc-v2-0-7-introduce-vm-sockets-virtio-transport#post1
> >
> > But somehow, the dgram part seems never merged to upstream linux (the
> > stream part is merged). If so, does anyone know what is the reason for
> > this? Did we drop dgram support for some specific reason or the code
> > needs some improvement?
>
> It's not just code it's the spec that needs work.
>
> See some issues pointed out here:
>
> https://lists.oasis-open.org/archives/virtio-dev/201506/msg00003.html
>
>
>
> > My current code differs from Asias' code in some ways. It does not use
> > credit and does not support fragmentation.  It basically adds two virt
> > queues and re-uses the existing functions for tx and rx ( there is
> > somewhat duplicate code for now, but I will try to make common
> > functions to reduce it). If we still want to support dgram in upstream
> > linux, which way do you guys recommend? If necessary, I can try to
> > base on Asias' old code and continue working on it. If there is
> > anything unclear, just let me know. Thanks.
> >
> > Regards,
> >
> > Jiang
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [External] Re: vsock virtio: questions about supporting DGRAM type
  2021-03-12  4:57   ` [External] " Jiang Wang .
@ 2021-03-14 22:19     ` Michael S. Tsirkin
  0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2021-03-14 22:19 UTC (permalink / raw)
  To: Jiang Wang .
  Cc: cong.wang, Xiongchun Duan, imbrenda, xieyongji, stefanha, asias,
	virtualization

On Thu, Mar 11, 2021 at 08:57:08PM -0800, Jiang Wang . wrote:
> Hi Michael,
> 
> Sorry for the late reply. I just saw your email yesterday somehow.
> 
> I read the email thread you mentioned, and I think the issue with
> dgram is that it may drop packets because the sender cannot track the
> tx_cnt with subtracting it from peer_fwd_cnt.
> 
> I agree with Stefan that the dgram is a best-effort service and may
> drop packets. For the sender, I just add a maximum buffer size to
> limit the memory usage. On the receiving side, I reuse the existing
> virtio_transport_inc_rx_pkt() that Stefano added a year ago to limit
> the memory usage. This will avoid denial of service attack to the
> other end (host or guest VM).
> 
> For the application of dgram, we will use it for some remote logging
> application. The application running in the VM will write some logs to
> a server running on the host. This is  one way communication and the
> log is not critical.
> 
> Regards,
> 
> Jiang

To make things short, please submit to the virtio TC a spec patch
documenting how is dgram supposed to work and we'll discuss.  The
existing mechanism was designed with a stream socket in mind.  dgram is
best-effort but some fairness could be a quality of implementation
issue.  I appreciate that your specific application might not care about
such concerns but we do need to worry about building a widely reuseable
interface as opposed to a very narrow one.

-- 
MST

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-03-14 22:19 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-12  6:04 vsock virtio: questions about supporting DGRAM type Jiang Wang .
2021-02-12  9:02 ` Stefano Garzarella
2021-02-13 23:26   ` [External] " Jiang Wang .
2021-02-15  8:31     ` Stefano Garzarella
2021-02-16  4:50       ` Jiang Wang .
2021-02-16  8:09         ` Stefano Garzarella
2021-02-16  8:23           ` Jiang Wang .
2021-02-16  8:50             ` Stefano Garzarella
2021-02-16 16:54               ` Jiang Wang .
     [not found]   ` <e62051f4-e65d-4967-da5c-50ea76f2c783@kaspersky.com>
2021-02-13 23:46     ` Jiang Wang .
     [not found]       ` <f6dd1500-53cf-afb0-ec3c-47de57f8490f@kaspersky.com>
2021-02-16  5:43         ` Jiang Wang .
2021-02-23  9:53 ` Michael S. Tsirkin
2021-03-12  4:57   ` [External] " Jiang Wang .
2021-03-14 22:19     ` Michael S. Tsirkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).