| Message ID | 20190510125843.95587-8-sgarzare@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2,1/8] vsock/virtio: limit the memory used per-socket | expand |
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@redhat.com> > --- > 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. Thanks
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@redhat.com> > > --- > > 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? 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. 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? Thanks, Stefano
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@redhat.com> >>> --- >>> 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. > > 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(). > > 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. Thanks > > Thanks, > Stefano
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@redhat.com> > > > > --- > > > > 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. Thanks, Stefano
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@redhat.com> >>>>> --- >>>>> 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. Thanks > > Thanks, > Stefano
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@redhat.com> > > > > > > --- > > > > > > 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
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)
In order to increase host -> guest throughput with large packets, we can use 64 KiB RX buffers. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- include/linux/virtio_vsock.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)