* [PATCH] slow_work_execute() needs mb() before test_bit(SLOW_WORK_PENDING)
@ 2009-04-13 20:13 Oleg Nesterov
2009-04-13 20:13 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2009-04-13 20:13 UTC (permalink / raw)
To: Andrew Morton, David Howells, David Miller
Cc: Serge Hallyn, Steve Dickson, Trond Myklebust, Al Viro,
Daire Byrne, linux-kernel
slow_work_execute:
clear_bit_unlock(SLOW_WORK_EXECUTING, &work->flags);
if (test_bit(SLOW_WORK_PENDING, &work->flags) {
clear_bit_unlock() implies release semantics, iow we have a one-way barrier
before clear_bit(). But we need the mb() semantics after clear_bit(), before
we test SLOW_WORK_PENDING. Otherwise we can miss SLOW_WORK_ENQ_DEFERRED if
we race slow_work_enqueue().
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
--- 6.30/kernel/slow-work.c~2_BITS_MB 2009-04-13 19:40:20.000000000 +0200
+++ 6.30/kernel/slow-work.c 2009-04-13 21:19:33.000000000 +0200
@@ -198,7 +198,8 @@ static bool slow_work_execute(void)
if (very_slow)
atomic_dec(&vslow_work_executing_count);
- clear_bit_unlock(SLOW_WORK_EXECUTING, &work->flags);
+ clear_bit(SLOW_WORK_EXECUTING, &work->flags);
+ smp_mb__after_clear_bit();
/* if someone tried to enqueue the item whilst we were executing it,
* then it'll be left unenqueued to avoid multiple threads trying to
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] slow_work_execute() needs mb() before test_bit(SLOW_WORK_PENDING)
2009-04-13 20:13 [PATCH] slow_work_execute() needs mb() before test_bit(SLOW_WORK_PENDING) Oleg Nesterov
@ 2009-04-13 20:13 ` Oleg Nesterov
2009-04-13 21:47 ` David Howells
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2009-04-13 20:13 UTC (permalink / raw)
To: Andrew Morton, David Howells, David Miller
Cc: Serge Hallyn, Steve Dickson, Trond Myklebust, Al Viro,
Daire Byrne, linux-kernel
On 04/13, Oleg Nesterov wrote:
>
> slow_work_execute:
>
> clear_bit_unlock(SLOW_WORK_EXECUTING, &work->flags);
>
> if (test_bit(SLOW_WORK_PENDING, &work->flags) {
>
> clear_bit_unlock() implies release semantics, iow we have a one-way barrier
> before clear_bit(). But we need the mb() semantics after clear_bit(), before
> we test SLOW_WORK_PENDING. Otherwise we can miss SLOW_WORK_ENQ_DEFERRED if
> we race slow_work_enqueue().
However, given that both clear_bit() and set_bit() use the same word, perhaps
this is not possible.
But in that case I don't understand why do we need clear_bit_unlock(), not just
clear_bit(), and how "mb is not needeed" could be derived from documentation.
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] slow_work_execute() needs mb() before test_bit(SLOW_WORK_PENDING)
2009-04-13 20:13 ` Oleg Nesterov
@ 2009-04-13 21:47 ` David Howells
2009-04-13 22:02 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: David Howells @ 2009-04-13 21:47 UTC (permalink / raw)
To: Oleg Nesterov
Cc: dhowells, Andrew Morton, David Miller, Serge Hallyn,
Steve Dickson, Trond Myklebust, Al Viro, Daire Byrne,
linux-kernel
Oleg Nesterov <oleg@redhat.com> wrote:
> clear_bit_unlock() implies release semantics, iow we have a one-way barrier
> before clear_bit().
That is correct. The stuff before the clear_bit_unlock() must not leak out
past it. SLOW_WORK_EXECUTING is a lock around the execution of the work item.
> But we need the mb() semantics after clear_bit(), before we test
> SLOW_WORK_PENDING. Otherwise we can miss SLOW_WORK_ENQ_DEFERRED if we race
> slow_work_enqueue().
But if you move the mb() to the other side, and reduce to clear_bit() you
break the above non-reentrancy guarantee.
> However, given that both clear_bit() and set_bit() use the same word,
> perhaps this is not possible.
I would assume clear_bit() and set_bit() on different bits of the same word
must interact as if they're on the same bit, with regard to atomiticity, but I
don't know for certain that it is guaranteed.
> But in that case I don't understand why do we need clear_bit_unlock(), not
> just clear_bit(), and how "mb is not needeed" could be derived from
> documentation.
As mentioned above, SLOW_WORK_EXECUTING is a lock against multiple entrance to
the execution function.
Perhaps I should amend the comments to make this clearer.
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] slow_work_execute() needs mb() before test_bit(SLOW_WORK_PENDING)
2009-04-13 21:47 ` David Howells
@ 2009-04-13 22:02 ` Oleg Nesterov
2009-04-16 9:43 ` David Howells
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2009-04-13 22:02 UTC (permalink / raw)
To: David Howells
Cc: Andrew Morton, David Miller, Serge Hallyn, Steve Dickson,
Trond Myklebust, Al Viro, Daire Byrne, linux-kernel
On 04/13, David Howells wrote:
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > clear_bit_unlock() implies release semantics, iow we have a one-way barrier
> > before clear_bit().
>
> That is correct. The stuff before the clear_bit_unlock() must not leak out
> past it. SLOW_WORK_EXECUTING is a lock around the execution of the work item.
>
> > But we need the mb() semantics after clear_bit(), before we test
> > SLOW_WORK_PENDING. Otherwise we can miss SLOW_WORK_ENQ_DEFERRED if we race
> > slow_work_enqueue().
>
> But if you move the mb() to the other side, and reduce to clear_bit() you
> break the above non-reentrancy guarantee.
Hmm. Can't understand... please see below.
> > However, given that both clear_bit() and set_bit() use the same word,
> > perhaps this is not possible.
>
> I would assume clear_bit() and set_bit() on different bits of the same word
> must interact as if they're on the same bit, with regard to atomiticity, but I
> don't know for certain that it is guaranteed.
Yes, this was my concern. Reading the docs, I can't see how it is possible
to "prove" that clear_bit(x, WORD) + test_bit(y, WORD) can't be "reversed".
> > But in that case I don't understand why do we need clear_bit_unlock(), not
> > just clear_bit(), and how "mb is not needeed" could be derived from
> > documentation.
>
> As mentioned above, SLOW_WORK_EXECUTING is a lock against multiple entrance to
> the execution function.
Yes I see.
But why do we need the barrier before clear_bit(SLOW_WORK_EXECUTING) ?
We do have one after test_and_clear_bit(SLOW_WORK_PENDING) above, and it
should be enough, no?
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] slow_work_execute() needs mb() before test_bit(SLOW_WORK_PENDING)
2009-04-13 22:02 ` Oleg Nesterov
@ 2009-04-16 9:43 ` David Howells
2009-04-16 14:15 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: David Howells @ 2009-04-16 9:43 UTC (permalink / raw)
To: Oleg Nesterov
Cc: dhowells, Andrew Morton, David Miller, Serge Hallyn,
Steve Dickson, Trond Myklebust, Al Viro, Daire Byrne,
linux-kernel
Oleg Nesterov <oleg@redhat.com> wrote:
> But why do we need the barrier before clear_bit(SLOW_WORK_EXECUTING) ?
> We do have one after test_and_clear_bit(SLOW_WORK_PENDING) above, and it
> should be enough, no?
No. There lock is covering work->ops->execute(work) too, and that is after
the clearance of SLOW_WORK_PENDING. The UNLOCK-class barrier must go between
execution and clearance of the execution lock bit.
David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] slow_work_execute() needs mb() before test_bit(SLOW_WORK_PENDING)
2009-04-16 9:43 ` David Howells
@ 2009-04-16 14:15 ` Oleg Nesterov
0 siblings, 0 replies; 6+ messages in thread
From: Oleg Nesterov @ 2009-04-16 14:15 UTC (permalink / raw)
To: David Howells
Cc: Andrew Morton, David Miller, Serge Hallyn, Steve Dickson,
Trond Myklebust, Al Viro, Daire Byrne, linux-kernel
On 04/16, David Howells wrote:
>
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > But why do we need the barrier before clear_bit(SLOW_WORK_EXECUTING) ?
> > We do have one after test_and_clear_bit(SLOW_WORK_PENDING) above, and it
> > should be enough, no?
>
> No. There lock is covering work->ops->execute(work) too, and that is after
> the clearance of SLOW_WORK_PENDING. The UNLOCK-class barrier must go between
> execution and clearance of the execution lock bit.
Ah. Now I see.
It is still not clear to me whether we need a barrier after clear_bit_unlock.
Thanks David!
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-04-16 14:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-04-13 20:13 [PATCH] slow_work_execute() needs mb() before test_bit(SLOW_WORK_PENDING) Oleg Nesterov
2009-04-13 20:13 ` Oleg Nesterov
2009-04-13 21:47 ` David Howells
2009-04-13 22:02 ` Oleg Nesterov
2009-04-16 9:43 ` David Howells
2009-04-16 14:15 ` Oleg Nesterov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox