On Mon, 2007-10-22 at 21:34 +0400, Oleg Nesterov wrote:
> On 10/22, Peter Zijlstra wrote:
> > @@ -136,10 +138,10 @@ static void insert_work(struct cpu_workq
> > */
> > smp_wmb();
> > plist_node_init(&work->entry, prio);
> > - plist_add(&work->entry, &cwq->worklist);
> > + __plist_add(&work->entry, &cwq->worklist, tail);
>
> Hmm. Not sure we really need __plist_add() here. If tail == 0, we must
> insert this work (barrier) at the head of the list. Can't we do
>
> work->entry->prio = tail ? prio : -1;
> plist_add(&work->entry, &cwq->worklist);
>
> instead?
Ah yes, that would work I guess. Very nice!
> > static void run_workqueue(struct cpu_workqueue_struct *cwq)
> > {
> > + struct plist_head *worklist = &cwq->worklist;
> > +
> > spin_lock_irq(&cwq->lock);
> > cwq->run_depth++;
> > if (cwq->run_depth > 3) {
> > @@ -267,16 +280,25 @@ static void run_workqueue(struct cpu_wor
> > __FUNCTION__, cwq->run_depth);
> > dump_stack();
> > }
> > - while (!plist_head_empty(&cwq->worklist)) {
> > - struct work_struct *work = plist_first_entry(&cwq->worklist,
> > +
> > +again:
> > + while (!plist_head_empty(worklist)) {
> > + int prio;
> > + struct work_struct *work = plist_first_entry(worklist,
> > struct work_struct, entry);
> > work_func_t f = work->func;
> >
> > - if (likely(cwq->thread->prio != work->entry.prio))
> > - task_setprio(cwq->thread, work->entry.prio);
> > + prio = work->entry.prio;
> > + if (unlikely(worklist != &cwq->worklist)) {
> > + prio = min(prio, cwq->barrier->prev_prio);
> > + prio = min(prio, plist_first(&cwq->worklist)->prio);
> > + }
> > +
> > + if (likely(cwq->thread->prio != prio))
> > + task_setprio(cwq->thread, prio);
> >
> > cwq->current_work = work;
> > - plist_del(&work->entry, &cwq->worklist);
> > + plist_del(&work->entry, worklist);
> > plist_node_init(&work->entry, MAX_PRIO);
> > spin_unlock_irq(&cwq->lock);
> >
> > @@ -289,7 +311,27 @@ static void run_workqueue(struct cpu_wor
> >
> > spin_lock_irq(&cwq->lock);
> > cwq->current_work = NULL;
> > +
> > + if (unlikely(cwq->barrier))
> > + worklist = &cwq->barrier->worklist;
> > + }
>
> At first glance this looks wrong, but I am not sure I get it right...
>
> So, now we iterate the local worklist, not cwq->worklist. Suppose it has
> the works w1 and w2.
>
> run_workqueue() starts w1->func().
>
> Another thread does cancel_work_sync(w1) under some LOCK. We insert the
> barrier at cwq->worklist and sleep.
>
> w1 completes, run_workqueue() fires w2, w2->func does lock(LOCK) ...
>
> Deadlock.
Ah, cancel_work_sync() will not use wq_full_barrier, but wq_barrier.
Its flush_cpu_workqueue() that will use this new nesting thing.
> (I'll try to read this patch carefully tomorrow, but it is a bit hard to
> read this series, and the very first patch has rejects. Could you make
> a single patch?)
Its against .23-rt1 and applies fine.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
