Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

> 
> >
> > This patch targets 19.02 release.
> >
> > Introduce rte_rwlock_read_trylock() and rte_rwlock_write_trylock().
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> >  .../common/include/generic/rte_rwlock.h       | 54 +++++++++++++++++++
> >  lib/librte_eal/rte_eal_version.map            |  2 +
> >  2 files changed, 56 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/include/generic/rte_rwlock.h
> > b/lib/librte_eal/common/include/generic/rte_rwlock.h
> > index 5751a0e6d..7e395781e 100644
> > --- a/lib/librte_eal/common/include/generic/rte_rwlock.h
> > +++ b/lib/librte_eal/common/include/generic/rte_rwlock.h
> > @@ -75,6 +75,33 @@ rte_rwlock_read_lock(rte_rwlock_t *rwl)
> >  	}
> >  }
> >
> > +/**
> Please add the experimental API warning
> 
> > + * try to take a read lock.
> > + *
> > + * @param rwl
> > + *   A pointer to a rwlock structure.
> > + * @return
> > + *   - zero if the lock is successfully taken
> > + *   - -EBUSY if lock could not be acquired for reading because a
> > + *     writer holds the lock
> > + */
> > +static inline __rte_experimental int
> > +rte_rwlock_read_trylock(rte_rwlock_t *rwl) {
> > +	int32_t x;
> > +	int success = 0;
> > +
> > +	while (success == 0) {
> > +		x = rwl->cnt;
> > +		/* write lock is held */
> > +		if (x < 0)
> > +			return -EBUSY;
> > +		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl->cnt,
> > +					      (uint32_t)x, (uint32_t)(x + 1));
> > +	}
> > +	return 0;
> > +}
> > +
> >  /**
> >   * Release a read lock.
> >   *
> > @@ -87,6 +114,33 @@ rte_rwlock_read_unlock(rte_rwlock_t *rwl)
> >  	rte_atomic32_dec((rte_atomic32_t *)(intptr_t)&rwl->cnt);  }
> >
> > +/**
> Please add the experimental API warning
> 
> > + * try to take a write lock.
> > + *
> > + * @param rwl
> > + *   A pointer to a rwlock structure.
> > + * @return
> > + *   - zero if the lock is successfully taken
> > + *   - -EBUSY if lock could not be acquired for writing because
> > + *     it was already locked for reading or writing
> > + */
> > +static inline __rte_experimental int
> > +rte_rwlock_write_trylock(rte_rwlock_t *rwl) {
> > +	int32_t x;
> > +	int success = 0;
> > +
> > +	while (success == 0) {
(Apologies for not thinking through all my comments earlier)
I am wondering if the 'while' loop is required for the write lock.

> > +		x = rwl->cnt;
> > +		/* a lock is held */
> > +		if (x != 0)
> > +			return -EBUSY;
> > +		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl->cnt,
> > +					      0, (uint32_t)-1);
This might fail as the lock was taken (either reader or writer). We should return from here as it already indicates that the lock is not available for the writer to take. Otherwise, there is a possibility that the while loop will run for multiple iterations. I would think, from the user's expectation, it might not be correct.

In the read_trylock, the while loop should be fine because the write lock itself is not held. The failure could be because some other reader incremented the counter before we did. i.e. the reader lock itself may be available to take in the next iteration.

> > +	}
> > +	return 0;
> > +}
> > +
> >  /**
> >   * Take a write lock. Loop until the lock is held.
> >   *
> > diff --git a/lib/librte_eal/rte_eal_version.map
> > b/lib/librte_eal/rte_eal_version.map
> > index 3fe78260d..8b1593dd8 100644
> > --- a/lib/librte_eal/rte_eal_version.map
> > +++ b/lib/librte_eal/rte_eal_version.map
> > @@ -355,6 +355,8 @@ EXPERIMENTAL {
> >  	rte_mp_request_async;
> >  	rte_mp_sendmsg;
> >  	rte_option_register;
> > +	rte_rwlock_read_trylock;
> > +	rte_rwlock_write_trylock;
> I do not see the other RW lock APIs in this file.
> 
> >  	rte_service_lcore_attr_get;
> >  	rte_service_lcore_attr_reset_all;
> >  	rte_service_may_be_active;
> > --
> > 2.17.1
> 
> Other than the minor comments,
> Reviewed-by: Honnappa Nagarahalli <Honnappa.nagarahalli@arm.com>