On Wed, May 15, 2019 at 10:50:43AM +0800, Jason Wang wrote:
>
> On 2019/5/15 上午12:20, Stefano Garzarella wrote:
> > On Tue, May 14, 2019 at 11:38:05AM +0800, Jason Wang wrote:
> > > On 2019/5/14 上午1:51, Stefano Garzarella wrote:
> > > > On Mon, May 13, 2019 at 06:01:52PM +0800, Jason Wang wrote:
> > > > > On 2019/5/10 下午8:58, Stefano Garzarella wrote:
> > > > > > In order to increase host -> guest throughput with large packets,
> > > > > > we can use 64 KiB RX buffers.
> > > > > >
> > > > > > Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> > > > > > ---
> > > > > > include/linux/virtio_vsock.h | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> > > > > > index 84b72026d327..5a9d25be72df 100644
> > > > > > --- a/include/linux/virtio_vsock.h
> > > > > > +++ b/include/linux/virtio_vsock.h
> > > > > > @@ -10,7 +10,7 @@
> > > > > > #define VIRTIO_VSOCK_DEFAULT_MIN_BUF_SIZE 128
> > > > > > #define VIRTIO_VSOCK_DEFAULT_BUF_SIZE (1024 * 256)
> > > > > > #define VIRTIO_VSOCK_DEFAULT_MAX_BUF_SIZE (1024 * 256)
> > > > > > -#define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 4)
> > > > > > +#define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 64)
> > > > > > #define VIRTIO_VSOCK_MAX_BUF_SIZE 0xFFFFFFFFUL
> > > > > > #define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64)
> > > > > We probably don't want such high order allocation. It's better to switch to
> > > > > use order 0 pages in this case. See add_recvbuf_big() for virtio-net. If we
> > > > > get datapath unified, we will get more stuffs set.
> > > > IIUC, you are suggesting to allocate only pages and put them in a
> > > > scatterlist, then add them to the virtqueue.
> > > >
> > > > Is it correct?
> > >
> > > Yes since you are using:
> > >
> > > pkt->buf = kmalloc(buf_len, GFP_KERNEL);
> > > if (!pkt->buf) {
> > > virtio_transport_free_pkt(pkt);
> > > break;
> > > }
> > >
> > > This is likely to fail when the memory is fragmented which is kind of
> > > fragile.
> > >
> > >
> > Thanks for pointing that out.
> >
> > > > The issue that I have here, is that the virtio-vsock guest driver, see
> > > > virtio_vsock_rx_fill(), allocates a struct virtio_vsock_pkt that
> > > > contains the room for the header, then allocates the buffer for the payload.
> > > > At this point it fills the scatterlist with the &virtio_vsock_pkt.hdr and the
> > > > buffer for the payload.
> > >
> > > This part should be fine since what is needed is just adding more pages to
> > > sg[] and call virtuqeueu_add_sg().
> > >
> > >
> > Yes, I agree.
> >
> > > > Changing this will require several modifications, and if we get datapath
> > > > unified, I'm not sure it's worth it.
> > > > Of course, if we leave the datapaths separated, I'd like to do that later.
> > > >
> > > > What do you think?
> > >
> > > For the driver it self, it should not be hard. But I think you mean the
> > > issue of e.g virtio_vsock_pkt itself which doesn't support sg. For short
> > > time, maybe we can use kvec instead.
> > I'll try to use kvec in the virtio_vsock_pkt.
> >
> > Since this struct is shared also with the host driver (vhost-vsock),
> > I hope the changes could be limited, otherwise we can remove the last 2
> > patches of the series for now, leaving the RX buffer size to 4KB.
>
>
> Yes and if it introduces too much changes, maybe we can do the 64KB buffer
> in the future with the conversion of using skb where supports page frag
> natively.
Yes, I completely agree!
Thanks,
Stefano
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
