* [PATCH v1] vhost: avoid large order allocations @ 2014-05-13 8:35 Michael Mueller 2014-05-13 8:35 ` Michael Mueller 0 siblings, 1 reply; 9+ messages in thread From: Michael Mueller @ 2014-05-13 8:35 UTC (permalink / raw) To: mst, kvm, virtualization, netdev, linux-kernel Cc: borntraeger, cornelia.huck, mimu, ddongch A test case which generates memory pressure while performing guest administration fails with vhost triggering "page allocation failure" and guest not starting up. After some analysis we discovered the allocation order of vhost to be rensponsible for this behaviour. Thus we suggest patch 1/1 which dynamically allocates the required memory. Please see its description for details. Thanks, Michael Dong Dong Chen (1): vhost: avoid large order allocations drivers/vhost/net.c | 4 ++-- drivers/vhost/scsi.c | 4 ++-- drivers/vhost/test.c | 2 +- drivers/vhost/vhost.c | 6 +++++- drivers/vhost/vhost.h | 2 +- 5 files changed, 11 insertions(+), 7 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1] vhost: avoid large order allocations 2014-05-13 8:35 [PATCH v1] vhost: avoid large order allocations Michael Mueller @ 2014-05-13 8:35 ` Michael Mueller 2014-05-13 8:40 ` Michael S. Tsirkin 0 siblings, 1 reply; 9+ messages in thread From: Michael Mueller @ 2014-05-13 8:35 UTC (permalink / raw) To: mst, kvm, virtualization, netdev, linux-kernel Cc: borntraeger, cornelia.huck, mimu, ddongch From: Dong Dong Chen <ddongch@cn.ibm.com> Under memory pressure we observe load issues with module vhost_net, as the vhost setup code will try to do an order 4 allocation for the device. The likeliness of this issue to occur can be reduced when the statically allocated variable "iov" in "struct vhost_virtqueue" is dynamically allocated with exactly the required size, reducing this to an order 2 allocation. Signed-off-by: Dong Dong Chen <ddongch@cn.ibm.com> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> Reviewed-by: Michael Mueller <mimu@linux.vnet.ibm.com> --- drivers/vhost/net.c | 4 ++-- drivers/vhost/scsi.c | 4 ++-- drivers/vhost/test.c | 2 +- drivers/vhost/vhost.c | 6 +++++- drivers/vhost/vhost.h | 2 +- 5 files changed, 11 insertions(+), 7 deletions(-) diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index be414d2..e3a9a68 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -374,7 +374,7 @@ static void handle_tx(struct vhost_net *net) break; head = vhost_get_vq_desc(&net->dev, vq, vq->iov, - ARRAY_SIZE(vq->iov), + UIO_MAXIOV, &out, &in, NULL, NULL); /* On error, stop handling until the next kick. */ @@ -506,7 +506,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq, goto err; } r = vhost_get_vq_desc(vq->dev, vq, vq->iov + seg, - ARRAY_SIZE(vq->iov) - seg, &out, + UIO_MAXIOV - seg, &out, &in, log, log_num); if (unlikely(r < 0)) goto err; diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c index cf50ce9..a70f1d9 100644 --- a/drivers/vhost/scsi.c +++ b/drivers/vhost/scsi.c @@ -607,7 +607,7 @@ tcm_vhost_do_evt_work(struct vhost_scsi *vs, struct tcm_vhost_evt *evt) again: vhost_disable_notify(&vs->dev, vq); head = vhost_get_vq_desc(&vs->dev, vq, vq->iov, - ARRAY_SIZE(vq->iov), &out, &in, + UIO_MAXIOV, &out, &in, NULL, NULL); if (head < 0) { vs->vs_events_missed = true; @@ -946,7 +946,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq) for (;;) { head = vhost_get_vq_desc(&vs->dev, vq, vq->iov, - ARRAY_SIZE(vq->iov), &out, &in, + UIO_MAXIOV, &out, &in, NULL, NULL); pr_debug("vhost_get_vq_desc: head: %d, out: %u in: %u\n", head, out, in); diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c index c2a54fb..2e01920 100644 --- a/drivers/vhost/test.c +++ b/drivers/vhost/test.c @@ -54,7 +54,7 @@ static void handle_vq(struct vhost_test *n) for (;;) { head = vhost_get_vq_desc(&n->dev, vq, vq->iov, - ARRAY_SIZE(vq->iov), + UIO_MAXIOV, &out, &in, NULL, NULL); /* On error, stop handling until the next kick. */ diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 78987e4..9017a55 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -251,6 +251,8 @@ static int vhost_worker(void *data) static void vhost_vq_free_iovecs(struct vhost_virtqueue *vq) { + kfree(vq->iov); + vq->iov = NULL; kfree(vq->indirect); vq->indirect = NULL; kfree(vq->log); @@ -267,11 +269,12 @@ static long vhost_dev_alloc_iovecs(struct vhost_dev *dev) for (i = 0; i < dev->nvqs; ++i) { vq = dev->vqs[i]; + vq->iov = kmalloc(sizeof(*vq->iov) * UIO_MAXIOV, GFP_KERNEL); vq->indirect = kmalloc(sizeof *vq->indirect * UIO_MAXIOV, GFP_KERNEL); vq->log = kmalloc(sizeof *vq->log * UIO_MAXIOV, GFP_KERNEL); vq->heads = kmalloc(sizeof *vq->heads * UIO_MAXIOV, GFP_KERNEL); - if (!vq->indirect || !vq->log || !vq->heads) + if (!vq->iov || !vq->indirect || !vq->log || !vq->heads) goto err_nomem; } return 0; @@ -310,6 +313,7 @@ void vhost_dev_init(struct vhost_dev *dev, for (i = 0; i < dev->nvqs; ++i) { vq = dev->vqs[i]; vq->log = NULL; + vq->iov = NULL; vq->indirect = NULL; vq->heads = NULL; vq->dev = dev; diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index 35eeb2a..541f757 100644 --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -100,7 +100,7 @@ struct vhost_virtqueue { bool log_used; u64 log_addr; - struct iovec iov[UIO_MAXIOV]; + struct iovec *iov; struct iovec *indirect; struct vring_used_elem *heads; /* Protected by virtqueue mutex. */ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1] vhost: avoid large order allocations 2014-05-13 8:35 ` Michael Mueller @ 2014-05-13 8:40 ` Michael S. Tsirkin 2014-05-13 8:57 ` Michael Mueller 2014-05-13 14:29 ` Romain Francoise 0 siblings, 2 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2014-05-13 8:40 UTC (permalink / raw) To: Michael Mueller Cc: kvm, virtualization, netdev, linux-kernel, borntraeger, cornelia.huck, ddongch On Tue, May 13, 2014 at 10:35:33AM +0200, Michael Mueller wrote: > From: Dong Dong Chen <ddongch@cn.ibm.com> > > Under memory pressure we observe load issues with module vhost_net, as the > vhost setup code will try to do an order 4 allocation for the device. > The likeliness of this issue to occur can be reduced when the statically > allocated variable "iov" in "struct vhost_virtqueue" is dynamically allocated > with exactly the required size, reducing this to an order 2 allocation. > > Signed-off-by: Dong Dong Chen <ddongch@cn.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> > Reviewed-by: Michael Mueller <mimu@linux.vnet.ibm.com> Please dont' do this, extra indirection hurts performance. Instead, please change vhost_net_open and scsi to allocate the whole structure with vmalloc if kmalloc fails, along the lines of 74d332c13b2148ae934ea94dac1745ae92efe8e5 > --- > drivers/vhost/net.c | 4 ++-- > drivers/vhost/scsi.c | 4 ++-- > drivers/vhost/test.c | 2 +- > drivers/vhost/vhost.c | 6 +++++- > drivers/vhost/vhost.h | 2 +- > 5 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c > index be414d2..e3a9a68 100644 > --- a/drivers/vhost/net.c > +++ b/drivers/vhost/net.c > @@ -374,7 +374,7 @@ static void handle_tx(struct vhost_net *net) > break; > > head = vhost_get_vq_desc(&net->dev, vq, vq->iov, > - ARRAY_SIZE(vq->iov), > + UIO_MAXIOV, > &out, &in, > NULL, NULL); > /* On error, stop handling until the next kick. */ > @@ -506,7 +506,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq, > goto err; > } > r = vhost_get_vq_desc(vq->dev, vq, vq->iov + seg, > - ARRAY_SIZE(vq->iov) - seg, &out, > + UIO_MAXIOV - seg, &out, > &in, log, log_num); > if (unlikely(r < 0)) > goto err; > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c > index cf50ce9..a70f1d9 100644 > --- a/drivers/vhost/scsi.c > +++ b/drivers/vhost/scsi.c > @@ -607,7 +607,7 @@ tcm_vhost_do_evt_work(struct vhost_scsi *vs, struct tcm_vhost_evt *evt) > again: > vhost_disable_notify(&vs->dev, vq); > head = vhost_get_vq_desc(&vs->dev, vq, vq->iov, > - ARRAY_SIZE(vq->iov), &out, &in, > + UIO_MAXIOV, &out, &in, > NULL, NULL); > if (head < 0) { > vs->vs_events_missed = true; > @@ -946,7 +946,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq) > > for (;;) { > head = vhost_get_vq_desc(&vs->dev, vq, vq->iov, > - ARRAY_SIZE(vq->iov), &out, &in, > + UIO_MAXIOV, &out, &in, > NULL, NULL); > pr_debug("vhost_get_vq_desc: head: %d, out: %u in: %u\n", > head, out, in); > diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c > index c2a54fb..2e01920 100644 > --- a/drivers/vhost/test.c > +++ b/drivers/vhost/test.c > @@ -54,7 +54,7 @@ static void handle_vq(struct vhost_test *n) > > for (;;) { > head = vhost_get_vq_desc(&n->dev, vq, vq->iov, > - ARRAY_SIZE(vq->iov), > + UIO_MAXIOV, > &out, &in, > NULL, NULL); > /* On error, stop handling until the next kick. */ > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index 78987e4..9017a55 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -251,6 +251,8 @@ static int vhost_worker(void *data) > > static void vhost_vq_free_iovecs(struct vhost_virtqueue *vq) > { > + kfree(vq->iov); > + vq->iov = NULL; > kfree(vq->indirect); > vq->indirect = NULL; > kfree(vq->log); > @@ -267,11 +269,12 @@ static long vhost_dev_alloc_iovecs(struct vhost_dev *dev) > > for (i = 0; i < dev->nvqs; ++i) { > vq = dev->vqs[i]; > + vq->iov = kmalloc(sizeof(*vq->iov) * UIO_MAXIOV, GFP_KERNEL); > vq->indirect = kmalloc(sizeof *vq->indirect * UIO_MAXIOV, > GFP_KERNEL); > vq->log = kmalloc(sizeof *vq->log * UIO_MAXIOV, GFP_KERNEL); > vq->heads = kmalloc(sizeof *vq->heads * UIO_MAXIOV, GFP_KERNEL); > - if (!vq->indirect || !vq->log || !vq->heads) > + if (!vq->iov || !vq->indirect || !vq->log || !vq->heads) > goto err_nomem; > } > return 0; > @@ -310,6 +313,7 @@ void vhost_dev_init(struct vhost_dev *dev, > for (i = 0; i < dev->nvqs; ++i) { > vq = dev->vqs[i]; > vq->log = NULL; > + vq->iov = NULL; > vq->indirect = NULL; > vq->heads = NULL; > vq->dev = dev; > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h > index 35eeb2a..541f757 100644 > --- a/drivers/vhost/vhost.h > +++ b/drivers/vhost/vhost.h > @@ -100,7 +100,7 @@ struct vhost_virtqueue { > bool log_used; > u64 log_addr; > > - struct iovec iov[UIO_MAXIOV]; > + struct iovec *iov; > struct iovec *indirect; > struct vring_used_elem *heads; > /* Protected by virtqueue mutex. */ > -- > 1.8.3.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] vhost: avoid large order allocations 2014-05-13 8:40 ` Michael S. Tsirkin @ 2014-05-13 8:57 ` Michael Mueller 2014-05-13 14:29 ` Romain Francoise 1 sibling, 0 replies; 9+ messages in thread From: Michael Mueller @ 2014-05-13 8:57 UTC (permalink / raw) To: Michael S. Tsirkin Cc: kvm, virtualization, netdev, linux-kernel, borntraeger, cornelia.huck, ddongch, mimu On Tue, 13 May 2014 11:40:49 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > Please dont' do this, extra indirection hurts performance. > Instead, please change vhost_net_open and scsi to allocate the whole > structure with vmalloc if kmalloc fails, along the lines of > 74d332c13b2148ae934ea94dac1745ae92efe8e5 Thanks for pointing us to: net: extend net_device allocation to vmalloc() We'll try to adapt. Michael ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] vhost: avoid large order allocations 2014-05-13 8:40 ` Michael S. Tsirkin 2014-05-13 8:57 ` Michael Mueller @ 2014-05-13 14:29 ` Romain Francoise 2014-05-13 15:07 ` Michael Mueller 2014-05-13 15:15 ` Michael S. Tsirkin 1 sibling, 2 replies; 9+ messages in thread From: Romain Francoise @ 2014-05-13 14:29 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Michael Mueller, kvm, virtualization, netdev, linux-kernel, borntraeger, cornelia.huck, ddongch "Michael S. Tsirkin" <mst@redhat.com> writes: > Please dont' do this, extra indirection hurts performance. > Instead, please change vhost_net_open and scsi to allocate the whole > structure with vmalloc if kmalloc fails, along the lines of > 74d332c13b2148ae934ea94dac1745ae92efe8e5 Back in January 2013, you didn't seem to think it was a good idea: https://lkml.org/lkml/2013/1/23/492 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] vhost: avoid large order allocations 2014-05-13 14:29 ` Romain Francoise @ 2014-05-13 15:07 ` Michael Mueller 2014-05-13 15:15 ` Michael S. Tsirkin 1 sibling, 0 replies; 9+ messages in thread From: Michael Mueller @ 2014-05-13 15:07 UTC (permalink / raw) To: Romain Francoise Cc: Michael S. Tsirkin, kvm, virtualization, netdev, linux-kernel, borntraeger, cornelia.huck, ddongch On Tue, 13 May 2014 16:29:58 +0200 Romain Francoise <romain@orebokech.com> wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > Please dont' do this, extra indirection hurts performance. > > Instead, please change vhost_net_open and scsi to allocate the whole > > structure with vmalloc if kmalloc fails, along the lines of > > 74d332c13b2148ae934ea94dac1745ae92efe8e5 > > Back in January 2013, you didn't seem to think it was a good idea: > > https://lkml.org/lkml/2013/1/23/492 > Hi Romain, in that case I'd suggest that you submit your patch, ours will look pretty much the same! Cheers Michael ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] vhost: avoid large order allocations 2014-05-13 14:29 ` Romain Francoise 2014-05-13 15:07 ` Michael Mueller @ 2014-05-13 15:15 ` Michael S. Tsirkin 2014-05-14 8:11 ` Michael Mueller 2014-06-13 11:56 ` Michael Mueller 1 sibling, 2 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2014-05-13 15:15 UTC (permalink / raw) To: Romain Francoise Cc: Michael Mueller, kvm, virtualization, netdev, linux-kernel, borntraeger, cornelia.huck, ddongch On Tue, May 13, 2014 at 04:29:58PM +0200, Romain Francoise wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > Please dont' do this, extra indirection hurts performance. > > Instead, please change vhost_net_open and scsi to allocate the whole > > structure with vmalloc if kmalloc fails, along the lines of > > 74d332c13b2148ae934ea94dac1745ae92efe8e5 > > Back in January 2013, you didn't seem to think it was a good idea: > > https://lkml.org/lkml/2013/1/23/492 Hmm true, and Dave thought the structure's too large. I'll have to do some benchmarks to see what the effect of Michael's patch is, performance-wise. If it's too expensive I can pick up your patch, no need to repost. -- MST ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] vhost: avoid large order allocations 2014-05-13 15:15 ` Michael S. Tsirkin @ 2014-05-14 8:11 ` Michael Mueller 2014-06-13 11:56 ` Michael Mueller 1 sibling, 0 replies; 9+ messages in thread From: Michael Mueller @ 2014-05-14 8:11 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Romain Francoise, kvm, virtualization, netdev, linux-kernel, borntraeger, cornelia.huck, ddongch On Tue, 13 May 2014 18:15:27 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, May 13, 2014 at 04:29:58PM +0200, Romain Francoise wrote: > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > > > Please dont' do this, extra indirection hurts performance. > > > Instead, please change vhost_net_open and scsi to allocate the whole > > > structure with vmalloc if kmalloc fails, along the lines of > > > 74d332c13b2148ae934ea94dac1745ae92efe8e5 > > > > Back in January 2013, you didn't seem to think it was a good idea: > > > > https://lkml.org/lkml/2013/1/23/492 > > Hmm true, and Dave thought the structure's too large. > I'll have to do some benchmarks to see what the effect > of Michael's patch is, performance-wise. > If it's too expensive I can pick up your patch, no need to > repost. > Thanks, let us know then. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] vhost: avoid large order allocations 2014-05-13 15:15 ` Michael S. Tsirkin 2014-05-14 8:11 ` Michael Mueller @ 2014-06-13 11:56 ` Michael Mueller 1 sibling, 0 replies; 9+ messages in thread From: Michael Mueller @ 2014-06-13 11:56 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Romain Francoise, kvm, virtualization, netdev, linux-kernel, borntraeger, cornelia.huck, ddongch On Tue, 13 May 2014 18:15:27 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, May 13, 2014 at 04:29:58PM +0200, Romain Francoise wrote: > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > > > Please dont' do this, extra indirection hurts performance. > > > Instead, please change vhost_net_open and scsi to allocate the whole > > > structure with vmalloc if kmalloc fails, along the lines of > > > 74d332c13b2148ae934ea94dac1745ae92efe8e5 > > > > Back in January 2013, you didn't seem to think it was a good idea: > > > > https://lkml.org/lkml/2013/1/23/492 > > Hmm true, and Dave thought the structure's too large. > I'll have to do some benchmarks to see what the effect > of Michael's patch is, performance-wise. > If it's too expensive I can pick up your patch, no need to > repost. > Hi Michael, do you have any update in this case for us? Michael ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-06-13 11:57 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-05-13 8:35 [PATCH v1] vhost: avoid large order allocations Michael Mueller 2014-05-13 8:35 ` Michael Mueller 2014-05-13 8:40 ` Michael S. Tsirkin 2014-05-13 8:57 ` Michael Mueller 2014-05-13 14:29 ` Romain Francoise 2014-05-13 15:07 ` Michael Mueller 2014-05-13 15:15 ` Michael S. Tsirkin 2014-05-14 8:11 ` Michael Mueller 2014-06-13 11:56 ` Michael Mueller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).