Jiang Wang .
2021-Feb-16 08:23 UTC
[External] Re: vsock virtio: questions about supporting DGRAM type
On Tue, Feb 16, 2021 at 12:09 AM Stefano Garzarella <sgarzare at 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 at 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 at 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 >
Stefano Garzarella
2021-Feb-16 08:50 UTC
[External] Re: vsock virtio: questions about supporting DGRAM type
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 at 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 at 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 at 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/4e89736e0bce15496460ff411cb4694b143d1c3dIt'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