Re: [Xen-devel] [PATCH v3 2/2] rwlock: allow recursive read locking when already locked in write mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.02.2020 09:44, Roger Pau Monne wrote:
> @@ -20,21 +21,30 @@ typedef struct {
>  #define DEFINE_RWLOCK(l) rwlock_t l = RW_LOCK_UNLOCKED
>  #define rwlock_init(l) (*(l) = (rwlock_t)RW_LOCK_UNLOCKED)
>  
> -/*
> - * Writer states & reader shift and bias.
> - *
> - * Writer field is 8 bit to allow for potential optimisation, see
> - * _write_unlock().
> - */
> -#define    _QW_WAITING  1               /* A writer is waiting     */
> -#define    _QW_LOCKED   0xff            /* A writer holds the lock */
> -#define    _QW_WMASK    0xff            /* Writer mask.*/
> -#define    _QR_SHIFT    8               /* Reader count shift      */
> +/* Writer states & reader shift and bias. */
> +#define    _QW_CPUMASK  0xfff               /* Writer CPU mask */
> +#define    _QW_SHIFT    12                  /* Writer flags shift */
> +#define    _QW_WAITING  (1u << _QW_SHIFT)   /* A writer is waiting */
> +#define    _QW_LOCKED   (3u << _QW_SHIFT)   /* A writer holds the lock */
> +#define    _QW_WMASK    (3u << _QW_SHIFT)   /* Writer mask */
> +#define    _QR_SHIFT    14                  /* Reader count shift */

In particular with the suggested change of atomic_and()'s first
parameter's type, perhaps the u suffixes want dropping here?

> +static inline bool _is_write_locked_by_me(uint32_t cnts)

Both here and ...

> +{
> +    BUILD_BUG_ON(_QW_CPUMASK < NR_CPUS);
> +    return (cnts & _QW_WMASK) == _QW_LOCKED &&
> +           (cnts & _QW_CPUMASK) == smp_processor_id();
> +}
> +
> +static inline bool _can_read_lock(uint32_t cnts)

... here, is a fixed width type really needed? I'd prefer if
"unsigned int" was used, and if eventually we'd then also
replace ...

> @@ -45,10 +55,10 @@ static inline int _read_trylock(rwlock_t *lock)
>      u32 cnts;

... this and ...

> @@ -64,7 +74,7 @@ static inline void _read_lock(rwlock_t *lock)
>      u32 cnts;

... this (and possible others).

> @@ -115,6 +125,11 @@ static inline int _rw_is_locked(rwlock_t *lock)
>      return atomic_read(&lock->cnts);
>  }
>  
> +static inline uint32_t _write_lock_val(void)

Same here then.

With these taken care of (as long as you agree, and likely doable
again while committing)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel