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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 25, 2020 at 04:23:34PM +0100, Jan Beulich wrote:
> 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?

That would be fine. But seeing as we are planning on handling the
result of atomic_read as an unsigned int I'm not sure if it wold be
better to also keep those as unsigned ints.

> > +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 ...

The code uniformly uses uint32_t as the cnts type, I'm fine with
switching to unsigned int, I've used uint32_t to keep it coherent with
the rest of the code.

> > @@ -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>

Thanks, feel free to adjust on commit, or else I can send a new
version.

Roger.

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