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

[Tejun Heo <tj@kernel.org>]

Hello,

On 05/31/2010 05:31 PM, Oleg Nesterov wrote:
>> I might have slightly over engineered this part not knowing the
>> expected workload.  ->queue_seq/->done_seq pair is to guarantee that
>> flushers never get starved.
> 
> Ah, indeed.
> 
> Well, afaics we do not need 2 counters anyway, both vhost_poll_queue()
> and vhost_poller() could increment the single counter and the flusher
> can take bit 0 into account. But I agree 2 counters are much more clean.

Right, we can do that too.  Cool. :-)

>>>> +static int vhost_poller(void *data)
>>>> +{
>>>> +	struct vhost_dev *dev = data;
>>>> +	struct vhost_poll *poll;
>>>> +
>>>> +repeat:
>>>> +	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */
>>>
>>> I don't understand the comment... why do we need this barrier?
>>
>> So that either kthread_stop()'s should_stop = 1 in kthread_stop() is
>> visible to kthread_should_stop() or task state is set to RUNNING.
> 
> Of course, you are right. I am really surprized I asked this question ;)

This part is always a bit tricky tho.  Maybe it would be a good idea
to make kthread_stop() do periodic wakeups.  It's already injecting
one rather unexpected wake up into the mix anyway so there isn't much
point in avoiding multiple and it would make designing kthread loops
easier.  Or maybe what we need is something like kthread_idle() which
encapsulates the check and sleep part.

Thanks.

-- 
tejun