| Message ID | 520B2B88.6020307@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 14, 2013 at 09:02:32AM +0200, Bart Van Assche wrote: > If the TIF_NEED_RESCHED task flag is set, wake up any vhost_work_flush() > waiters before rescheduling instead of after rescheduling. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Asias He <asias@redhat.com> Why exactly? It's not like flush needs to be extra fast ... > --- > drivers/vhost/vhost.c | 42 +++++++++++++++++++----------------------- > 1 file changed, 19 insertions(+), 23 deletions(-) > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > index e58cf00..e7ffc10 100644 > --- a/drivers/vhost/vhost.c > +++ b/drivers/vhost/vhost.c > @@ -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; > -- > 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/14/13 13:37, Michael S. Tsirkin wrote: > On Wed, Aug 14, 2013 at 09:02:32AM +0200, Bart Van Assche wrote: >> If the TIF_NEED_RESCHED task flag is set, wake up any vhost_work_flush() >> waiters before rescheduling instead of after rescheduling. >> >> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Cc: Asias He <asias@redhat.com> > > 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 14, 2013 at 05:22:36PM +0200, Bart Van Assche wrote: > On 08/14/13 13:37, Michael S. Tsirkin wrote: > >On Wed, Aug 14, 2013 at 09:02:32AM +0200, Bart Van Assche wrote: > >>If the TIF_NEED_RESCHED task flag is set, wake up any vhost_work_flush() > >>waiters before rescheduling instead of after rescheduling. > >> > >>Signed-off-by: Bart Van Assche <bvanassche@acm.org> > >>Cc: Michael S. Tsirkin <mst@redhat.com> > >>Cc: Asias He <asias@redhat.com> > > > >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. 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.
On Wed, Aug 14, 2013 at 08:58:25PM +0300, Michael S. Tsirkin wrote: > On Wed, Aug 14, 2013 at 05:22:36PM +0200, Bart Van Assche wrote: > > On 08/14/13 13:37, Michael S. Tsirkin wrote: > > >On Wed, Aug 14, 2013 at 09:02:32AM +0200, Bart Van Assche wrote: > > >>If the TIF_NEED_RESCHED task flag is set, wake up any vhost_work_flush() > > >>waiters before rescheduling instead of after rescheduling. > > >> > > >>Signed-off-by: Bart Van Assche <bvanassche@acm.org> > > >>Cc: Michael S. Tsirkin <mst@redhat.com> > > >>Cc: Asias He <asias@redhat.com> > > > > > >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. > > 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. The extra locking introduced here does not look good to me neither. > -- > MST
On 08/15/13 03:30, Asias He wrote: > On Wed, Aug 14, 2013 at 08:58:25PM +0300, Michael S. Tsirkin wrote: >> On Wed, Aug 14, 2013 at 05:22:36PM +0200, Bart Van Assche wrote: >>> On 08/14/13 13:37, Michael S. Tsirkin wrote: >>>> On Wed, Aug 14, 2013 at 09:02:32AM +0200, Bart Van Assche wrote: >>>>> If the TIF_NEED_RESCHED task flag is set, wake up any vhost_work_flush() >>>>> waiters before rescheduling instead of after rescheduling. >>>>> >>>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >>>>> Cc: Michael S. Tsirkin <mst@redhat.com> >>>>> Cc: Asias He <asias@redhat.com> >>>> >>>> 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. >> >> 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. > > 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 15, 2013 at 08:13:14AM +0200, Bart Van Assche wrote: > On 08/15/13 03:30, Asias He wrote: > >On Wed, Aug 14, 2013 at 08:58:25PM +0300, Michael S. Tsirkin wrote: > >>On Wed, Aug 14, 2013 at 05:22:36PM +0200, Bart Van Assche wrote: > >>>On 08/14/13 13:37, Michael S. Tsirkin wrote: > >>>>On Wed, Aug 14, 2013 at 09:02:32AM +0200, Bart Van Assche wrote: > >>>>>If the TIF_NEED_RESCHED task flag is set, wake up any vhost_work_flush() > >>>>>waiters before rescheduling instead of after rescheduling. > >>>>> > >>>>>Signed-off-by: Bart Van Assche <bvanassche@acm.org> > >>>>>Cc: Michael S. Tsirkin <mst@redhat.com> > >>>>>Cc: Asias He <asias@redhat.com> > >>>> > >>>>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. > >> > >>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. > > > >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.
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index e58cf00..e7ffc10 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -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;
If the TIF_NEED_RESCHED task flag is set, wake up any vhost_work_flush() waiters before rescheduling instead of after rescheduling. Signed-off-by: Bart Van Assche <bvanassche@acm.org> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Asias He <asias@redhat.com> --- drivers/vhost/vhost.c | 42 +++++++++++++++++++----------------------- 1 file changed, 19 insertions(+), 23 deletions(-)