Jiang Wang .
2021-May-13 23:26 UTC
[RFC v2] virtio-vsock: add description for datagram type
On Mon, May 10, 2021 at 7:52 AM Stefano Garzarella <sgarzare at redhat.com> wrote:> > On Fri, May 07, 2021 at 09:53:19AM -0700, Jiang Wang . wrote: > >Hi guys, > > > >I have one question about adding two new virtqueues for dgram. One new > >thought is that we don't add two new virtqueues but keep using existing > >virtqueues for both stream and dgram. > > > >At the beginning when I first thought about supporting dgram, I thought > >adding two new virtqueues would be easier and have better performance. > >But now, after the prototype is done, I think that to keep using > >existing virtqueues is also not complicated and could in fact be simpler. > >The performance difference may not be very big. > > I honestly didn't think it was easier to use two new queues, quite the > opposite.Got it.> > > >Original code has about 3 places which have assumptions about the > >virtqueues are only used by the stream. But we can change those codes. > >One place is to check pkt len. We can check only for stream pkts. > >Another two are in tx and rx code path where if queued replies pkts are > >too much, the code will stop the rx queue and resume later. We can keep > >that same logic. The dgram will be affected a little bit but that should > >be fine I think. Are there any other places that we should fix? > > Did you take a look at Arseny's series? > I think he's already found the places where to check the type and it > seems to me they are the places you listed.Yes. I checked his patch. And that helps.> > > >In short, the virtqueues are in a lower level and can support multiple > >flows and socket types. Use existing virtqueues also make it more > >compatible with old versions. > > It's not clear to me how compatibility is improved. Can you elaborate on > this?I was thinking if we don't add two new virtqueues, then maybe we don't need to add new feature bit too? If the other end does not support dgram, then the packets will be just dropped. What do you think? Do we still need to add dgram feature bits? I can have a feature bit for mergeable buffer.> > > >What do you guys think? I remember Stefano mentioned that we should add > >two new virtqueues for dgram. Stefano, do you have some specific reasons > >for that? Could we just keep using existing virtqueues? Thanks. > > > > My biggest concern was about the credit mechanism for datagrams. I mean > avoiding datagrams from crowding the queue without limits, preventing > streams from communicating. > > If you've found a way to limit datagram traffic, then maybe it's doable.I see. I will add some limit to dgram packets. Also, when the virtqueues are shared between stream and dgram, both of them need to grab a lock before using the virtqueue, so one will not completely block another one.> Thanks, > Stefano >
Stefano Garzarella
2021-May-14 15:17 UTC
[RFC v2] virtio-vsock: add description for datagram type
On Thu, May 13, 2021 at 04:26:03PM -0700, Jiang Wang . wrote:>On Mon, May 10, 2021 at 7:52 AM Stefano Garzarella <sgarzare at redhat.com> wrote: >> On Fri, May 07, 2021 at 09:53:19AM -0700, Jiang Wang . wrote:[...]>I was thinking if we don't add two new virtqueues, then maybe we don't >need to add new feature bit too? If the other end does not support >dgram, then the packets will be just dropped. What do you think? Do >we still need to add dgram feature bits? I can have a feature bit for >mergeable buffer.With seqpacket, where we reuse stream queues, we decided to add the new feature bit, so I guess we should do the same for dgram. In this way the driver knows if the protocol is supported and we can avoid for example to open a listening socket. Without the feature bit this would not be possible. I mean, the sender will get an error, but the receiver will never know if it can receive or not.>> >What do you guys think? I remember Stefano mentioned that we should >> >add >> >two new virtqueues for dgram. Stefano, do you have some specific reasons >> >for that? Could we just keep using existing virtqueues? Thanks. >> >> My biggest concern was about the credit mechanism for datagrams. I mean >> avoiding datagrams from crowding the queue without limits, preventing >> streams from communicating. >> >> If you've found a way to limit datagram traffic, then maybe it's doable. > >I see. I will add some limit to dgram packets. Also, when the virtqueues >are shared between stream and dgram, both of them need to grab a lock >before using the virtqueue, so one will not completely block another one.I'm not worried about the concurrent access that we definitely need to handle with a lock, but more about the uncontrolled packet sending that dgram might have, flooding the queues and preventing others from communicating. So having 2 dedicated queues could avoid a credit mechanism at all for connection-less sockets, and simply the receiver discards packets that it can't handle. Thanks, Stefano