[PATCHSET] workqueue: implement and use WQ_UNBOUND

[PATCHSET] workqueue: implement and use WQ_UNBOUND

[Tejun Heo]

Hello, David, Arjan.

These four patches implement unbound workqueues which can be used as
simple execution context provider.  I changed async to use it and will
also make fscache use it.  This can be used by setting WQ_UNBOUND on
workqueue creation.  Works queued to unbound workqueues are implicitly
HIGHPRI and dispatched to unbound workers as soon as resources are
available and the only limitation applied by workqueue code is
@max_active.  IOW, for both async and fscache, things will stay about
the same.

WQ_UNBOUND can serve the role of WQ_SINGLE_CPU.  WQ_SINGLE_CPU is
dropped and replaced by WQ_UNBOUND.

Arjan, I still think we'll be better off using bound workqueues for
async but let's first convert without causing behavior difference.
Either way isn't gonna result in any noticeable difference anyway.  If
you're okay with the conversion, please ack it.

David, this should work for fscache/slow-work the same way too.  That
should relieve your concern, right?  Oh, and Frederic suggested that
we would be better off with something based on tracing API and I
agree, so the debugfs thing is currently dropped from the tree.  What
do you think?

Thanks.

-- 
tejun

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[Tejun Heo]

On 07/02/2010 11:17 AM, Tejun Heo wrote:
> Hello, David, Arjan.
> 
> These four patches implement unbound workqueues which can be used as
> simple execution context provider.  I changed async to use it and will
> also make fscache use it.  This can be used by setting WQ_UNBOUND on
> workqueue creation.  Works queued to unbound workqueues are implicitly
> HIGHPRI and dispatched to unbound workers as soon as resources are
> available and the only limitation applied by workqueue code is
> @max_active.  IOW, for both async and fscache, things will stay about
> the same.
> 
> WQ_UNBOUND can serve the role of WQ_SINGLE_CPU.  WQ_SINGLE_CPU is
> dropped and replaced by WQ_UNBOUND.
> 
> Arjan, I still think we'll be better off using bound workqueues for
> async but let's first convert without causing behavior difference.
> Either way isn't gonna result in any noticeable difference anyway.  If
> you're okay with the conversion, please ack it.
> 
> David, this should work for fscache/slow-work the same way too.  That
> should relieve your concern, right?  Oh, and Frederic suggested that
> we would be better off with something based on tracing API and I
> agree, so the debugfs thing is currently dropped from the tree.  What
> do you think?

Oops, forgot something.  These four patches are on top of
wq#for-next-candidate branch which is cmwq take#6 + four fix patches

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

and available in the following git tree.

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git review-cmwq

Thanks.

-- 
tejun

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[Tejun Heo]

On 07/02/2010 11:17 AM, Tejun Heo wrote:
> Arjan, I still think we'll be better off using bound workqueues for
> async but let's first convert without causing behavior difference.
> Either way isn't gonna result in any noticeable difference anyway.  If
> you're okay with the conversion, please ack it.

Ping, Arjan.

Thanks.

--
tejun

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[Tejun Heo]

Hello,

On 07/07/2010 07:41 AM, Tejun Heo wrote:
> On 07/02/2010 11:17 AM, Tejun Heo wrote:
>> Arjan, I still think we'll be better off using bound workqueues for
>> async but let's first convert without causing behavior difference.
>> Either way isn't gonna result in any noticeable difference anyway.  If
>> you're okay with the conversion, please ack it.
> 
> Ping, Arjan.

Just for the record, I pinged Arjan again offlist and Arjan acked the
conversion in the reply.  Added Acked-by and pushed the conversion to
for-next-candidate which will be pushed into linux-next the next week.

Thanks.

-- 
tejun

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[David Howells]

Tejun Heo <tj@kernel.org> wrote:

> David, this should work for fscache/slow-work the same way too.  That
> should relieve your concern, right?

Not at the moment.  What does this mean:

	 * Unbound workqueues aren't concurrency managed and should be
	 * dispatched to workers immediately.

Does this mean you don't get reentrancy guarantees with unbounded work queues?

I can't work out how you're achieving it with unbounded queues.  I presume with
CPU-bound workqueues your doing it by binding the work item to the current CPU
still...

Btw, how does this fare in an RT system, where work items bound to a CPU can't
get executed because their CPU is busy with an RT thread, even though there are
other, idle CPUs?

> Oh, and Frederic suggested that we would be better off with something based
> on tracing API and I agree, so the debugfs thing is currently dropped from
> the tree.  What do you think?

I probably disagree.  I just want to be able to cat a file and see the current
runqueue state.  I don't want to have to write and distribute a special program
to do this.  Of course, I don't know that much about the tracing API, so
cat'ing a file to get the runqueue listed nicely may be possible with that.

David

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[Tejun Heo]

Hello, David.

"David Howells" <dhowells@redhat.com> wrote:
> Does this mean you don't get reentrancy guarantees with unbounded work queues?

It means that unbound wq behaves like a generic worker pool.  Bound wq limits concurrency to minimal level but unbound one executes works as long as resources are available.  I'll continue below.

>I can't work out how you're achieving it with unbounded queues.  I presume with
>CPU-bound workqueues your doing it by binding the work item to the current CPU
>still...

Unbound works are served by a dedicated gcwq whose workers are not affine to any particular CPU.  As all unbound works are served by the same gcwq, non reentrancy is automatically guaranteed.

>Btw, how does this fare in an RT system, where work items bound to a CPU can't
>get executed because their CPU is busy with an RT thread, even though there are
>other, idle CPUs?

Sure, there's nothing special about unbound workers.  They're just normal kthreads.

>> Oh, and Frederic suggested that we would be better off with something based
>> on tracing API and I agree, so the debugfs thing is currently dropped from
>> the tree.  What do you think?
>
>I probably disagree.  I just want to be able to cat a file and see the current
>runqueue state.  I don't want to have to write and distribute a special program
>to do this.  Of course, I don't know that much about the tracing API, so
>cat'ing a file to get the runqueue listed nicely may be possible with that.

I'm relatively sure we can do that.  Frederic?

Thanks.

-- 
tejun

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[David Howells]

Tejun Heo <tj@kernel.org> wrote:

> As all unbound works are served by the same gcwq, non reentrancy is
> automatically guaranteed.

That doesn't actually explain _how_ it's non-reentrant.  The gcwq includes a
collection of threads that can execute from it, right?  If so, what mechanism
prevents two threads from executing the same work item, if that work item
isn't bound to a CPU?  I've been trying to figure this out from the code, but
I don't see it offhand.

> > Btw, how does this fare in an RT system, where work items bound to a CPU
> > can't get executed because their CPU is busy with an RT thread, even
> > though there are other, idle CPUs?
> 
> Sure, there's nothing special about unbound workers.  They're just normal
> kthreads.

I should've been clearer: As I understand it, normal (unbound) worker items
are bound to the CPU on which they were queued, and will be executed there
only (barring CPU removal).  If that's the case, isn't it possible that work
items can be prevented from getting execution time by an RT thread that's
hogging a CPU and won't let go?

David

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[Tejun Heo]

Hello,

On 07/21/2010 03:08 PM, David Howells wrote:
> Tejun Heo <tj@kernel.org> wrote:
> 
>> As all unbound works are served by the same gcwq, non reentrancy is
>> automatically guaranteed.
> 
> That doesn't actually explain _how_ it's non-reentrant.  The gcwq includes a
> collection of threads that can execute from it, right?  If so, what mechanism
> prevents two threads from executing the same work item, if that work item
> isn't bound to a CPU?  I've been trying to figure this out from the code, but
> I don't see it offhand.

Sharing the same gcwq is why workqueues bound to one CPU have
non-reentrancy, so they're using the same mechanism.  If it doesn't
work for unbound workqueues, the normal ones are broken too.  Each
gcwq keeps track of currently running works in a hash table and looks
whether the work in question is already executing before starting
executing it.  It's a bit complex but as a work_struct may be freed
once execution starts, the status needs to be tracked outside.

>>> Btw, how does this fare in an RT system, where work items bound to a CPU
>>> can't get executed because their CPU is busy with an RT thread, even
>>> though there are other, idle CPUs?
>>
>> Sure, there's nothing special about unbound workers.  They're just normal
>> kthreads.
> 
> I should've been clearer: As I understand it, normal (unbound) worker items
> are bound to the CPU on which they were queued, and will be executed there
> only (barring CPU removal).  If that's the case, isn't it possible that work
> items can be prevented from getting execution time by an RT thread that's
> hogging a CPU and won't let go?

Yeah, for bound workqueues, sure.  That's exactly the same as the
original workqueue implementation.  For unbound workqueues, it doesn't
matter.

Thanks.

-- 
tejun

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[Tejun Heo]

Just a bit of clarification.

On 07/21/2010 04:59 PM, Tejun Heo wrote:
>> I should've been clearer: As I understand it, normal (unbound) worker items

In workqueue land, normal workqueues would be bound to CPUs while
workers for WQ_UNBOUND workqueues aren't affined to any specific CPU.

Thanks.

-- 
tejun

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[David Howells]

Tejun Heo <tj@kernel.org> wrote:

> Each gcwq keeps track of currently running works in a hash table and looks
> whether the work in question is already executing before starting executing
> it.  It's a bit complex but as a work_struct may be freed once execution
> starts, the status needs to be tracked outside.

Thanks, that's what I wanted to know.

I presume this survives an executing work_struct being freed, reallocated and
requeued before the address of the work_struct is removed from the hash table?

I can see at least one way of doing this: marking the work_struct address in
the hash when the address becomes pending again so that the process of hash
removal will cause the work_struct to be requeued automatically.

David

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[Tejun Heo]

Hello,

On 07/21/2010 05:25 PM, David Howells wrote:
>> Each gcwq keeps track of currently running works in a hash table and looks
>> whether the work in question is already executing before starting executing
>> it.  It's a bit complex but as a work_struct may be freed once execution
>> starts, the status needs to be tracked outside.
> 
> Thanks, that's what I wanted to know.
> 
> I presume this survives an executing work_struct being freed, reallocated and
> requeued before the address of the work_struct is removed from the hash table?

It will unnecessarily stall the execution of the new work if the last
work is still running but nothing will be broken correctness-wise.

> I can see at least one way of doing this: marking the work_struct address in
> the hash when the address becomes pending again so that the process of hash
> removal will cause the work_struct to be requeued automatically.

If I'm correctly understanding what you're saying, the code already
does about the same thing.

Thanks.

-- 
tejun

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[David Howells]

Tejun Heo <tj@kernel.org> wrote:

> If I'm correctly understanding what you're saying, the code already
> does about the same thing.

Cool.

Btw, it seems to work for fscache.  Feel free to add my Acked-by to your
patches.

David

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[Tejun Heo]

Hello,

On 07/21/2010 05:38 PM, David Howells wrote:
> Tejun Heo <tj@kernel.org> wrote:
> 
>> If I'm correctly understanding what you're saying, the code already
>> does about the same thing.
> 
> Cool.
> 
> Btw, it seems to work for fscache.  Feel free to add my Acked-by to your
> patches.

Great, I'll start working on the debugging stuff once things settle
down a bit.  Thank you.

-- 
tejun

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[David Howells]

Tejun Heo <tj@kernel.org> wrote:

> It will unnecessarily stall the execution of the new work if the last
> work is still running but nothing will be broken correctness-wise.

That's fine.  Better that than risk unexpected reentrance.  You could add a
function to allow an executing work item to yield the hash entry to indicate
that the work_item that invoked it has been destroyed, but it's probably not
worth it, and it has scope for mucking things up horribly if used at the wrong
time.

I presume also that if a work_item being executed on one work queue is queued
on another work queue, then there is no non-reentrancy guarantee (which is
fine; if you don't like that, don't do it).

David

Re: [PATCHSET] workqueue: implement and use WQ_UNBOUND

[Tejun Heo]

Hello,

On 07/21/2010 05:45 PM, David Howells wrote:
> That's fine.  Better that than risk unexpected reentrance.  You could add a
> function to allow an executing work item to yield the hash entry to indicate
> that the work_item that invoked it has been destroyed, but it's probably not
> worth it, and it has scope for mucking things up horribly if used at the wrong
> time.

Yeah, I agree, it's going too far and can be easily misused.  Given
that there are very few users which actually do that, I think it would
be best to leave it alone.

> I presume also that if a work_item being executed on one work queue is queued
> on another work queue, then there is no non-reentrancy guarantee (which is
> fine; if you don't like that, don't do it).

Right, there is no non-reentrancy guarantee.

Thanks.

-- 
tejun