lkml.org 
[lkml]   [2008]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] workqueues: insert_work: use "list_head *" instead of "int tail"
On 06/12, Peter Zijlstra wrote:
>
> On Thu, 2008-06-12 at 21:44 +0400, Oleg Nesterov wrote:
>
> > > Hence that idea of flush context and completions.
> >
> > Do you mean something like (just for example) below? If yes, then yes
> > sure, flush_work() is limited. But I can't see how it is possible to
> > "generalize" this idea.
> >
> > (hmm... actually, if we add flush_work(), we can speedup schedule_on_each_cpu(),
> > instead of flush_workqueue(keventd_wq) we can do
> >
> > for_each_online_cpu(cpu)
> > flush_work(per_cpu_ptr(works, cpu));
> >
> > not sure this really makes sense though).
>
> Speedups are always nice ;-),

OK, I'm sending the patch.

> but the below also gets us there.

yeah, and it needs only 1 wakeup. But otoh it is much more complex :(

> > +struct xxx
> > +{
> > + atomic_t count;
> > + struct completion done;
> > + work_func_t func;
> > +};
> > +
> > +struct yyy
> > +{
> > + struct work_struct work;
> > + struct xxx *xxx;
> > +};
> > +
> > +static void yyy_func(struct work_struct *work)
> > +{
> > + struct xxx *xxx = container_of(work, struct yyy, work)->xxx;
> > + xxx->func(work);
> > +
> > + if (atomic_dec_and_test(&xxx->count))
> > + complete(&xxx->done);
> > +}
> > ...
>
> Yes, along those lines.
>
> you can call xxx a flush_context and create an interface like:
>
> int queue_work_contex(struct workqueue_struct *wq,
> struct flush_context *fc, struct work_struct *work)
> {
> work->context = fc;
> return queue_work(wq, work);
> }
>
> void flush_workqueue_context(struct workqueue_strucy *wq, t
> struct flush_context *fc)
> {
> if (atomic_read(&context->count))
> wait_for_completion(&fc->completion);
> /* except that the above is racy, wait_event() comes to mind */
> }
>
> of course run_workqueue() would then need to be augmented with something
> like:
>
> context = work->context;
> ...
> f(work);
> ...
> if (context && atomic_dec_and_test(&context->count))
> complete(&context->done);

> also, I seem to have quitely ignored the fact that struct work doesn't
> have the context pointer, and growing it unconditionally like this isn't
> nice - hummm,. perhaps we have a bit left in data and can signify a
> larger struct work_struct.. ?

Yes, we have a free bit... but afaics we can do better.

struct context_barrier {
struct work_struct work;
struct flush_context *fc;
...
}
void context_barrier_barrier_func(struct work_struct *work)
{
struct flush_context *fc = container_of();
if (atomic_dec_and_test())
...
}
void insert_context_barrier(work, barr)
{
...insert barr after work, like flush_work() does...
}
queue_work_contex(struct workqueue_struct *wq,
struct work_struct *work,
struct flush_context *fc)
{
int ret = queue_work(wq, work);
if (ret)
insert_context_barrier(work, barr);
return ret;
}
this way we shouldn't change run_workqueue() and introduce a "parallel"
larger work_struct which needs its own INIT_()/etc.

However I'm a bit sceptical this will be widely used... I may be wrong.

> making all this PI savvy for -rt is going to be fun though.. I guess we
> can just queue a normal barrier of the flusher's priority, and cancel it
> once we complete.. hey - that doesn't sound hard at all :-)

Yes!!! I think this is much better (because _much_ simple) than re-ordering
the pending work_struct's, we can just boost the whole ->worklist. We can
implement flush_work_pi() in the same manner as queue_work_contex() above.
That is why I said previously that flush_() should govern the priority,
not queue.

But we can also implement queue_work_pi(struct work_struct_pi *work).

Oleg.



\
 
 \ /
  Last update: 2008-06-13 16:29    [from the cache]
©2003-2011 Jasper Spaans. Advertise on this site