> > > > new file mode 100644
> > > > index 00000000000..e5c8015d27c
> > > > --- /dev/null
> > > > +++ lib/librthread/rthread_sem_atomic.c
> > >
> > > I'm not fan of the _atomic suffix. What about rthread_semaphore.c?
> >
> > I am not set on the name, but I do think rthread_semaphore is not a good
> > option as it only adds confusion. Why is one _sem and another
> > _semaphore? There is no info in that naming scheme.
>
> I'm hoping that in the long run we get rid of all __thrsleep(2) based
> functions. So I don't see a point of naming the file based on which
> underlying syscall it uses.
>
> Maybe we should take the other approach and copy the existing
> implementation into a rthread_sem_compat.c and use rthread_sem.c for
> your new implementation.
That sounds a lot better to me. I will include this renaming in the
final diff as otherwise it gets harder to read.
> > > > + 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;
> > > > +
> > > > + atomic_inc_int(&sem->waitcount);
> > >
> > > Can you keep the destroy check without adding two atomic operations here?
> >
> > I don't understand what you mean here. What destroy check? How can I get
> > rid of the atomic inc/dec? Should I do the operations without atomics?
> > Is that what you mean?
>
> 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.
I do not understand how the current implementation is racy.
Let's say we have two threads, T0 and T1, and that waitcount equals 1.
T0 just came back from _twait() and is about to do a decrement of
waitcount. At the same time, T1 is about to call sem_destroy() on the
same semaphore.
If T0 decrements first, T1 succedes and all is well.
If T0 decrements last, T1 fails with EBUSY and the caller has to retry.
So again, if I am not mistaken all is well.
There might be a "race" between incrementing and destroying a semapahore
with 0 waitcount. But I don't think we have any control over that. With
atomics or without.
What my diff adds from the compat implementation is atomic increments
and decrements of waitcount. I did this in order to ensure that, for
example, two threads on two CPUs do not increment the counter at the
same time. And I think it is better like this than with the compat
implemnetation.
>
> > > > + }
> > > > + sem->lock = _SPINLOCK_UNLOCKED;
> > >
> > > `sem->lock' is no longer unused and can be #ifdef out.
> >
> > Good catch!
>
> But you forgot the thread_private.h diff in your new version :)
>
> > > > + //membar_exit_before_atomic();
> > > > + //*sval = atomic_add_int_nv(&sem->value, 0);
> > >
> > > What does that mean?
> >
> > I wasn't sure that I can do just a read of the semaphore value (without
> > locking)
> > so I left the potential locking code commented out in case the review
> > process will point out that it is indeed needed.
>
> 'struct pthread_mutex' has its `lock' member as first argument to help
> thinking about alignment for this reason. I'd suggest you do the same
> ;)
I will remove that member when rthread_sem_compat.c will also be
removed from the tree. Otherwise I will need to create an ifdef maze.
