Linux Virtualization - May 2019 - [PATCH v2 8/8] vsock/virtio: make the RX buffer size tunable

On 2019/5/10 ??8:58, Stefano Garzarella wrote:
> The RX buffer size determines the memory consumption of the
> vsock/virtio guest driver, so we make it tunable through
> a module parameter.
>
> The size allowed are between 4 KB and 64 KB in order to be
> compatible with old host drivers.
>
> Suggested-by: Stefan Hajnoczi <stefanha at redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>

I don't see much value of doing this through kernel command line. We 
should deal with them automatically like what virtio-net did. Or even a 
module parameter is better.

Thanks

> ---
>   include/linux/virtio_vsock.h     |  1 +
>   net/vmw_vsock/virtio_transport.c | 27 ++++++++++++++++++++++++++-
>   2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
> index 5a9d25be72df..b9f8c3d91f80 100644
> --- a/include/linux/virtio_vsock.h
> +++ b/include/linux/virtio_vsock.h
> @@ -13,6 +13,7 @@
>   #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)
> +#define VIRTIO_VSOCK_MIN_PKT_BUF_SIZE		(1024 * 4)
>   
>   enum {
>   	VSOCK_VQ_RX     = 0, /* for host to guest data */
> diff --git a/net/vmw_vsock/virtio_transport.c
b/net/vmw_vsock/virtio_transport.c
> index af1d2ce12f54..732398b4e28f 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -66,6 +66,31 @@ struct virtio_vsock {
>   	u32 guest_cid;
>   };
>   
> +static unsigned int rx_buf_size = VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE;
> +
> +static int param_set_rx_buf_size(const char *val, const struct
kernel_param *kp)
> +{
> +	unsigned int size;
> +	int ret;
> +
> +	ret = kstrtouint(val, 0, &size);
> +	if (ret)
> +		return ret;
> +
> +	if (size < VIRTIO_VSOCK_MIN_PKT_BUF_SIZE ||
> +	    size > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE)
> +		return -EINVAL;
> +
> +	return param_set_uint(val, kp);
> +};
> +
> +static const struct kernel_param_ops param_ops_rx_buf_size = {
> +	.set = param_set_rx_buf_size,
> +	.get = param_get_uint,
> +};
> +
> +module_param_cb(rx_buf_size, &param_ops_rx_buf_size, &rx_buf_size,
0644);
> +
>   static struct virtio_vsock *virtio_vsock_get(void)
>   {
>   	return the_virtio_vsock;
> @@ -261,7 +286,7 @@ virtio_transport_cancel_pkt(struct vsock_sock *vsk)
>   
>   static void virtio_vsock_rx_fill(struct virtio_vsock *vsock)
>   {
> -	int buf_len = VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE;
> +	int buf_len = rx_buf_size;
>   	struct virtio_vsock_pkt *pkt;
>   	struct scatterlist hdr, buf, *sgs[2];
>   	struct virtqueue *vq;

On 2019/5/13 ??6:05, Jason Wang wrote:
>
> On 2019/5/10 ??8:58, Stefano Garzarella wrote:
>> The RX buffer size determines the memory consumption of the
>> vsock/virtio guest driver, so we make it tunable through
>> a module parameter.
>>
>> The size allowed are between 4 KB and 64 KB in order to be
>> compatible with old host drivers.
>>
>> Suggested-by: Stefan Hajnoczi <stefanha at redhat.com>
>> Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
>
>
> I don't see much value of doing this through kernel command line. We 
> should deal with them automatically like what virtio-net did. Or even 
> a module parameter is better.
>
> Thanks

Sorry, I misread the patch. But even module parameter is something not 
flexible enough. We should deal with them transparently.

Thanks

>
>
>> ---
>> ? include/linux/virtio_vsock.h???? |? 1 +
>> ? net/vmw_vsock/virtio_transport.c | 27 ++++++++++++++++++++++++++-
>> ? 2 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/virtio_vsock.h
b/include/linux/virtio_vsock.h
>> index 5a9d25be72df..b9f8c3d91f80 100644
>> --- a/include/linux/virtio_vsock.h
>> +++ b/include/linux/virtio_vsock.h
>> @@ -13,6 +13,7 @@
>> ? #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)
>> +#define VIRTIO_VSOCK_MIN_PKT_BUF_SIZE??????? (1024 * 4)
>> ? ? enum {
>> ????? VSOCK_VQ_RX???? = 0, /* for host to guest data */
>> diff --git a/net/vmw_vsock/virtio_transport.c 
>> b/net/vmw_vsock/virtio_transport.c
>> index af1d2ce12f54..732398b4e28f 100644
>> --- a/net/vmw_vsock/virtio_transport.c
>> +++ b/net/vmw_vsock/virtio_transport.c
>> @@ -66,6 +66,31 @@ struct virtio_vsock {
>> ????? u32 guest_cid;
>> ? };
>> ? +static unsigned int rx_buf_size = VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE;
>> +
>> +static int param_set_rx_buf_size(const char *val, const struct 
>> kernel_param *kp)
>> +{
>> +??? unsigned int size;
>> +??? int ret;
>> +
>> +??? ret = kstrtouint(val, 0, &size);
>> +??? if (ret)
>> +??????? return ret;
>> +
>> +??? if (size < VIRTIO_VSOCK_MIN_PKT_BUF_SIZE ||
>> +??????? size > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE)
>> +??????? return -EINVAL;
>> +
>> +??? return param_set_uint(val, kp);
>> +};
>> +
>> +static const struct kernel_param_ops param_ops_rx_buf_size = {
>> +??? .set = param_set_rx_buf_size,
>> +??? .get = param_get_uint,
>> +};
>> +
>> +module_param_cb(rx_buf_size, &param_ops_rx_buf_size,
&rx_buf_size,
>> 0644);
>> +
>> ? static struct virtio_vsock *virtio_vsock_get(void)
>> ? {
>> ????? return the_virtio_vsock;
>> @@ -261,7 +286,7 @@ virtio_transport_cancel_pkt(struct vsock_sock *vsk)
>> ? ? static void virtio_vsock_rx_fill(struct virtio_vsock *vsock)
>> ? {
>> -??? int buf_len = VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE;
>> +??? int buf_len = rx_buf_size;
>> ????? struct virtio_vsock_pkt *pkt;
>> ????? struct scatterlist hdr, buf, *sgs[2];
>> ????? struct virtqueue *vq;

On Mon, May 13, 2019 at 08:46:19PM +0800, Jason Wang
wrote:
> 
> On 2019/5/13 ??6:05, Jason Wang wrote:
> > 
> > On 2019/5/10 ??8:58, Stefano Garzarella wrote:
> > > The RX buffer size determines the memory consumption of the
> > > vsock/virtio guest driver, so we make it tunable through
> > > a module parameter.
> > > 
> > > The size allowed are between 4 KB and 64 KB in order to be
> > > compatible with old host drivers.
> > > 
> > > Suggested-by: Stefan Hajnoczi <stefanha at redhat.com>
> > > Signed-off-by: Stefano Garzarella <sgarzare at redhat.com>
> > 
> > 
> > I don't see much value of doing this through kernel command line.
We
> > should deal with them automatically like what virtio-net did. Or even
a
> > module parameter is better.
> > 
> > Thanks
> 
> 
> Sorry, I misread the patch. But even module parameter is something not
> flexible enough. We should deal with them transparently.
> 
Okay, I'll try to understand how we can automatically adapt the RX
buffer size. Since the flow is stream based, the receiver doesn't know the
original packet size.

Maybe I can reuse the EWMA approach to understand if the buffers are
entirely filled or not.
In that case I can increase (e.g. double) or decrease the size.

I'll try to do it!

Thanks,
Stefano