Re: Race conditions galore (2.0.33 and possibly 2.1.x)

Linus Torvalds (
Mon, 22 Dec 1997 09:50:54 -0800 (PST)

On Mon, 22 Dec 1997, Stephen R. van den Berg wrote:
> Well, it *does* fix some hung processes on a newsserver of mine.
> First, several processes hung in buffer.c (very reproducable).
> I patched it (using the described patch).
> Now, a process hung in filemap.c (same construct). So I patched that
> as well.

Ok, something is definitely going on here. But as you don't even have an
SMP setup, I don't see exactly what..

> This happened within 15 minutes uptime, almost every time.
> The machine has been running for 17 hours now, no hangs anymore.
> I'm running with ECC, and the kernel did and does *not* generate any warnings
> or errors. So hardware failures are very unlikely.

Your patch looked ok - there was nothing wrong with it. It just shouldn't
really have any effect, because the old code should be equally stable.

Was that the _only_ change you did?

> >Anyway, the basic race-free wait loop looks like this (there are
> > add_wait_queue(..);
> > repeat:
> In buffer.c it runs some queues at this point. Can these affect the
> state of the should_wait_condition?

Yes, but the effect should in the end be the same..

> E.g. it unlocks the buffer, sets the current state to TASK_RUNNING
> (redundant), then locks the buffer again. What happens to the wait-queue?
> Will it still wake up the process even though it has been unlocked, then
> locked again?

The wait queues will be unaffected - they are only affected by the
"add_qait_queue()"/"remove_wait_queue()" thing (ie the act of waking
somebody up does _not_ remove the sleeper from a wait queue).

> > current->state = TASK_UNINTERRUPTIBLE;
> > if (should_wait_condition) {
> What if the should_wait_condition changes here?
> I.e. the buffer is unlocked. Will the current->state be affected?
> Or will wake_up process avoid touching the current->state because the
> current process is "still running"?

The wake_up() will indeed change current->state - even if the process is
really running (it doesn't care, and that's the whole reason why the
construct works).

I wonder whether gcc might be changing the ordering here. That would
explain why your patch would make a difference..

In fact, the more I think about it, the more your patch makes sense
considering what gcc might do to the sources. Do you by any chance use
egcs or a recent snapshot of gcc?

> >There are only two important rules:
> > - you have to add yourself to the wait queue _before_ testing for the
> > condition.
> > - you have to mark yourself asleep _before_ testing for the condition.
> Maybe we need to change the last rule?
> - You have to mark yourself asleep _before_ adding yourself to the waitqueue.

That would be a reasonably ok rule, but in general there are cases where
you want to add yourself to the wait-queue just once (outside a larger
loop), and mark yourself sleeping inside the loop.

IF this is a compiler optimization thing, could you test the original code
but just add a "mb()" to after marking yourself as sleeping. Ie

current->state = TASK_UNINTERRUPTIBLE;
+ mb();
if (condition..)

The mb() should make sure that gcc cannot move things around, and on some
architectures it will also do some other coherency stuff for SMP. If that
also fixes it I'll have to think about this some more..

(Your patch essentially did a similar thing to the "mb()" - the wait queue
manipulation will also act as a barrier for gcc optimizations. So that
might explain the thing).

> No, actually. Unless my system has a cache-coherency problem somewhere;
> but it's not an SMP system by any stretch of the imagination.

Any coherency issue that would make the code not work on UP has to be
serious enough that _nothing_ would work. So I would want a bit more
information about this first - if it isn't a compiler thing (not a
compiler bug - looking at the thing the compiler might quite reasonably do
some things to the code that would defeat the ordering) I want to
understand exactly _what_ it is..

Anyway, thanks for testing, this looks like a potentially very interesting
problem that needs to be solved..