lkml.org 
[lkml]   [2009]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/3] workqueue: not allow recursion run_workqueue
Hi Lai,


On Thu, Feb 05, 2009 at 04:18:57PM +0800, Lai Jiangshan wrote:
> Peter Zijlstra wrote:
> > On Thu, 2009-01-22 at 12:06 +0100, Frédéric Weisbecker wrote:
> >
> >> Actually I don't understand when Lai says that it will actually not flush.
> >
> > Yeah, his changelog is an utter mistery to many..
> >
> >
>
> ----
> Suppose what I wanted to say is A, but sometimes I wrote B for my poor
> English, and people got C when they read it. Thank you, Peter.
> ----


Me too! My poor english takes me double time to explain something :-)


> "if (cwq->thread == current)" is a narrowed checking. lockdep can perform
> the proper checking. I think we could hardly write some code which can
> perform the proper checking when lockdep is off.
>
> Why "if (cwq->thread == current)" is a narrowed checking,
> It hasn't tested "if (brother_cwq->thread == current)". (*brother* cwq)
>
> DEADLOCK EXAMPLE for explain my above option:
>
> (work_func0() and work_func1() are work callback, and they
> calls flush_workqueue())
>
> CPU#0 CPU#1
> run_workqueue() run_workqueue()
> work_func0() work_func1()
> flush_workqueue() flush_workqueue()
> flush_cpu_workqueue(0) .
> flush_cpu_workqueue(cpu#1) flush_cpu_workqueue(cpu#0)
> waiting work_func1() in cpu#1 waiting work_func0 in cpu#0


Heh you're right, I did not imagine this one.
But this race condition should be rare, and still, lockdep should have
warned before concerning the recursion flushing, hopefully assuming the developer
built lockdep.


> DEADLOCK!
> So we do not allow recursion.
> And "BUG_ON(cwq->thread == current)" is not enough(but it's better
> than we don't have this line, I think). we should use lockdep to detect
> recursion when we develop.
>
> Answer other email-thread:
>
> Peter Zijlstra wrote:
> > On Thu, 2009-01-22 at 14:03 +0800, Lai Jiangshan wrote:
> >> void do_some_cleanup(void)
> >> {
> >> find_all_queued_work_struct_and_mark_it_old();
> >> flush_workqueue(workqueue);
> >> /* we can destroy old work_struct for we have flushed them */
> >> destroy_old_work_structs();
> >> }
> >>
> >> if work->func() called do_some_cleanup(), it's very probably a bug.
> >
> > Of course it is, if only because calling flush on the same workqueue is
> > pretty dumb.
>
> flush_workqueue() should ensure works are finished, but this example shows
> the work hasn't finished, so flush_workqueue()'s code is not right.
>
> See also flush_workqueue()'s doc:
> * We sleep until all works which were queued on entry have been handled,
> * but we are not livelocked by new incoming ones.
>
> And this example show a bug(destroy the work which still be used)
> for recursion. So in my changlog:
>
> I said it hide deadlock:
> "We use recursion run_workqueue to hidden deadlock when
> keventd trying to flush its own queue."
>
> I said it will be bug(for flush_workqueue() and it's doc is inconsistent):
> "It's bug. When flush_workqueue()(nested in a work callback)returns,
> the workqueue is not really flushed, the sequence statement of
> this work callback will do some thing bad."
>
> And I concluded:
> "So we should not allow workqueue trying to flush its own queue."
>
> If it still mistery, I will explain more.
> I will change my changlog too, I sincerely hope you help me more.
>
> Thanks, Lai
>
> >
> > But I'm still not getting it, flush_workqueue() provides the guarantee
> > that all work enqueued previous to the call will be finished thereafter.
>
> In my example, flush_workqueue() can't guarantee.
>
> >
> > The self-flush stuff you propose to rip out doesn't violate that
> > guarantee afaict.
> >
> > Suppose we have a workqueue Q, with pending work W1..Wn.
> >
> > Suppose W5 will have the nested flush, it will then recursively complete
> > W6..Wn+i, where i accounts for any concurrent worklet additions.
> >
> > Therefore it will have completed (at least) those worklets that were
> > enqueued at the time flush got called.
> >
> > So, to get back at your changelog.
> >
> > 1) yes lockdep will complain -- for good reasons, and I'm all for
> > getting rid of this mis-feature.
> >
> > 2) I've no clue what you're on about
> >
> > 3) more mystery.
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2009-02-05 14:49    [W:0.095 / U:0.568 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site