On 24.02.2022 11:54, Juergen Gross wrote:
> --- a/xen/arch/x86/mm/mm-locks.h
> +++ b/xen/arch/x86/mm/mm-locks.h
> @@ -42,7 +42,7 @@ static inline void mm_lock_init(mm_lock_t *l)
>
> static inline bool mm_locked_by_me(const mm_lock_t *l)
> {
> - return (l->lock.recurse_cpu == current->processor);
> + return (l->lock.data.cpu == current->processor);
> }
I see a fair risk with this: Behavior will now differ between debug and
non-debug builds. E.g. a livelock because of trying to acquire the same
lock again would not be noticed in a debug build if the acquire is
conditional upon this function's return value. I think this is the main
reason behind having two separate field, despite the apparent redundancy.
Nevertheless a few more comments in case I'm missing something.
> @@ -81,19 +79,19 @@ static void check_barrier(spinlock_t *lock)
> * However, if we spin on an IRQ-unsafe lock with IRQs disabled then that
> * is clearly wrong, for the same reason outlined in check_lock() above.
> */
> - BUG_ON(!local_irq_is_enabled() && !lock->debug.irq_safe);
> + BUG_ON(!local_irq_is_enabled() && !lock->data.irq_safe);
> }
>
> static void got_lock(spinlock_t *lock)
> {
> - lock->debug.cpu = smp_processor_id();
> + lock->data.cpu = smp_processor_id();
This assignment breaks ...
> @@ -230,9 +228,9 @@ int _spin_is_locked(spinlock_t *lock)
> * "false" here, making this function suitable only for use in
> * ASSERT()s and alike.
> */
> - return lock->recurse_cpu == SPINLOCK_NO_CPU
> + return lock->data.cpu == SPINLOCK_NO_CPU
... the use of this field to distinguish recursively locked locks
from "simple" ones. At the very least the comment needs updating,
but ...
> ? lock->tickets.head != lock->tickets.tail
... in how far this is still a sensible check in debug builds is
also questionable. The effect here certainly also deserves pointing
out in the description.
> - : lock->recurse_cpu == smp_processor_id();
> + : lock->data.cpu == smp_processor_id();
> }
>
> int _spin_trylock(spinlock_t *lock)
> @@ -296,22 +294,24 @@ int _spin_trylock_recursive(spinlock_t *lock)
> {
> unsigned int cpu = smp_processor_id();
>
> - /* Don't allow overflow of recurse_cpu field. */
> + /* Don't allow overflow of cpu field. */
> BUILD_BUG_ON(NR_CPUS > SPINLOCK_NO_CPU);
> BUILD_BUG_ON(SPINLOCK_RECURSE_BITS < 3);
>
> check_lock(lock, true);
>
> - if ( likely(lock->recurse_cpu != cpu) )
> + if ( likely(lock->data.cpu != cpu) )
> {
> if ( !spin_trylock(lock) )
> return 0;
> - lock->recurse_cpu = cpu;
> +#ifndef CONFIG_DEBUG_LOCKS
> + lock->data.cpu = cpu;
> +#endif
Maybe worth an ASSERT() in the #else case (and elsewhere as applicable)?
> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -6,26 +6,34 @@
> #include <asm/spinlock.h>
> #include <asm/types.h>
>
> -#define SPINLOCK_CPU_BITS 12
> +#define SPINLOCK_CPU_BITS 12
> +#define SPINLOCK_NO_CPU ((1u << SPINLOCK_CPU_BITS) - 1)
> +#define SPINLOCK_RECURSE_BITS (16 - SPINLOCK_CPU_BITS)
> +#define SPINLOCK_MAX_RECURSE ((1u << SPINLOCK_RECURSE_BITS) - 1)
> +#define SPINLOCK_PAD_BITS (30 - SPINLOCK_CPU_BITS -
> SPINLOCK_RECURSE_BITS)
>
> -#ifdef CONFIG_DEBUG_LOCKS
> -union lock_debug {
> - uint16_t val;
> -#define LOCK_DEBUG_INITVAL 0xffff
> +typedef union {
> + u32 val;
> struct {
> - uint16_t cpu:SPINLOCK_CPU_BITS;
> -#define LOCK_DEBUG_PAD_BITS (14 - SPINLOCK_CPU_BITS)
> - uint16_t :LOCK_DEBUG_PAD_BITS;
> + u32 cpu:SPINLOCK_CPU_BITS;
> + u32 recurse_cnt:SPINLOCK_RECURSE_BITS;
> + u32 pad:SPINLOCK_PAD_BITS;
> +#ifdef CONFIG_DEBUG_LOCKS
> bool irq_safe:1;
> bool unseen:1;
> +#define SPINLOCK_DEBUG_INITVAL 0xc0000000
> +#else
> + u32 debug_pad:2;
Prior to your change we had two well-formed uint16_t. You replace them by
five new instances of the being-phased-out u32?
Also - do the two padding fields really need names?
Jan
