vhost_worker fixes
Subsystems (2)
cover vhost_worker fixes
There are two patches in this series:
* A patch that reduces vhost_work_flush() latency on busy systems.
* A patch that avoids that vhost_work_flush() returns early.
I found these issues via code inspection.
PATCH 1/2 vhost: Reduce vhost_work_flush() wakeup latency
If the TIF_NEED_RESCHED task flag is set, wake up any vhost_work_flush() waiters before rescheduling instead of after rescheduling.
--- drivers/vhost/vhost.c | 42 +++++++++++++++++++----------------------- 1 file changed, 19 insertions(+), 23 deletions(-)
drivers/vhost/vhost.c +19 -23
@@ -201,47 +201,43 @@ static void vhost_vq_reset(struct vhost_dev *dev,static int vhost_worker (void *data) {struct vhost_dev *dev = data; -struct vhost_work *work =NULL ; -unsigned uninitialized_var (seq ); +struct vhost_work *work; +unsigned seq;mm_segment_t oldfs =get_fs ();set_fs (USER_DS );use_mm (dev->mm); -for (;;) { +spin_lock_irq (&dev->work_lock); +while (!kthread_should_stop ()) {/* mb paired w/ kthread_stop */ set_current_state (TASK_INTERRUPTIBLE ); - -spin_lock_irq (&dev->work_lock); -if (work) { - work->done_seq = seq; -if (work->flushing) -wake_up_all (&work->done); - } - -if (kthread_should_stop ()) { -spin_unlock_irq (&dev->work_lock); -__set_current_state (TASK_RUNNING ); -break ; - }if (!list_empty (&dev->work_list)) { work =list_first_entry (&dev->work_list, struct vhost_work, node);list_del_init (&work->node); seq = work->queue_seq; - }else - work =NULL ; -spin_unlock_irq (&dev->work_lock); +spin_unlock_irq (&dev->work_lock); -if (work) {__set_current_state (TASK_RUNNING ); work->fn (work); -if (need_resched ()) -schedule (); - }else + +spin_lock_irq (&dev->work_lock); + work->done_seq = seq; +if (work->flushing) +wake_up_all (&work->done); + } +if (list_empty (&dev->work_list) ||need_resched ()) { +spin_unlock_irq (&dev->work_lock); +schedule (); +spin_lock_irq (&dev->work_lock); + } } +spin_unlock_irq (&dev->work_lock); + +__set_current_state (TASK_RUNNING );unuse_mm (dev->mm);set_fs (oldfs);return 0 ;
Signature · 1 line
1.7.10.4
Michael S. Tsirkin
maintainerRe: [PATCH 1/2] vhost: Reduce vhost_work_flush() wakeup latency
Quoted 6 lines · Bart Van Assche
If the
TIF_NEED_RESCHEDtask flag is set, wake up anyvhost_work_flush()waiters before rescheduling instead of after rescheduling.Signed-off-by Bart Van Assche <[email protected]>Cc Michael S. Tsirkin <[email protected]>Cc Asias He <[email protected]>
Why exactly? It's not like flush needs to be extra fast ...
Quoted 77 lines
--- drivers/vhost/vhost.c | 42 +++++++++++++++++++----------------------- 1 file changed, 19 insertions(+), 23 deletions(-)drivers/vhost/vhost.c +19
-23@@ -201,47 +201,43 @@ static void vhost_vq_reset(struct vhost_dev *dev,static int vhost_worker (void *data) {struct vhost_dev *dev = data; -struct vhost_work *work =NULL ; -unsigned uninitialized_var (seq ); +struct vhost_work *work; +unsigned seq;mm_segment_t oldfs =get_fs ();set_fs (USER_DS );use_mm (dev->mm); -for (;;) { +spin_lock_irq (&dev->work_lock); +while (!kthread_should_stop ()) {/* mb paired w/ kthread_stop */ set_current_state (TASK_INTERRUPTIBLE ); - -spin_lock_irq (&dev->work_lock); -if (work) { - work->done_seq = seq; -if (work->flushing) -wake_up_all (&work->done); - } - -if (kthread_should_stop ()) { -spin_unlock_irq (&dev->work_lock); -__set_current_state (TASK_RUNNING ); -break ; - }if (!list_empty (&dev->work_list)) { work =list_first_entry (&dev->work_list, struct vhost_work, node);list_del_init (&work->node); seq = work->queue_seq; - }else - work =NULL ; -spin_unlock_irq (&dev->work_lock); +spin_unlock_irq (&dev->work_lock); -if (work) {__set_current_state (TASK_RUNNING ); work->fn (work); -if (need_resched ()) -schedule (); - }else + +spin_lock_irq (&dev->work_lock); + work->done_seq = seq; +if (work->flushing) +wake_up_all (&work->done); + } +if (list_empty (&dev->work_list) ||need_resched ()) { +spin_unlock_irq (&dev->work_lock); +schedule (); +spin_lock_irq (&dev->work_lock); + } } +spin_unlock_irq (&dev->work_lock); + +__set_current_state (TASK_RUNNING );unuse_mm (dev->mm);set_fs (oldfs);return 0 ;Signature · 1 line
1.7.10.4
Re: [PATCH 1/2] vhost: Reduce vhost_work_flush() wakeup latency
Quoted 9 lines · Michael S. Tsirkin
Quoted 6 lines · Bart Van Assche
If the
TIF_NEED_RESCHEDtask flag is set, wake up anyvhost_work_flush()waiters before rescheduling instead of after rescheduling.Signed-off-by Bart Van Assche <[email protected]>Cc Michael S. Tsirkin <[email protected]>Cc Asias He <[email protected]>Why exactly? It's not like flush needs to be extra fast ...
I'm not worried about how fast a flush is processed either. But I think this patch is a nice cleanup of the vhost_worker() function. It eliminates the uninitialized_var() construct and moves the assignment of "done_seq" below the read of "queue_seq".
Bart.
Michael S. Tsirkin
maintainerRe: [PATCH 1/2] vhost: Reduce vhost_work_flush() wakeup latency
Quoted 9 lines · Bart Van Assche
Quoted 8 lines · Michael S. Tsirkin
Quoted 6 lines · Bart Van Assche
If the
TIF_NEED_RESCHEDtask flag is set, wake up anyvhost_work_flush()waiters before rescheduling instead of after rescheduling.Signed-off-by Bart Van Assche <[email protected]>Cc Michael S. Tsirkin <[email protected]>Cc Asias He <[email protected]>
Quoted 8 lines
Quoted 1 line
Why exactly? It's not like flush needs to be extra fast ...
I'm not worried about how fast a flush is processed either. But I think this patch is a nice cleanup of the
vhost_worker()function. It eliminates theuninitialized_var()construct and moves the assignment of "done_seq" below the read of "queue_seq".Bart.
I'm not worried about uninitialized_var - it's just a compiler bug. done_seq below read is nice, but this is at the cost of sprinkling lock/unlock, lock/unlock all over the code. Maybe we can come up with something that doesn't have this property.
Signature · 1 line
MST
Re: [PATCH 1/2] vhost: Reduce vhost_work_flush() wakeup latency
Quoted 17 lines · Michael S. Tsirkin
Quoted 16 lines · Bart Van Assche
Quoted 9 lines · Michael S. Tsirkin
Quoted 6 lines · Bart Van Assche
If the
TIF_NEED_RESCHEDtask flag is set, wake up anyvhost_work_flush()waiters before rescheduling instead of after rescheduling.Signed-off-by Bart Van Assche <[email protected]>Cc Michael S. Tsirkin <[email protected]>Cc Asias He <[email protected]>Why exactly? It's not like flush needs to be extra fast ...
I'm not worried about how fast a flush is processed either. But I think this patch is a nice cleanup of the
vhost_worker()function. It eliminates theuninitialized_var()construct and moves the assignment of "done_seq" below the read of "queue_seq".
Quoted 6 lines
Quoted 1 line
Bart.
I'm not worried about
uninitialized_var- it's just a compiler bug.done_seqbelow read is nice, but this is at the cost of sprinkling lock/unlock, lock/unlock all over the code. Maybe we can come up with something that doesn't have this property.
The extra locking introduced here does not look good to me neither.
Quoted 2 lines
Signature · 1 line
MST
Signature · 1 line
Asias
Re: [PATCH 1/2] vhost: Reduce vhost_work_flush() wakeup latency
Quoted 18 lines · Asias He
Quoted 17 lines · Michael S. Tsirkin
Quoted 16 lines · Bart Van Assche
Quoted 9 lines · Michael S. Tsirkin
Quoted 6 lines · Bart Van Assche
If the
TIF_NEED_RESCHEDtask flag is set, wake up anyvhost_work_flush()waiters before rescheduling instead of after rescheduling.Signed-off-by Bart Van Assche <[email protected]>Cc Michael S. Tsirkin <[email protected]>Cc Asias He <[email protected]>Why exactly? It's not like flush needs to be extra fast ...
I'm not worried about how fast a flush is processed either. But I think this patch is a nice cleanup of the
vhost_worker()function. It eliminates theuninitialized_var()construct and moves the assignment of "done_seq" below the read of "queue_seq".
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Bart.
I'm not worried about
uninitialized_var- it's just a compiler bug.done_seqbelow read is nice, but this is at the cost of sprinkling lock/unlock, lock/unlock all over the code. Maybe we can come up with something that doesn't have this property.The extra locking introduced here does not look good to me neither.
Please note that although there are additional locking statements in the code, there are no additional locking calls at runtime.
Bart.
Michael S. Tsirkin
maintainerRe: [PATCH 1/2] vhost: Reduce vhost_work_flush() wakeup latency
Quoted 26 lines · Bart Van Assche
Quoted 25 lines · Asias He
Quoted 23 lines · Michael S. Tsirkin
Quoted 9 lines · Bart Van Assche
Quoted 8 lines · Michael S. Tsirkin
Quoted 6 lines · Bart Van Assche
If the
TIF_NEED_RESCHEDtask flag is set, wake up anyvhost_work_flush()waiters before rescheduling instead of after rescheduling.Signed-off-by Bart Van Assche <[email protected]>Cc Michael S. Tsirkin <[email protected]>Cc Asias He <[email protected]>Quoted 8 lines
Quoted 1 line
Why exactly? It's not like flush needs to be extra fast ...
I'm not worried about how fast a flush is processed either. But I think this patch is a nice cleanup of the
vhost_worker()function. It eliminates theuninitialized_var()construct and moves the assignment of "done_seq" below the read of "queue_seq".Bart.
I'm not worried about
uninitialized_var- it's just a compiler bug.done_seqbelow read is nice, but this is at the cost of sprinkling lock/unlock, lock/unlock all over the code. Maybe we can come up with something that doesn't have this property.
Quoted 6 lines
Quoted 1 line
The extra locking introduced here does not look good to me neither.
Please note that although there are additional locking statements in the code, there are no additional locking calls at runtime.
Bart.
That's true but this is supposed to be a cleanup.
Signature · 1 line
MST
PATCH 2/2 vhost: Avoid that vhost_work_flush() returns early
In reply to Bart Van AsscheIf two or more items are queued on dev->work_list before vhost_worker() starts processing these then the value of work->done_seq will be set to the sequence number of a work item that has not yet been processed. Avoid this by letting vhost_worker() count the number of items that have already been processed.
--- drivers/vhost/vhost.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/vhost/vhost.c +2 -3
@@ -202,7 +202,7 @@ static int vhost_worker(void *data) {struct vhost_dev *dev = data;struct vhost_work *work; -unsigned seq; +unsigned seq =0 ;mm_segment_t oldfs =get_fs ();set_fs (USER_DS );
@@ -216,14 +216,13 @@ static int vhost_worker(void *data) work =list_first_entry (&dev->work_list, struct vhost_work, node);list_del_init (&work->node); - seq = work->queue_seq;spin_unlock_irq (&dev->work_lock);__set_current_state (TASK_RUNNING ); work->fn (work);spin_lock_irq (&dev->work_lock); - work->done_seq = seq; + work->done_seq = ++seq;if (work->flushing)wake_up_all (&work->done); }
Signature · 1 line
1.7.10.4
Michael S. Tsirkin
maintainerRe: [PATCH 2/2] vhost: Avoid that vhost_work_flush() returns early
Quoted 10 lines · Bart Van Assche
If two or more items are queued on
dev->work_listbeforevhost_worker()starts processing these then the value ofwork->done_seqwill be set to the sequence number of a work item that has not yet been processed. Avoid this by lettingvhost_worker()count the number of items that have already been processed.Signed-off-by Bart Van Assche <[email protected]>Cc Michael S. Tsirkin <[email protected]>Cc Asias He <[email protected]>
I'm confused by this explanation.done_seq is set here:
if (work) {
work->done_seq = seq;
if (work->flushing)
wake_up_all(&work->done);
}and work is set here:
if (!list_empty(&dev->work_list)) {
work = list_first_entry(&dev->work_list,
struct vhost_work, node);
list_del_init(&work->node);
seq = work->queue_seq;
}this work is processed on the next line:
if (work) {
__set_current_state(TASK_RUNNING);
work->fn(work);
if (need_resched())
schedule();
}so how do we end up with a sequence of a work item that isn't processed?
Quoted 35 lines
--- drivers/vhost/vhost.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/vhost/vhost.c +2
-3@@ -202,7 +202,7 @@ static int vhost_worker(void *data) {struct vhost_dev *dev = data;struct vhost_work *work; -unsigned seq; +unsigned seq =0 ;mm_segment_t oldfs =get_fs ();set_fs (USER_DS );@@ -216,14 +216,13 @@ static int vhost_worker(void *data) work =list_first_entry (&dev->work_list, struct vhost_work, node);list_del_init (&work->node); - seq = work->queue_seq;spin_unlock_irq (&dev->work_lock);__set_current_state (TASK_RUNNING ); work->fn (work);spin_lock_irq (&dev->work_lock); - work->done_seq = seq; + work->done_seq = ++seq;if (work->flushing)wake_up_all (&work->done); }Signature · 1 line
1.7.10.4
Re: [PATCH 2/2] vhost: Avoid that vhost_work_flush() returns early
Quoted 26 lines · Michael S. Tsirkin
I'm confused by this explanation.
done_seqis set here:if (work) { work->done_seq = seq; if (work->flushing) wake_up_all(&work->done); }and work is set here:
if (!list_empty(&dev->work_list)) { work = list_first_entry(&dev->work_list, struct vhost_work, node); list_del_init(&work->node); seq = work->queue_seq; }this work is processed on the next line:
if (work) { __set_current_state(TASK_RUNNING); work->fn(work); if (need_resched()) schedule(); }
Quoted 2 lines
so how do we end up with a sequence of a work item that isn't processed?
I was wondering what would happen if a work item got requeued before done_seq is set. I think you are right and this should work fine.
Bart.
Michael S. Tsirkin
maintainerQuoted 5 lines · Bart Van Assche
There are two patches in this series:
* A patch that reducesvhost_work_flush()latency on busy systems.
* A patch that avoids thatvhost_work_flush()returns early.I found these issues via code inspection.
Interesting. What makes you think vhost_work_flush latency matters?
Please copy all relevant mailing lists in the future:
you can find them in the MAINTAINERS file.