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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 20/02/2020 17:31, Roger Pau Monne wrote:
> Allow a CPU already holding the lock in write mode to also lock it in
> read mode. There's no harm in allowing read locking a rwlock that's
> already owned by the caller (ie: CPU) in write mode. Allowing such
> accesses is required at least for the CPU maps use-case.
>
> In order to do this reserve 14bits of the lock, this allows to support
> up to 16384 CPUs. Also reduce the write lock mask to 2 bits: one to
> signal there are pending writers waiting on the lock and the other to
> signal the lock is owned in write mode. Note the write related data
> is using 16bits, this is done in order to be able to clear it (and
> thus release the lock) using a 16bit atomic write.
>
> This reduces the maximum number of concurrent readers from 16777216 to
> 65536, I think this should still be enough, or else the lock field
> can be expanded from 32 to 64bits if all architectures support atomic
> operations on 64bit integers.
FWIW, arm32 is able to support atomic operations on 64-bit integers.

>   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);
> +    /* Since the writer field is atomic, it can be cleared directly. */
> +    ASSERT(_is_write_locked_by_me(atomic_read(&lock->cnts)));
> +    BUILD_BUG_ON(_QR_SHIFT != 16);
> +    write_atomic((uint16_t *)&lock->cnts, 0);
I think this is an abuse to cast an atomic_t() directly into a uint16_t. 
You would at least want to use &lock->cnts.counter here.
The rest of the code looks good to me.

Cheers,

--
Julien Grall

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