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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

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

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.

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

Jan

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