lkml.org 
[lkml]   [2010]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 01/30] infiniband: update workqueue usage
Hello, Roland.  Sorry about the delay.

On 12/15/2010 07:33 PM, Roland Dreier wrote:
> Thanks Tejun. A couple questions:
>
> > * ib_wq is added, which is used as the common workqueue for infiniband
> > instead of the system workqueue. All system workqueue usages
> > including flush_scheduled_work() callers are converted to use and
> > flush ib_wq. This is to prepare for deprecation of
> > flush_scheduled_work().
>
> Why do we want to move to a subsystem-specific workqueue? Can we just
> replace flush_scheduled_work() by cancel_delayed_work_sync() as
> appropriate and not create yet another work queue?

Because there are places where work is used to free the containing
structure. Before a module is unloaded, all works which uses
functions in the module should be flushed; however, if a work is used
to free the containing structure, such work can't be flushed
explicitly, so the workqueue which processes such works should be
flushed.

So, in this case, ib_wq is added primarily to serve as a flush domain.
For driver midlayers, this seems often necessary. Also, the workqueue
doesn't have any dedicated worker and is quite cheap.

>
> > * qib_wq is removed and ib_wq is used instead.
>
> You obviously looked at the comment
>
> - /*
> - * We create our own workqueue mainly because we want to be
> - * able to flush it when devices are being removed. We can't
> - * use schedule_work()/flush_scheduled_work() because both
> - * unregister_netdev() and linkwatch_event take the rtnl lock,
> - * so flush_scheduled_work() can deadlock during device
> - * removal.
> - */
> - qib_wq = create_workqueue("qib");
>
> and know that with the new workqueue stuff, this issue no longer
> exists. But for both my education and also the clarity of the changelog
> for this patch, perhaps you could expand on why ib_wq is safe here.

I think I got confused. I thought the comment was indicating the
separation between qib_wq and qib_cq_wq. It's between system_wq and
qib_wq, right? I'll drop this part from the series, but then again
what's the difference from ib_srp, which flushes the common workqueue?
Why doesn't ib_srp have the same problem?

> > * create[_singlethread]_workqueue() usages are replaced with the new
> > alloc[_ordered]_workqueue(). This removes rescuers from all
> > infiniband workqueues.
>
> What are rescuers?

Normally, all workqueues share global per-cpu worker pool, but certain
workqueues needs forward progress guarantee under memory pressure (the
ones which are used to free memory). In this case, the workqueues are
created with WQ_MEM_RECLAIM and has a single rescuer worker reserved.
So, any workqueue which is in memory reclaim path needs to have the
flag set to avoid the unlikely but still possible deadlock under
memory pressure.

> Can we replace some of these driver-specific work queues by the ib_wq?
>
> Are all these things just possibilities for future cleanup?

Hmm... Yeah, sure, they can be. With the new implementation, separate
workqueues are used for the following purposes.

* As a forward progress guarantee domain as decribed above.

* As a flushing domain.

* As a property domain. Different workqueues have different execution
and queueing properties set.

Unless one of the above is necessary, work items can be queued
together into the same workqueue. Concurrency-wise, it wouldn't make
any difference. They all use the same set of workers anyway, but I
don't know the code well enough to make the changes myself. If you're
interested in doing it, I'll be happy to help.

Thanks.

--
tejun


\
 
 \ /
  Last update: 2010-12-16 17:53    [W:0.121 / U:0.888 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site