Messages in this thread Patch in this message | | | From | Rusty Russell <> | Subject | Re: [PATCH] Futexes IV (Fast Lightweight Userspace Semaphores) | Date | Wed, 20 Mar 2002 17:45:02 +1100 |
| |
In message <3C96F81F.1020608@dlr.de> you write: > Rusty Russell wrote: > > > > 1) Where this is flawed, > > > I. There is a race in __pthread_cond_wait between timeout and a > cond_signal or broadcast. If the signal comes in > > } while (ret < 0 && errno == EINTR); > >>>>> we leave with errno==ETIMEDOUT and get signal or broadcast called > here > if (atomic_dec_and_test(cond->num_waiting)) > > then you up cond->wait one time to often, leaving it in an invalid state.
Hmmm... this is true.
> II. Your implementation relies on the fact that the signal or broadcast > caller owns the mutex used in cond_wait. According to the POSIX spec > this need not be the case. The only thing that may happen is that you > miss a wakeup. But it is not allowed to screw up the internal state of > of the condition variable, which might well happen in your > implementation. (Note: Calling cond_signal without holding the mutex is > not necessarily flawed software. Think of a periodically occurring > new_data or data_changed flag where it is not really important to sleep > race free)
I hadn't appreciated this. That makes it harder. I think I have to abandon the atomics and use a mutex inside the condition variable.
> III. Minor nit: You should also clear cond->ack.count > in cond_signal otherwise it may wrap around soon (at least for a > 24-bit atomic variable) if you mostly use cond_signal.
Yep.
> > 2) Where this is suboptimal, > > > As said in a previous e-mail, you need an futex_up(..,n) that > really wakes_up n thread at once.
OK, we could read the value in the kernel's up() and wake that many.
> > 3) What kernel primitive would help to resolve these? > > Your exported waitqueues or my suggestion for a second waitqueue > associated with a futex.
Any chance of a rough patch (to the code below, at least)?
Thanks! Rusty. -- Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
--- non-pthreads.c.19-March-2002 Wed Mar 20 17:37:17 2002 +++ non-pthreads.c Wed Mar 20 17:43:42 2002 @@ -11,6 +11,7 @@ typedef struct { + struct futex lock; int num_waiting; struct futex wait, ack; } pthread_cond_t; @@ -48,23 +49,29 @@ int pthread_cond_signal(pthread_cond_t *cond) { + futex_down(&cond->lock); + /* Reset this so it doesn't overflow */ + cond->ack.count = 0; if (cond->num_waiters) return futex_up(&cond->futex, 1); + futex_up(&cond->lock, 1); return 0; } int pthread_cond_broadcast(pthread_cond_t *cond) { - unsigned int waiters = cond->num_waiting; - - if (waiters) { - /* Re-initialize ACK. Could have been upped by - pthread_cond_signal and pthread_cond_wait. */ + futex_down(&cond->lock); + if (cond->num_waiting) { cond->ack.count = 0; + /* Release the waiters. */ futex_up(&cond->futex, waiters); /* Wait for ack before returning. */ futex_down(&cond->ack); + /* Reset wait, in case someone who was waiting timed + out and didn't decrement. */ + cond->wait.count = 0; } + futex_up(&cond->lock); return 0; } @@ -75,8 +82,10 @@ int ret; /* Increment first so broadcaster knows we are waiting. */ + futex_down(&cond->lock); atomic_inc(cond->num_waiting); futex_up(&mutex, 1); + futex_up(&cond->lock, 1); do { ret = futex_down_time(&cond, reltime); } while (ret < 0 && errno == EINTR); - 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/
| |