On 12/02/19(Tue) 23:25, Paul Irofti wrote:
> On Wed, Jan 30, 2019 at 12:29:20PM -0200, Martin Pieuchot wrote:
> > On 28/01/19(Mon) 14:57, Martin Pieuchot wrote:
> > > Summer is really warm here.  No need to make my machines hotter by
> > > spinning a lot.  So here's a futex(2) based pthread_rwlock*
> > > implementation.  I should look familiar to those of you that reviewed
> > > the mutex & semaphore implementations.  It uses amotic cas & inc/dec.
> > > 
> > > I moved the existing implementation to rthread_rwlock_compat.c to match
> > > what has been done for semaphores.
> > > 
> > > Tests, benchmarks and reviews are more than welcome!
> > 
> > Newer version with fewer atomics and some barrier corrections as pointed
> > out by visa@.
> 
> Appologies again for the late review.
> 
> I sprinkled various comments inline. The implementation is nice, clean
> and easy to read! Except, perhaps, for the COUNT macro.
> 
> So I am generally OK with this diff. One question though, is the manual
> page statement about preventing writer starvation still true with your
> version? I don't think so. Or perhaps I am missing the bit where a
> reader checks if a writer is waiting for the lock...

I'll fix the manpages :)

> > @@ -52,26 +59,25 @@ DEF_STD(pthread_rwlock_init);
> >  int
> >  pthread_rwlock_destroy(pthread_rwlock_t *lockp)
> >  {
> > -   pthread_rwlock_t lock;
> > +   pthread_rwlock_t rwlock;
> >  
> > -   assert(lockp);
> > -   lock = *lockp;
> > -   if (lock) {
> > -           if (lock->readers || !TAILQ_EMPTY(&lock->writers)) {
> > +   rwlock = *lockp;
> > +   if (rwlock) {
> > +           if (rwlock->value != UNLOCKED) {
> >  #define MSG "pthread_rwlock_destroy on rwlock with waiters!\n"
> >                     write(2, MSG, sizeof(MSG) - 1);
> >  #undef MSG
> 
> Is there no better way than a macro?

Feel free to improve it.  Note that this idiom is used in all rthread
files.

> >                     return (EBUSY);
> >             }
> > -           free(lock);
> > +           free((void *)rwlock);
> 
> No need to cast.

Until we move it to libc/thread.  This is done for coherency with
rthread_mutex.c from which this implementation is based :o)

> 
> > +           *lockp = NULL;
> >     }
> > -   *lockp = NULL;
> >  
> >     return (0);
> >  }
> >  
> >  static int
> > -_rthread_rwlock_ensure_init(pthread_rwlock_t *lockp)
> > +_rthread_rwlock_ensure_init(pthread_rwlock_t *rwlockp)
> >  {
> >     int ret = 0;
> 
> I really like this functionality. Is this mandated by POSIX? Can we add
> it to other locking or pthread primitives?

It's already present where needed, it was already there (:

> >  {
> > -   pthread_rwlock_t lock;
> > -   pthread_t thread = pthread_self();
> > -   int error;
> > +   pthread_t self = pthread_self();
> > +   pthread_rwlock_t rwlock;
> > +   unsigned int val, new;
> > +   int i, error;
> 
> You need to check the timespec here

This is checked by _twait() like for mutexes. 

> > +   error = _rthread_rwlock_tryrdlock(rwlock);
> > +   if (error != EBUSY || trywait)
> > +           return (error);
> > +
> > +   /* Try hard to not enter the kernel. */
> > +   for (i = 0; i < SPIN_COUNT; i ++) {
>                                      ^ style

Indeed (bad copy paste)!

> > +           val = rwlock->value;
> > +           if (val == UNLOCKED || (val & WAITING))
> > +                   break;
> > +
> > +           SPIN_WAIT();
> > +   }
> > +
> > +   while ((error = _rthread_rwlock_tryrdlock(rwlock)) == EBUSY) {
> > +           val = rwlock->value;
> > +           if (val == UNLOCKED || (COUNT(val)) != WRITER)
> > +                   continue;
> > +           new = val | WAITING;
> > +           if (atomic_cas_uint(&rwlock->value, val, new) == val) {
> 
> Don't you need a membar_after_atomic() here?

Why?  The lock hasn't been acquired here.

> 
> > +                   error = _twait(&rwlock->value, new, CLOCK_REALTIME,
> > +                       abs);
> > +           }
> > +           if (error == ETIMEDOUT)
> > +                   break;
> >     }
> > -   _spinunlock(&lock->lock);
> >  
> >     return (error);
> > +
> >  int
> > -pthread_rwlock_unlock(pthread_rwlock_t *lockp)
> > +pthread_rwlock_unlock(pthread_rwlock_t *rwlockp)
> >  {
> > +   pthread_t self = pthread_self();
> > +   pthread_rwlock_t rwlock;
> > +   unsigned int val, new;
> > +
> > +   rwlock = *rwlockp;
> > +   _rthread_debug(5, "%p: rwlock_unlock %p\n", self, (void *)rwlock);
> > +
> > +   membar_exit_before_atomic();
> 
> Wouldn't this membar need to be inside the loop? Or perhaps a
> corresponding membar_enter() after exiting the loop?

Why?

The membar is here to enforce that writes done during the critical
section are visible before the lock is released.  Such that another
thread wont grab the lock and see outdated data inside the critical
section.

Reply via email to