Messages in this thread | | | Date | Fri, 13 Jun 2008 18:26:58 +0400 | From | Oleg Nesterov <> | Subject | Re: [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.
| |