[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


  • To: Juergen Gross <jgross@xxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 24 Feb 2022 17:11:05 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=DbGOzmSn+1hs/xlf3H8j1wvPbL6MWHAaVHEbBRxUdn0=; b=R2Q325Ii6lpCXnGWZITP1NPgpoY9TdSV7KPA5Zi1qPkNx7lb4AjmjdMSGrUlGoVcENVVjx/J5U5vTx5kjng2Yiap9ek/DkelvpP2UF9CtRqGckUztGOxB9csbdhTeHAgVTgrfw6g2OMs3ERdwRdyIBn3dHyPh7skthRJpnaH5ZwuDC/3zrGH5JK5/z8jnTx2NfZxU44kXCFjrI3a2FZAQSjILXT3e7jOGfx502n+dlHki2oPw5hA/PCMWw23SynJlDdz4q2XV9+P2Zlhtgf47VARu2Mqe/StoBbnjqFCWgwilE1mIwHAlUh9U0XM3BRZycpATc3D7erHzDx5KrwnUg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=gklaYfKXougR3U17BnOJl5tQo04d8wp1n1W0UD8OxYUlI5KF1hUKgtxY/RI6jxmBvT/JvWlJ+jpjiA5LtYG264541Pz8aUVEcVI2GrOUG0msUAvt7bh+YOaGdnt7hu0yIILU7IIKsh7ZioBR7+pl3uN/lVoPgym9FDmJTuOUxHHsnqYrdVLAqJmDuAyHNaT3dSGkJfRY33V2TAYfWDlkYYkOnbkkrhVRHoiXVVZY54E5mns+HvoLOOL0zrfg6zTrfkUgLQRj/8WH0fwb1rim25HWj+R+6kYWIkHqJBkCAS03YpO0jLWlExRhvYlZ2388S73k9UkrXHHNKVppSyaBaQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 24 Feb 2022 16:11:21 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.