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

"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

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 09:23    [W:0.072 / U:6.352 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site