| Message ID | 20190510125843.95587-9-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: > 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@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@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, ¶m_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@redhat.com> >> Signed-off-by: Stefano Garzarella <sgarzare@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, ¶m_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@redhat.com> > > > Signed-off-by: Stefano Garzarella <sgarzare@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
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, ¶m_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;
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@redhat.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- include/linux/virtio_vsock.h | 1 + net/vmw_vsock/virtio_transport.c | 27 ++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-)