Hello, On 07/22/2010 05:58 PM, Michael S. Tsirkin wrote: > All the tricky barrier pairing made me uncomfortable. So I came up with > this on top (untested): if we do all operations under the spinlock, we > can get by without barriers and atomics. And since we need the lock for > list operations anyway, this should have no paerformance impact. > > What do you think? I've created kthread_worker in wq#for-next tree and already converted ivtv to use it. Once this lands in mainline, I think converting vhost to use it would be better choice. kthread worker code uses basically the same logic used in the vhost_workqueue code but is better organized and documented. So, I think it would be better to stick with the original implementation, as otherwise we're likely to just decrease test coverage without much gain. http://git.kernel.org/?p=linux/kernel/git/tj/wq.git;a=commitdiff;h=b56c0d8937e665a27d90517ee7a746d0aa05af46;hp=53c5f5ba42c194cb13dd3083ed425f2c5b1ec439 > @@ -151,37 +161,37 @@ static void vhost_vq_reset(struct vhost_dev *dev, > static int vhost_worker(void *data) > { > struct vhost_dev *dev = data; > - struct vhost_work *work; > + struct vhost_work *work = NULL; > > -repeat: > - set_current_state(TASK_INTERRUPTIBLE); /* mb paired w/ kthread_stop */ > + for (;;) { > + set_current_state(TASK_INTERRUPTIBLE); /* mb paired w/ kthread_stop */ > > - if (kthread_should_stop()) { > - __set_current_state(TASK_RUNNING); > - return 0; > - } > + if (kthread_should_stop()) { > + __set_current_state(TASK_RUNNING); > + return 0; > + } > > - work = NULL; > - spin_lock_irq(&dev->work_lock); > - if (!list_empty(&dev->work_list)) { > - work = list_first_entry(&dev->work_list, > - struct vhost_work, node); > - list_del_init(&work->node); > - } > - spin_unlock_irq(&dev->work_lock); > + spin_lock_irq(&dev->work_lock); > + if (work) { > + work->done_seq = work->queue_seq; > + if (work->flushing) > + wake_up_all(&work->done); I don't think doing this before executing the function is correct, so you'll have to release the lock, execute the function, regrab the lock and then do the flush processing. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
