On Wed, May 09, 2018 at 11:33:06AM +0200, Martin Pieuchot wrote:
> On 09/05/18(Wed) 11:43, Paul Irofti wrote:
> > > [...]
> > > I'm saying that increasing/decreasing `waitcount' now serves a single
> > > purpose: the check in sem_destroy(). However with your implementation
> > > this check is racy and adds two atomic operation on top of every
> > > syscall. So my question is could you rewrite the check in sem_destroy()
> > > differently?
> >
> > waitcount was used for the same purpose in the old (compat)
> > implementation. The ident bit did not matter all that much as any other
> > sem field could have been used instead.
>
> In the old implementation `waitcount' is used to let decide if sem_post()
> needs to do a wakeup. In that implementation the field is protected by a
> spinlock().
>
> > I do not understand how the current implementation is racy.
>
> If a thread is in the while() loop of _sem_wait() it is waiting, but you
> do not increment `waitcount'.
Because it is not actually waiting to be woken up. It is waiting to
decrement sem->value. Waiters lists the number of threads that are
blocked on the semaphore because sem->value is zero.
So there are no waiters at that point.
> Then the writer uses atomic operations to increment/decrement `waitcount'
> but is it enough to have the reader, see the updated value?
On x86 I think it is guaranteed. Not sure about others or if we need
membars here too. Visa said membars are not needed in this case for
mips.
> What if the
> check is done before the atomic operation has been performed?
Then we should probably wrap the for (;;;) loop in atomic increments and
decrements.
atomic_inc_int(&sem->waitcount);
for (;;) {
while ((v = sem->value) > 0) {
ov = atomic_cas_uint(&sem->value, v, v - 1);
if (ov == v) {
membar_enter_after_atomic();
return 0;
}
}
if (r)
break;
r = _twait(&sem->value, 0, CLOCK_REALTIME, abstime);
/* ignore interruptions other than cancelation */
if ((r == ECANCELED && *delayed_cancel == 0) ||
(r == EINTR && !can_eintr) || r == EAGAIN)
r = 0;
}
atomic_dec_int(&sem->waitcount);
> Worst
> what if the cache of the reader doesn't contain the last value?
The way to be absolutely sure is to also do an atomic operation when
reading the waitcount in sem_destroy.
If we decide to be that paranoid, we should do the same for
sem_getvalue() :)
> What I'm just saying is that, in my opinion, adding 2 atomic operations
> on top of the syscall is not worth it.
This might be so performance wise. But I am not sure about correctness.
> Why not check `sem->value' instead?
Because it can happen that sem->value is zero and no threads are
stuck in sem_wait() which means nobody is using the semaphore.
So I should be able to destroy it.
