* [PATCH 0/3] vhost: logging fixes
@ 2010-02-23 16:57 Michael S. Tsirkin
2010-02-23 16:57 ` [PATCH 1/3] vhost: logging math fix Michael S. Tsirkin
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw)
To: Rusty Russell, kvm, virtualization, netdev, linux-kernel,
David Miller, gl
The following patches on top of net-next fix issues related to write
logging in vhost. This fixes all known to me logging issues, migration
now works for me while under stress in both TX and RX directions.
Rusty's going on vacation, I am guessing he won't have time to review
this: Gleb, Juan, Herbert, could one of you review this patchset please?
There's also the send queue full issue reported by
Sridhar Samudrala which I'm testing various fixes for,
that patch is contained to vhost/net though,
so there's no conflict, patch will be posted separately.
Michael S. Tsirkin (3):
vhost: logging thinko fix
vhost: initialize log eventfd context pointer
vhost: fix get_user_pages_fast error handling
drivers/vhost/vhost.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 1/3] vhost: logging math fix 2010-02-23 16:57 [PATCH 0/3] vhost: logging fixes Michael S. Tsirkin @ 2010-02-23 16:57 ` Michael S. Tsirkin 2010-02-23 19:26 ` Juan Quintela 2010-02-23 16:57 ` [PATCH 2/3] vhost: initialize log eventfd context pointer Michael S. Tsirkin 2010-02-23 16:57 ` [PATCH 3/3] vhost: fix get_user_pages_fast error handling Michael S. Tsirkin 2 siblings, 1 reply; 17+ messages in thread From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw) To: Rusty Russell, kvm, virtualization, netdev, linux-kernel, David Miller, ma vhost was dong some complex math to get offset to log at, and got it wrong by a couple of bytes, while in fact it's simple: get address where we write, subtract start of buffer, add log base. Do it this way. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- drivers/vhost/vhost.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 6eb1525..c767279 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -1004,10 +1004,12 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len) if (unlikely(vq->log_used)) { /* Make sure data is seen before log. */ smp_wmb(); - log_write(vq->log_base, vq->log_addr + sizeof *vq->used->ring * - (vq->last_used_idx % vq->num), - sizeof *vq->used->ring); - log_write(vq->log_base, vq->log_addr, sizeof *vq->used->ring); + log_write(vq->log_base, + vq->log_addr + ((void *)used - (void *)vq->used), + sizeof *used); + log_write(vq->log_base, + vq->log_addr + offsetof(struct vring_used, idx), + sizeof vq->used->idx); if (vq->log_ctx) eventfd_signal(vq->log_ctx, 1); } -- 1.7.0.18.g0d53a5 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] vhost: logging math fix 2010-02-23 16:57 ` [PATCH 1/3] vhost: logging math fix Michael S. Tsirkin @ 2010-02-23 19:26 ` Juan Quintela 0 siblings, 0 replies; 17+ messages in thread From: Juan Quintela @ 2010-02-23 19:26 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel, David Miller, markmc, gleb, herbert.xu, dlaor, avi "Michael S. Tsirkin" <mst@redhat.com> wrote: > vhost was dong some complex math to get > offset to log at, and got it wrong by a couple of bytes, > while in fact it's simple: get address where we write, > subtract start of buffer, add log base. > > Do it this way. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> > --- > drivers/vhost/vhost.c | 10 ++++++---- > 1 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index 6eb1525..c767279 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -1004,10 +1004,12 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len) > if (unlikely(vq->log_used)) { > /* Make sure data is seen before log. */ We explain what smp_wmb() does. > smp_wmb(); > - log_write(vq->log_base, vq->log_addr + sizeof *vq->used->ring * > - (vq->last_used_idx % vq->num), > - sizeof *vq->used->ring); > - log_write(vq->log_base, vq->log_addr, sizeof *vq->used->ring); > + log_write(vq->log_base, > + vq->log_addr + ((void *)used - (void *)vq->used), > + sizeof *used); > + log_write(vq->log_base, > + vq->log_addr + offsetof(struct vring_used, idx), > + sizeof vq->used->idx); Once here, can we add a comment explaining _what_ are we trying to write to the log? michael explains that t is the used element and the index, but nothing states that. > if (vq->log_ctx) > eventfd_signal(vq->log_ctx, 1); > } ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] vhost: initialize log eventfd context pointer 2010-02-23 16:57 [PATCH 0/3] vhost: logging fixes Michael S. Tsirkin 2010-02-23 16:57 ` [PATCH 1/3] vhost: logging math fix Michael S. Tsirkin @ 2010-02-23 16:57 ` Michael S. Tsirkin 2010-02-23 19:31 ` Juan Quintela 2010-02-23 16:57 ` [PATCH 3/3] vhost: fix get_user_pages_fast error handling Michael S. Tsirkin 2 siblings, 1 reply; 17+ messages in thread From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw) To: Rusty Russell, kvm, virtualization, netdev, linux-kernel, David Miller, ma vq log eventfd context pointer needs to be initialized, otherwise operation may fail or oops if log is enabled but log eventfd not set by userspace. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- drivers/vhost/vhost.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index c767279..d4f8fdf 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -121,6 +121,7 @@ static void vhost_vq_reset(struct vhost_dev *dev, vq->kick = NULL; vq->call_ctx = NULL; vq->call = NULL; + vq->log_ctx = NULL; } long vhost_dev_init(struct vhost_dev *dev, -- 1.7.0.18.g0d53a5 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] vhost: initialize log eventfd context pointer 2010-02-23 16:57 ` [PATCH 2/3] vhost: initialize log eventfd context pointer Michael S. Tsirkin @ 2010-02-23 19:31 ` Juan Quintela 0 siblings, 0 replies; 17+ messages in thread From: Juan Quintela @ 2010-02-23 19:31 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel, David Miller, markmc, gleb, herbert.xu, dlaor, avi "Michael S. Tsirkin" <mst@redhat.com> wrote: > vq log eventfd context pointer needs to be initialized, otherwise > operation may fail or oops if log is enabled but log eventfd not set by > userspace. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> When log_ctx for device is created, it is copied to the vq. This reset was missing. > --- > drivers/vhost/vhost.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index c767279..d4f8fdf 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -121,6 +121,7 @@ static void vhost_vq_reset(struct vhost_dev *dev, > vq->kick = NULL; > vq->call_ctx = NULL; > vq->call = NULL; > + vq->log_ctx = NULL; > } > > long vhost_dev_init(struct vhost_dev *dev, ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-23 16:57 [PATCH 0/3] vhost: logging fixes Michael S. Tsirkin 2010-02-23 16:57 ` [PATCH 1/3] vhost: logging math fix Michael S. Tsirkin 2010-02-23 16:57 ` [PATCH 2/3] vhost: initialize log eventfd context pointer Michael S. Tsirkin @ 2010-02-23 16:57 ` Michael S. Tsirkin 2010-02-23 17:34 ` Gleb Natapov 2010-02-23 19:56 ` Juan Quintela 2 siblings, 2 replies; 17+ messages in thread From: Michael S. Tsirkin @ 2010-02-23 16:57 UTC (permalink / raw) To: Rusty Russell, kvm, virtualization, netdev, linux-kernel, David Miller, ma get_user_pages_fast returns number of pages on success, negative value on failure, but never 0. Fix vhost code to match this logic. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- drivers/vhost/vhost.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index d4f8fdf..d003504 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr) int bit = nr + (log % PAGE_SIZE) * 8; int r; r = get_user_pages_fast(log, 1, 1, &page); - if (r) + if (r < 0) return r; + BUG_ON(r != 1); base = kmap_atomic(page, KM_USER0); set_bit(bit, base); kunmap_atomic(base, KM_USER0); -- 1.7.0.18.g0d53a5 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-23 16:57 ` [PATCH 3/3] vhost: fix get_user_pages_fast error handling Michael S. Tsirkin @ 2010-02-23 17:34 ` Gleb Natapov 2010-02-23 17:32 ` Michael S. Tsirkin 2010-02-23 19:56 ` Juan Quintela 1 sibling, 1 reply; 17+ messages in thread From: Gleb Natapov @ 2010-02-23 17:34 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel, David Miller, markmc, herbert.xu, quintela, dlaor, avi On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote: > get_user_pages_fast returns number of pages on success, negative value > on failure, but never 0. Fix vhost code to match this logic. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > drivers/vhost/vhost.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index d4f8fdf..d003504 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr) > int bit = nr + (log % PAGE_SIZE) * 8; > int r; > r = get_user_pages_fast(log, 1, 1, &page); > - if (r) > + if (r < 0) > return r; > + BUG_ON(r != 1); Can't this be easily triggered from user space? > base = kmap_atomic(page, KM_USER0); > set_bit(bit, base); > kunmap_atomic(base, KM_USER0); > -- > 1.7.0.18.g0d53a5 -- Gleb. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-23 17:34 ` Gleb Natapov @ 2010-02-23 17:32 ` Michael S. Tsirkin 2010-02-23 17:39 ` Gleb Natapov 0 siblings, 1 reply; 17+ messages in thread From: Michael S. Tsirkin @ 2010-02-23 17:32 UTC (permalink / raw) To: Gleb Natapov Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel, David Miller, markmc, herbert.xu, quintela, dlaor, avi On Tue, Feb 23, 2010 at 07:34:34PM +0200, Gleb Natapov wrote: > On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote: > > get_user_pages_fast returns number of pages on success, negative value > > on failure, but never 0. Fix vhost code to match this logic. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > drivers/vhost/vhost.c | 3 ++- > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > index d4f8fdf..d003504 100644 > > --- a/drivers/vhost/vhost.c > > +++ b/drivers/vhost/vhost.c > > @@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr) > > int bit = nr + (log % PAGE_SIZE) * 8; > > int r; > > r = get_user_pages_fast(log, 1, 1, &page); > > - if (r) > > + if (r < 0) > > return r; > > + BUG_ON(r != 1); > Can't this be easily triggered from user space? I think no. get_user_pages_fast always returns number of pages pinned (in this case always 1) or an error (< 0). Anything else is a kernel bug. > > base = kmap_atomic(page, KM_USER0); > > set_bit(bit, base); > > kunmap_atomic(base, KM_USER0); > > -- > > 1.7.0.18.g0d53a5 > > -- > Gleb. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-23 17:32 ` Michael S. Tsirkin @ 2010-02-23 17:39 ` Gleb Natapov 2010-02-23 17:39 ` Michael S. Tsirkin 2010-02-23 22:42 ` David Miller 0 siblings, 2 replies; 17+ messages in thread From: Gleb Natapov @ 2010-02-23 17:39 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel, David Miller, markmc, herbert.xu, quintela, dlaor, avi On Tue, Feb 23, 2010 at 07:32:58PM +0200, Michael S. Tsirkin wrote: > On Tue, Feb 23, 2010 at 07:34:34PM +0200, Gleb Natapov wrote: > > On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote: > > > get_user_pages_fast returns number of pages on success, negative value > > > on failure, but never 0. Fix vhost code to match this logic. > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > --- > > > drivers/vhost/vhost.c | 3 ++- > > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > index d4f8fdf..d003504 100644 > > > --- a/drivers/vhost/vhost.c > > > +++ b/drivers/vhost/vhost.c > > > @@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr) > > > int bit = nr + (log % PAGE_SIZE) * 8; > > > int r; > > > r = get_user_pages_fast(log, 1, 1, &page); > > > - if (r) > > > + if (r < 0) > > > return r; > > > + BUG_ON(r != 1); > > Can't this be easily triggered from user space? > > I think no. get_user_pages_fast always returns number of pages > pinned (in this case always 1) or an error (< 0). > Anything else is a kernel bug. > But what if page is unmapped from userspace? > > > base = kmap_atomic(page, KM_USER0); > > > set_bit(bit, base); > > > kunmap_atomic(base, KM_USER0); > > > -- > > > 1.7.0.18.g0d53a5 > > > > -- > > Gleb. -- Gleb. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-23 17:39 ` Gleb Natapov @ 2010-02-23 17:39 ` Michael S. Tsirkin 2010-02-23 17:43 ` Gleb Natapov 2010-02-23 22:42 ` David Miller 1 sibling, 1 reply; 17+ messages in thread From: Michael S. Tsirkin @ 2010-02-23 17:39 UTC (permalink / raw) To: Gleb Natapov Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel, David Miller, markmc, herbert.xu, quintela, dlaor, avi On Tue, Feb 23, 2010 at 07:39:52PM +0200, Gleb Natapov wrote: > On Tue, Feb 23, 2010 at 07:32:58PM +0200, Michael S. Tsirkin wrote: > > On Tue, Feb 23, 2010 at 07:34:34PM +0200, Gleb Natapov wrote: > > > On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote: > > > > get_user_pages_fast returns number of pages on success, negative value > > > > on failure, but never 0. Fix vhost code to match this logic. > > > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > > > drivers/vhost/vhost.c | 3 ++- > > > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > index d4f8fdf..d003504 100644 > > > > --- a/drivers/vhost/vhost.c > > > > +++ b/drivers/vhost/vhost.c > > > > @@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr) > > > > int bit = nr + (log % PAGE_SIZE) * 8; > > > > int r; > > > > r = get_user_pages_fast(log, 1, 1, &page); > > > > - if (r) > > > > + if (r < 0) > > > > return r; > > > > + BUG_ON(r != 1); > > > Can't this be easily triggered from user space? > > > > I think no. get_user_pages_fast always returns number of pages > > pinned (in this case always 1) or an error (< 0). > > Anything else is a kernel bug. > > > But what if page is unmapped from userspace? Then we get -EFAULT > > > > base = kmap_atomic(page, KM_USER0); > > > > set_bit(bit, base); > > > > kunmap_atomic(base, KM_USER0); > > > > -- > > > > 1.7.0.18.g0d53a5 > > > > > > -- > > > Gleb. > > -- > Gleb. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-23 17:39 ` Michael S. Tsirkin @ 2010-02-23 17:43 ` Gleb Natapov 0 siblings, 0 replies; 17+ messages in thread From: Gleb Natapov @ 2010-02-23 17:43 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel, David Miller, markmc, herbert.xu, quintela, dlaor, avi On Tue, Feb 23, 2010 at 07:39:08PM +0200, Michael S. Tsirkin wrote: > On Tue, Feb 23, 2010 at 07:39:52PM +0200, Gleb Natapov wrote: > > On Tue, Feb 23, 2010 at 07:32:58PM +0200, Michael S. Tsirkin wrote: > > > On Tue, Feb 23, 2010 at 07:34:34PM +0200, Gleb Natapov wrote: > > > > On Tue, Feb 23, 2010 at 06:57:58PM +0200, Michael S. Tsirkin wrote: > > > > > get_user_pages_fast returns number of pages on success, negative value > > > > > on failure, but never 0. Fix vhost code to match this logic. > > > > > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > --- > > > > > drivers/vhost/vhost.c | 3 ++- > > > > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > > > index d4f8fdf..d003504 100644 > > > > > --- a/drivers/vhost/vhost.c > > > > > +++ b/drivers/vhost/vhost.c > > > > > @@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr) > > > > > int bit = nr + (log % PAGE_SIZE) * 8; > > > > > int r; > > > > > r = get_user_pages_fast(log, 1, 1, &page); > > > > > - if (r) > > > > > + if (r < 0) > > > > > return r; > > > > > + BUG_ON(r != 1); > > > > Can't this be easily triggered from user space? > > > > > > I think no. get_user_pages_fast always returns number of pages > > > pinned (in this case always 1) or an error (< 0). > > > Anything else is a kernel bug. > > > > > But what if page is unmapped from userspace? > > Then we get -EFAULT > Ah correct. > > > > > base = kmap_atomic(page, KM_USER0); > > > > > set_bit(bit, base); > > > > > kunmap_atomic(base, KM_USER0); > > > > > -- > > > > > 1.7.0.18.g0d53a5 > > > > > > > > -- > > > > Gleb. > > > > -- > > Gleb. -- Gleb. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-23 17:39 ` Gleb Natapov 2010-02-23 17:39 ` Michael S. Tsirkin @ 2010-02-23 22:42 ` David Miller 2010-02-24 5:37 ` Michael S. Tsirkin 1 sibling, 1 reply; 17+ messages in thread From: David Miller @ 2010-02-23 22:42 UTC (permalink / raw) To: gleb Cc: mst, rusty, kvm, virtualization, netdev, linux-kernel, markmc, herbert.xu, quintela, dlaor, avi Just for the record I'm generally not interested in vhost patches. If it's a specific network one that will be merged via the networking tree, yes please CC: me. But if it's a bunch of changes to vhost.c and other pieces of infrastructure, feel free to leave me out of it. It just clutters my already overflowing inbox. Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-23 22:42 ` David Miller @ 2010-02-24 5:37 ` Michael S. Tsirkin 2010-02-24 7:04 ` David Miller 0 siblings, 1 reply; 17+ messages in thread From: Michael S. Tsirkin @ 2010-02-24 5:37 UTC (permalink / raw) To: David Miller Cc: gleb, rusty, kvm, virtualization, netdev, linux-kernel, markmc, herbert.xu, quintela, dlaor, avi On Tue, Feb 23, 2010 at 02:42:35PM -0800, David Miller wrote: > > Just for the record I'm generally not interested in vhost > patches. > > If it's a specific network one that will be merged via > the networking tree, yes please CC: me. > > But if it's a bunch of changes to vhost.c and other pieces > of infrastructure, feel free to leave me out of it. It just > clutters my already overflowing inbox. > > Thanks. Dave, so while Rusty's on vacation, what's the best way to get vhost infrastructure fixes in? Are you ok with getting pull requests and merging them into net-next? That should keep the clutter in your inbox to the minimum. Of course network changes would still go the usual way. -- MST ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-24 5:37 ` Michael S. Tsirkin @ 2010-02-24 7:04 ` David Miller 2010-02-24 7:34 ` Michael S. Tsirkin 0 siblings, 1 reply; 17+ messages in thread From: David Miller @ 2010-02-24 7:04 UTC (permalink / raw) To: mst Cc: gleb, rusty, kvm, virtualization, netdev, linux-kernel, markmc, herbert.xu, quintela, dlaor, avi From: "Michael S. Tsirkin" <mst@redhat.com> Date: Wed, 24 Feb 2010 07:37:37 +0200 > Dave, so while Rusty's on vacation, what's the best way to get vhost > infrastructure fixes in? Are you ok with getting pull requests and > merging them into net-next? That should keep the clutter in your inbox > to the minimum. > > Of course network changes would still go the usual way. Well, who is providing oversight of vhost work while he's gone? Has he, implicitly or explicitly, appointed a maintainer while he's away? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-24 7:04 ` David Miller @ 2010-02-24 7:34 ` Michael S. Tsirkin 2010-02-24 7:41 ` David Miller 0 siblings, 1 reply; 17+ messages in thread From: Michael S. Tsirkin @ 2010-02-24 7:34 UTC (permalink / raw) To: David Miller Cc: markmc, kvm, quintela, netdev, linux-kernel, virtualization, avi, herbert.xu On Tue, Feb 23, 2010 at 11:04:28PM -0800, David Miller wrote: > From: "Michael S. Tsirkin" <mst@redhat.com> > Date: Wed, 24 Feb 2010 07:37:37 +0200 > > > Dave, so while Rusty's on vacation, what's the best way to get vhost > > infrastructure fixes in? Are you ok with getting pull requests and > > merging them into net-next? That should keep the clutter in your inbox > > to the minimum. > > > > Of course network changes would still go the usual way. > > Well, who is providing oversight of vhost work while he's > gone? My plan was to get peer review of the patches before merging. So far Juan Quintela and Gleb Natapov gave feedback. > Has he, implicitly or explicitly, appointed a maintainer > while he's away? Implicitly, I guess. He said "if there's an issue Michael Tsirkin is the best person to resolve it", this was wrt merging his virtio&lguest tree. He didn't mention vhost, I wrote all of vhost though, there shouldn't be an issue with that. -- MST ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-24 7:34 ` Michael S. Tsirkin @ 2010-02-24 7:41 ` David Miller 0 siblings, 0 replies; 17+ messages in thread From: David Miller @ 2010-02-24 7:41 UTC (permalink / raw) To: mst Cc: gleb, rusty, kvm, virtualization, netdev, linux-kernel, markmc, herbert.xu, quintela, dlaor, avi From: "Michael S. Tsirkin" <mst@redhat.com> Date: Wed, 24 Feb 2010 09:34:25 +0200 > Implicitly, I guess. He said "if there's an issue Michael Tsirkin is the > best person to resolve it", this was wrt merging his virtio&lguest tree. > He didn't mention vhost, I wrote all of vhost though, there shouldn't be > an issue with that. That's good enough for me. Feel free to setup a tree for me to pull from. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling 2010-02-23 16:57 ` [PATCH 3/3] vhost: fix get_user_pages_fast error handling Michael S. Tsirkin 2010-02-23 17:34 ` Gleb Natapov @ 2010-02-23 19:56 ` Juan Quintela 1 sibling, 0 replies; 17+ messages in thread From: Juan Quintela @ 2010-02-23 19:56 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Rusty Russell, kvm, virtualization, netdev, linux-kernel, David Miller, markmc, gleb, herbert.xu, dlaor, avi "Michael S. Tsirkin" <mst@redhat.com> wrote: > get_user_pages_fast returns number of pages on success, negative value > on failure, but never 0. Fix vhost code to match this logic. It can return 0 if you ask for 0 pages :) >From the comment: * Returns number of pages pinned. This may be fewer than the number * requested. If nr_pages is 0 or negative, returns 0. If no pages * were pinned, returns -errno. */ I agree that code was wrong, but the BUG_ON() is not neccessary IMHO. The important bit is the change in the comparison. Reviewed-by: Juan Quintela <quintela@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > drivers/vhost/vhost.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index d4f8fdf..d003504 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -646,8 +646,9 @@ static int set_bit_to_user(int nr, void __user *addr) > int bit = nr + (log % PAGE_SIZE) * 8; > int r; > r = get_user_pages_fast(log, 1, 1, &page); > - if (r) > + if (r < 0) > return r; > + BUG_ON(r != 1); > base = kmap_atomic(page, KM_USER0); > set_bit(bit, base); > kunmap_atomic(base, KM_USER0); ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2010-02-24 7:41 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-02-23 16:57 [PATCH 0/3] vhost: logging fixes Michael S. Tsirkin 2010-02-23 16:57 ` [PATCH 1/3] vhost: logging math fix Michael S. Tsirkin 2010-02-23 19:26 ` Juan Quintela 2010-02-23 16:57 ` [PATCH 2/3] vhost: initialize log eventfd context pointer Michael S. Tsirkin 2010-02-23 19:31 ` Juan Quintela 2010-02-23 16:57 ` [PATCH 3/3] vhost: fix get_user_pages_fast error handling Michael S. Tsirkin 2010-02-23 17:34 ` Gleb Natapov 2010-02-23 17:32 ` Michael S. Tsirkin 2010-02-23 17:39 ` Gleb Natapov 2010-02-23 17:39 ` Michael S. Tsirkin 2010-02-23 17:43 ` Gleb Natapov 2010-02-23 22:42 ` David Miller 2010-02-24 5:37 ` Michael S. Tsirkin 2010-02-24 7:04 ` David Miller 2010-02-24 7:34 ` Michael S. Tsirkin 2010-02-24 7:41 ` David Miller 2010-02-23 19:56 ` Juan Quintela
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).