Prev: [PATCH] ACPI: skip checking BM_STS if the BIOS doesn't ask for it
Next: generic changes for memblock
From: Tejun Heo on 22 Jul 2010 17:30
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 linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on 25 Jul 2010 03:50
Hello,

On 07/24/2010 09:14 PM, Michael S. Tsirkin wrote:
>> 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
>
> Sure, if we keep using workqueue. But I'd like to investigate this
> direction a bit more because there's discussion to switching from kthread to
> regular threads altogether.

Hmmm? It doesn't have much to do with workqueue. kthread_worker is a
simple wrapper around kthread. It now assumes kthread but changing it
to be useable with any thread shouldn't be too hard. Wouldn't that be
better?

>> I don't think doing this before executing the function is correct,
>
> Well, before I execute the function work is NULL, so this is skipped.
> Correct?
>
>> so
>> you'll have to release the lock, execute the function, regrab the lock
>> and then do the flush processing.
>>
>> Thanks.
>
> It's done in the loop, so I thought we can reuse the locking
> done for the sake of processing the next work item.
> Makes sense?

Yeap, right. I think it would make much more sense to use common code
when it becomes available but if you think the posted change is
necessary till then, please feel free to go ahead.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on 26 Jul 2010 11:40
Hello,

On 07/26/2010 05:25 PM, Michael S. Tsirkin wrote:
> BTW, kthread_worker would benefit from the optimization I implemented
> here as well.

Hmmm... I'm not quite sure whether it's an optimization. I thought
the patch was due to feeling uncomfortable about using barriers? Is
it an optimization?

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on 26 Jul 2010 11:50
On 07/26/2010 05:34 PM, Tejun Heo wrote:
> Hello,
>
> On 07/26/2010 05:25 PM, Michael S. Tsirkin wrote:
>> BTW, kthread_worker would benefit from the optimization I implemented
>> here as well.
>
> Hmmm... I'm not quite sure whether it's an optimization. I thought
> the patch was due to feeling uncomfortable about using barriers? Is
> it an optimization?

Yeah, one less smp_mb() in execution path. The lock dancing in
flush() is ugly but then again mucking with barriers could be harder
to understand. Care to send a patch against wq#for-next tree?

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on 26 Jul 2010 12:10
Hello,

On 07/26/2010 05:50 PM, Michael S. Tsirkin wrote:
>> Hmmm... I'm not quite sure whether it's an optimization. I thought
>> the patch was due to feeling uncomfortable about using barriers?
>
> Oh yes. But getting rid of barriers is what motivated me originally.

Yeah, getting rid of barriers is always good. :-)

> Is there a git tree with kthread_worker applied?
> I might do this just for fun ...

git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-next

For the original implementaiton, please take a look at commit
b56c0d8937e665a27d90517ee7a746d0aa05af46.

* Can you please keep the outer goto repeat loop? I just don't like
outermost for (;;).

* Placing try_to_freeze() could be a bit annoying. It shouldn't be
executed when there's a work to flush.

* I think A - B <= 0 test would be more familiar. At least
time_before/after() are implemented that way.

Thanks.

--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3
Prev: [PATCH] ACPI: skip checking BM_STS if the BIOS doesn't ask for it
Next: generic changes for memblock