(on top of [PATCH] workqueues: insert_work: use "list_head *" instead of "int tail"
http://marc.info/?l=linux-kernel&m=121328944230175)
Most of users of flush_workqueue() can be changed to use cancel_work_sync(),
but sometimes we really need to wait for the completion and cancelling is not
an option. schedule_on_each_cpu() is good example.
Add the new helper, flush_work(work), which waits for the completion of the
specific work_struct.
By its nature it requires that this work must not be re-queued, and thus its
usage is limited. For example, this code
queue_work(wq, work);
/* WINDOW */
queue_work(wq, work);
flush_work(work);
is not right. What can happen in the WINDOW above is
- wq starts the execution of work->func()
- the caller migrates to another CPU
now, after the 2nd queue_work() this work is active on the previous CPU, and
at the same time it is queued on another. We can fix this limitation, but in
that case we have to iterate over all CPUs like wait_on_work() does, this
will depreciate the advantages of this helper.
Signed-off-by: Oleg Nesterov <[email protected]>
--- 26-rc2/include/linux/workqueue.h~WQ_2_FLUSH_WORK 2008-05-18 15:42:34.000000000 +0400
+++ 26-rc2/include/linux/workqueue.h 2008-05-18 15:42:34.000000000 +0400
@@ -198,6 +198,8 @@ extern int keventd_up(void);
extern void init_workqueues(void);
int execute_in_process_context(work_func_t fn, struct execute_work *);
+extern int flush_work(struct work_struct *work);
+
extern int cancel_work_sync(struct work_struct *work);
/*
--- 26-rc2/kernel/workqueue.c~WQ_2_FLUSH_WORK 2008-06-12 21:28:13.000000000 +0400
+++ 26-rc2/kernel/workqueue.c 2008-06-13 17:31:54.000000000 +0400
@@ -399,6 +399,52 @@ void flush_workqueue(struct workqueue_st
}
EXPORT_SYMBOL_GPL(flush_workqueue);
+/**
+ * flush_work - block until a work_struct's callback has terminated
+ * @work: the work which is to be flushed
+ *
+ * It is expected that, prior to calling flush_work(), the caller has
+ * arranged for the work to not be requeued, otherwise it doesn't make
+ * sense to use this function.
+ */
+int flush_work(struct work_struct *work)
+{
+ struct cpu_workqueue_struct *cwq;
+ struct list_head *prev;
+ struct wq_barrier barr;
+
+ might_sleep();
+ cwq = get_wq_data(work);
+ if (!cwq)
+ return 0;
+
+ prev = NULL;
+ spin_lock_irq(&cwq->lock);
+ if (unlikely(cwq->current_work == work)) {
+ prev = &cwq->worklist;
+ } else {
+ if (list_empty(&work->entry))
+ goto out;
+ /*
+ * See the comment near try_to_grab_pending()->smp_rmb().
+ * If it was re-queued under us we are not going to wait.
+ */
+ smp_rmb();
+ if (cwq != get_wq_data(work))
+ goto out;
+ prev = &work->entry;
+ }
+ insert_wq_barrier(cwq, &barr, prev->next);
+out:
+ spin_unlock_irq(&cwq->lock);
+ if (!prev)
+ return 0;
+
+ wait_for_completion(&barr.done);
+ return 1;
+}
+EXPORT_SYMBOL_GPL(flush_work);
+
/*
* Upon a successful return (>= 0), the caller "owns" WORK_STRUCT_PENDING bit,
* so this work can't be re-armed in any way.
On 06/13, Oleg Nesterov wrote:
>
> Add the new helper, flush_work(work), which waits for the completion of the
> specific work_struct.
>
> By its nature it requires that this work must not be re-queued, and thus its
> usage is limited. For example, this code
>
> queue_work(wq, work);
> /* WINDOW */
> queue_work(wq, work);
>
> flush_work(work);
>
> is not right. What can happen in the WINDOW above is
>
> - wq starts the execution of work->func()
>
> - the caller migrates to another CPU
>
> now, after the 2nd queue_work() this work is active on the previous CPU, and
> at the same time it is queued on another.
To clarify, in the scenario above, flush_work() behaves correctly. It "flushes"
the result of the last queue_work() which is visible to the caller. We can add
another simple helper,
int flush_work_sync(work)
{
return flush_work(work) || wait_on_work(work);
}
but I think it is not needed.
In short: flush_work() should be use with care when it is really needed.
Oleg.
On Fri, Jun 13, 2008 at 06:28:01PM +0400, Oleg Nesterov wrote:
> (on top of [PATCH] workqueues: insert_work: use "list_head *" instead of "int tail"
> http://marc.info/?l=linux-kernel&m=121328944230175)
>
> Most of users of flush_workqueue() can be changed to use cancel_work_sync(),
> but sometimes we really need to wait for the completion and cancelling is not
> an option. schedule_on_each_cpu() is good example.
>
> Add the new helper, flush_work(work), which waits for the completion of the
> specific work_struct.
This all looks right and better than current flush_, but... the main
problem is that probably in 90% cases cancel_ + self-running a work
function (if cancelled) should be both more efficient and safer wrt
locking (what you convince me to, BTW). But, since there is no visible
macro/wrapper for this, people will probably mostly choose what they
can see in workqueue.h, so not necessarily the best thing.
Another question is if schedule_on_each_cpu() is really such a good
example here: it seems these "xxx && yyy" examples could be faster,
but I've lost track of this earlier thread.
BTW, flush_work() probably needs a lockdep annotation similar to
flush_workqueue().
Otherwise this all looks OK to me.
Thanks,
Jarek P.
On 06/14, Jarek Poplawski wrote:
>
> On Fri, Jun 13, 2008 at 06:28:01PM +0400, Oleg Nesterov wrote:
> > (on top of [PATCH] workqueues: insert_work: use "list_head *" instead of "int tail"
> > http://marc.info/?l=linux-kernel&m=121328944230175)
> >
> > Most of users of flush_workqueue() can be changed to use cancel_work_sync(),
> > but sometimes we really need to wait for the completion and cancelling is not
> > an option. schedule_on_each_cpu() is good example.
> >
> > Add the new helper, flush_work(work), which waits for the completion of the
> > specific work_struct.
>
> This all looks right and better than current flush_, but... the main
> problem is that probably in 90% cases cancel_ + self-running a work
> function (if cancelled) should be both more efficient and safer wrt
> locking (what you convince me to, BTW).
Yes, in most cases cancel_ is enough. And it is safer, note the limitations
of flush_work(). Basically, flush_work(work) should be used when this
work_struct can be queued only once.
> Another question is if schedule_on_each_cpu() is really such a good
> example here: it seems these "xxx && yyy" examples could be faster,
> but I've lost track of this earlier thread.
schedule_on_each_cpu() can't use cancel_ + ->func(), the code should
be executed on the remote CPU.
And note that flush_work() doesn't iterate over all CPUs, this is the
reason why it is limited, but this also means it is faster than
flush_work_sync() == flush_work() + wait_on_work().
> BTW, flush_work() probably needs a lockdep annotation similar to
> flush_workqueue().
Yes I know... but I'd prefer to send another patch, I'm a bit paranoid
when it comes to copy-and-pasting the code.
> Otherwise this all looks OK to me.
Thanks for review!
Oleg.
On Sat, Jun 14, 2008 at 05:59:51PM +0400, Oleg Nesterov wrote:
> On 06/14, Jarek Poplawski wrote:
...
> > Another question is if schedule_on_each_cpu() is really such a good
> > example here: it seems these "xxx && yyy" examples could be faster,
> > but I've lost track of this earlier thread.
>
> schedule_on_each_cpu() can't use cancel_ + ->func(), the code should
> be executed on the remote CPU.
???
Actually I meant this "xxx && yyy" schedule_on_each_cpu():
http://marc.info/?l=linux-kernel&m=121329259203187&w=2
Regards,
Jarek P.
On 06/14, Jarek Poplawski wrote:
>
> On Sat, Jun 14, 2008 at 05:59:51PM +0400, Oleg Nesterov wrote:
> > On 06/14, Jarek Poplawski wrote:
> ...
> > > Another question is if schedule_on_each_cpu() is really such a good
> > > example here: it seems these "xxx && yyy" examples could be faster,
> > > but I've lost track of this earlier thread.
> >
> > schedule_on_each_cpu() can't use cancel_ + ->func(), the code should
> > be executed on the remote CPU.
>
> ???
> Actually I meant this "xxx && yyy" schedule_on_each_cpu():
>
> http://marc.info/?l=linux-kernel&m=121329259203187&w=2
Ah, sorry, I misunderstood.
Yes, "xxx && yyy" needs only 1 wakeup, so it needs less CPU. But it also
more complex, and it is not easy to generalize this.
Oleg.