Re: [PATCH UPDATED 1/3] vhost: replace vhost_workqueue with per-vhost kthread

[Tejun Heo <tj@kernel.org>]

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