Re: [PATCH 2/3] workqueue: not allow recursion run_workqueue

[Frederic Weisbecker <fweisbec@gmail.com>]

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.
>