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

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

On Thu, Feb 20, 2020 at 01:48:54PM +0100, Jan Beulich wrote:
> On 20.02.2020 13:02, Roger Pau Monne wrote:
> > I've done some testing and at least the CPU down case is fixed now.
> > Posting early in order to get feedback on the approach taken.
> 
> Looks good, thanks, just a question and two comments:
> 
> > --- a/xen/include/xen/rwlock.h
> > +++ b/xen/include/xen/rwlock.h
> > @@ -20,21 +20,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_WAITING  1                       /* A writer is waiting */
> > +#define    _QW_LOCKED   3                       /* A writer holds the lock 
> > */
> 
> Aiui things would work equally well if 2 was used here?

I think so, I left it as 3 because previously LOCKED would also
include WAITING, and I didn't want to change it in case I've missed
some code path that was relying on that.

> 
> > +#define    _QW_WMASK    3                       /* Writer mask */
> > +#define    _QW_CPUSHIFT 2                       /* Writer CPU shift */
> > +#define    _QW_CPUMASK  0x3ffc                  /* Writer CPU mask */
> 
> At least on x86, the shift involved here is quite certainly
> more expensive than using wider immediates on AND and CMP
> resulting from the _QW_MASK-based comparisons. I'd therefore
> like to suggest to put the CPU in the low 12 bits.

Hm right. The LOCKED and WAITING bits don't need shifting anyway.

> 
> Another option is to use the recurse_cpu field of the
> associated spin lock: The field is used for recursive locks
> only, and hence the only conflict would be with
> _spin_is_locked(), which we don't (and in the future then
> also shouldn't) use on this lock.

I looked into that also, but things get more complicated AFAICT, as it's
not possible to atomically fetch the state of the lock and the owner
CPU at the same time. Neither you could set the LOCKED bit and the CPU
at the same time.

> 
> > @@ -166,7 +180,8 @@ static inline void _write_unlock(rwlock_t *lock)
> >       * If the writer field is atomic, it can be cleared directly.
> >       * Otherwise, an atomic subtraction will be used to clear it.
> >       */
> > -    atomic_sub(_QW_LOCKED, &lock->cnts);
> > +    ASSERT(_is_write_locked_by_me(atomic_read(&lock->cnts)));
> > +    atomic_sub(_write_lock_val(), &lock->cnts);
> 
> I think this would be more efficient with atomic_and(), not
> the least because of the then avoided smp_processor_id().
> Whether to mask off just _QW_WMASK or also the CPU number of
> the last write owner would need to be determined. But with
> using subtraction, in case of problems it'll likely be
> harder to understand what actually went on, from looking at
> the resulting state of the lock (this is in part a pre-
> existing problem, but gets worse with subtraction of CPU
> numbers).

Right, a mask would be better. Right now both need to be cleared (the
LOCKED and the CPU fields) as there's code that relies on !lock->cnts
as a way to determine that the lock is not read or write locked. If we
left the CPU lying around those checks would need to be adjusted.

Thanks, Roger.

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