[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] xen/spinlock: merge recurse_cpu and debug.cpu fields in struct spinlock
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |