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 03:23:38PM +0100, Jürgen Groß wrote:
> On 20.02.20 15:11, Roger Pau Monné wrote:
> > 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.
> 
> In case you make _QR_SHIFT 16 it is possible to just write a 2-byte zero
> value for write_unlock() (like its possible to do so today using a
> single byte write).

That would limit the readers count to 65536, what do you think Jan?

Thanks, Roger.

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