vhost: logging fixes
Subsystems (5)
Michael S. Tsirkin
maintainercover vhost: logging fixes
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(-)
Michael S. Tsirkin
maintainerPATCH 1/3 vhost: logging math fix
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.
--- drivers/vhost/vhost.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)
drivers/vhost/vhost.c +6 -4
@@ -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 ); }
Signature · 1 line
1.7.0.18.g0d53a5
Re: [PATCH 1/3] vhost: logging math fix
Quoted 8 lines · Michael S. Tsirkin
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 <[email protected]>
drivers/vhost/vhost.c int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len) · line 1005
Quoted 11 lines
--- drivers/vhost/vhost.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)drivers/vhost/vhost.c +0
-0@@ -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.
drivers/vhost/vhost.c int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len) · line 1012
Quoted 11 lines
drivers/vhost/vhost.c +6
-4smp_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.
Quoted 3 lines
drivers/vhost/vhost.c +0
-0if (vq->log_ctx)eventfd_signal (vq->log_ctx,1 ); }
Michael S. Tsirkin
maintainerPATCH 2/3 vhost: initialize log eventfd context pointer
In reply to Michael S. Tsirkinvq 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.
--- drivers/vhost/vhost.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/vhost/vhost.c +1 -0
@@ -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,
Signature · 1 line
1.7.0.18.g0d53a5
Re: [PATCH 2/3] vhost: initialize log eventfd context pointer
Quoted 5 lines · Michael S. Tsirkin
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 <[email protected]>
When log_ctx for device is created, it is copied to the vq. This reset was missing.
Quoted 16 lines
--- drivers/vhost/vhost.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/vhost/vhost.c +1
-0@@ -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,
Michael S. Tsirkin
maintainerPATCH 3/3 vhost: fix get_user_pages_fast error handling
In reply to Michael S. Tsirkinget_user_pages_fast returns number of pages on success, negative value on failure, but never 0. Fix vhost code to match this logic.
--- drivers/vhost/vhost.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/vhost/vhost.c +2 -1
@@ -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 );
Signature · 1 line
1.7.0.18.g0d53a5
Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
Quoted 13 lines · Michael S. Tsirkin
get_user_pages_fastreturns 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 <[email protected]>--- drivers/vhost/vhost.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)drivers/vhost/vhost.c +0
-0
drivers/vhost/vhost.c static int set_bit_to_user(int nr, void __user *addr) · line 651
Quoted 7 lines
drivers/vhost/vhost.c +2
-1@@ -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?
Quoted 5 lines
drivers/vhost/vhost.c +0
-0base =kmap_atomic (page,KM_USER0 );set_bit (bit, base);kunmap_atomic (base,KM_USER0 );Signature · 1 line
1.7.0.18.g0d53a5
--
Gleb.
Michael S. Tsirkin
maintainerRe: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Michael S. Tsirkin
get_user_pages_fastreturns 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 <[email protected]>--- drivers/vhost/vhost.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
Quoted 13 lines
drivers/vhost/vhost.c static int set_bit_to_user(int nr, void __user *addr) · line 651
Quoted 12 lines
drivers/vhost/vhost.c +2
-1@@ -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.
Quoted 8 lines
Quoted 5 lines
drivers/vhost/vhost.c +0
-0base =kmap_atomic (page,KM_USER0 );set_bit (bit, base);kunmap_atomic (base,KM_USER0 );Signature · 1 line
1.7.0.18.g0d53a5--
Gleb.
Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
Quoted 24 lines · Michael S. Tsirkin
Quoted 9 lines · Gleb Natapov
Quoted 8 lines · Michael S. Tsirkin
get_user_pages_fastreturns 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 <[email protected]>--- drivers/vhost/vhost.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)Quoted 13 lines
drivers/vhost/vhost.c static int set_bit_to_user(int nr, void __user *addr) · line 651
Quoted 12 lines
drivers/vhost/vhost.c +2
-1@@ -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?
Quoted 4 lines
I think no.
get_user_pages_fastalways 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?
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
drivers/vhost/vhost.c +0
-0base =kmap_atomic (page,KM_USER0 );set_bit (bit, base);kunmap_atomic (base,KM_USER0 );Signature · 1 line
1.7.0.18.g0d53a5--
Gleb.
--
Gleb.
Michael S. Tsirkin
maintainerRe: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
Quoted 25 lines · Gleb Natapov
Quoted 24 lines · Michael S. Tsirkin
Quoted 22 lines · Gleb Natapov
Quoted 13 lines · Michael S. Tsirkin
get_user_pages_fastreturns 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 <[email protected]>--- drivers/vhost/vhost.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)drivers/vhost/vhost.c +0
-0drivers/vhost/vhost.c static int set_bit_to_user(int nr, void __user *addr) · line 651
Quoted 7 lines
drivers/vhost/vhost.c +2
-1@@ -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?
Quoted 5 lines
Quoted 4 lines
I think no.
get_user_pages_fastalways 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
Quoted 11 lines
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
drivers/vhost/vhost.c +0
-0base =kmap_atomic (page,KM_USER0 );set_bit (bit, base);kunmap_atomic (base,KM_USER0 );Signature · 1 line
1.7.0.18.g0d53a5--
Gleb.--
Gleb.
Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
Quoted 26 lines · Michael S. Tsirkin
Quoted 25 lines · Gleb Natapov
Quoted 24 lines · Michael S. Tsirkin
Quoted 22 lines · Gleb Natapov
Quoted 13 lines · Michael S. Tsirkin
get_user_pages_fastreturns 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 <[email protected]>--- drivers/vhost/vhost.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)drivers/vhost/vhost.c +0
-0drivers/vhost/vhost.c static int set_bit_to_user(int nr, void __user *addr) · line 651
Quoted 7 lines
drivers/vhost/vhost.c +2
-1@@ -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?
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
I think no.
get_user_pages_fastalways 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.
Quoted 11 lines
Quoted 11 lines
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
drivers/vhost/vhost.c +0
-0base =kmap_atomic (page,KM_USER0 );set_bit (bit, base);kunmap_atomic (base,KM_USER0 );Signature · 1 line
1.7.0.18.g0d53a5--
Gleb.--
Gleb.
--
Gleb.
David Miller
maintainerRe: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
In reply to Gleb NatapovJust 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.
Michael S. Tsirkin
maintainerRe: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
Quoted 4 lines · David Miller
Just for the record I'm generally not interested in vhost patches.
Quoted 8 lines
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.cand 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.
Signature · 1 line
MST
David Miller
maintainerRe: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
From: "Michael S. Tsirkin" <[email protected]>
Date: Wed, 24 Feb 2010 07:37:37 +0200
Quoted 6 lines
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?
Michael S. Tsirkin
maintainerRe: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
Quoted 8 lines · David Miller
From: "Michael S. Tsirkin" <[email protected]>
Date: Wed, 24 Feb 2010 07:37:37 +0200Quoted 5 lines
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.
Quoted 4 lines
Quoted 1 line
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.
Quoted 2 lines
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.
Signature · 1 line
MST
David Miller
maintainerRe: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
From: "Michael S. Tsirkin" <[email protected]>
Date: Wed, 24 Feb 2010 09:34:25 +0200
Quoted 4 lines
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.
Re: [PATCH 3/3] vhost: fix get_user_pages_fast error handling
In reply to Michael S. TsirkinQuoted 2 lines · Michael S. Tsirkin
get_user_pages_fastreturns 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 :)
Quoted 1 line
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.
Quoted 20 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- drivers/vhost/vhost.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)drivers/vhost/vhost.c +2
-1@@ -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 );