[1/2] rwlock: introduce 'try' semantics for RD and WR locking
Checks
Commit Message
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(+)
Comments
>
> 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) {
> + 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);
> + }
> + 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>
> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Wednesday, December 19, 2018 2:38 PM
> To: Konstantin Ananyev <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>; nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD
> and WR locking
>
> >
> > 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) {
> > + 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);
> > + }
> > + 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>
Same comments as Honnappa, experimental warning is required in new code.
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
On 2018-12-19 07:37, Honnappa Nagarahalli wrote:
>> 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.
>
They, just like those added, are static inline functions, will not
result in any symbols in any shared objects and thus shouldn't be in any
version.map file.
> -----Original Message-----
> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> Sent: Wednesday, December 19, 2018 10:28 AM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> Subject: Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
>
> On 2018-12-19 07:37, Honnappa Nagarahalli wrote:
> >> 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.
> >
>
> They, just like those added, are static inline functions, will not
> result in any symbols in any shared objects and thus shouldn't be in any
> version.map file.
So, are you guys trying to say that there is no need to put these 'trylock'
functions into 'experimental' section?
Konstantin
On Wed, Dec 19, 2018 at 10:56:09AM +0000, Ananyev, Konstantin wrote:
>
>
> > -----Original Message-----
> > From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > Sent: Wednesday, December 19, 2018 10:28 AM
> > To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> > Cc: nd <nd@arm.com>; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
> >
> > On 2018-12-19 07:37, Honnappa Nagarahalli wrote:
> > >> 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.
> > >
> >
> > They, just like those added, are static inline functions, will not
> > result in any symbols in any shared objects and thus shouldn't be in any
> > version.map file.
>
> So, are you guys trying to say that there is no need to put these 'trylock'
> functions into 'experimental' section?
> Konstantin
>
Anything static inline doesn't have a mapfile entry, so there is no issue
there. However, the APIs themselves, since they are new should probably
have the __rte_experimental attribute marked on them in the code.
/Bruce
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Wednesday, December 19, 2018 11:00 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Mattias Rönnblom <hofors@lysator.liu.se>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; dev@dpdk.org; nd
> <nd@arm.com>; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> Subject: Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
>
> On Wed, Dec 19, 2018 at 10:56:09AM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > > Sent: Wednesday, December 19, 2018 10:28 AM
> > > To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> > > Cc: nd <nd@arm.com>; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> > > Subject: Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
> > >
> > > On 2018-12-19 07:37, Honnappa Nagarahalli wrote:
> > > >> 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.
> > > >
> > >
> > > They, just like those added, are static inline functions, will not
> > > result in any symbols in any shared objects and thus shouldn't be in any
> > > version.map file.
> >
> > So, are you guys trying to say that there is no need to put these 'trylock'
> > functions into 'experimental' section?
> > Konstantin
> >
> Anything static inline doesn't have a mapfile entry, so there is no issue
> there.
Ok, if these map entries are not needed, I'll remove them in v2.
> However, the APIs themselves, since they are new should probably
> have the __rte_experimental attribute marked on them in the code.
They do have __rte_experimental attribute in the .h file.
Konstantin
>
> >
> > 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>
> > > + * 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.
Good point - it will also save us extra read in case of failure.
Will change in v2.
Konstantin
>
> 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;
> > > +}
> > > +
>
@@ -75,6 +75,33 @@ rte_rwlock_read_lock(rte_rwlock_t *rwl)
}
}
+/**
+ * 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);
}
+/**
+ * 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) {
+ 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);
+ }
+ return 0;
+}
+
/**
* Take a write lock. Loop until the lock is held.
*
@@ -355,6 +355,8 @@ EXPERIMENTAL {
rte_mp_request_async;
rte_mp_sendmsg;
rte_option_register;
+ rte_rwlock_read_trylock;
+ rte_rwlock_write_trylock;
rte_service_lcore_attr_get;
rte_service_lcore_attr_reset_all;
rte_service_may_be_active;