lkml.org 
[lkml]   [2012]   [Jan]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] x86: xen: size struct xen_spinlock to always fit in arch_spinlock_t
On Mon, Jan 23, 2012 at 07:32:25PM +0000, David Vrabel wrote:
> From: David Vrabel <david.vrabel@citrix.com>
>
> If NR_CPUS < 256 then arch_spinlock_t is only 16 bits wide but struct
> xen_spinlock is 32 bits. When a spin lock is contended and
> xl->spinners is modified the two bytes immediately after the spin lock
> would be corrupted.
>
> This is a regression caused by 84eb950db13ca40a0572ce9957e14723500943d6
> (x86, ticketlock: Clean up types and accessors) which reduced the size
> of arch_spinlock_t.
>
> Fix this by making xl->spinners a u8 if NR_CPUS < 256. A
> BUILD_BUG_ON() is also added to check the sizes of the two structures
> are compatible.
>
> In many cases this was not noticable as there would often be padding
> bytes after the lock (e.g., if any of CONFIG_GENERIC_LOCKBREAK,
> CONFIG_DEBUG_SPINLOCK, or CONFIG_DEBUG_LOCK_ALLOC were enabled).
>
> The bnx2 driver is affected. In struct bnx2, phy_lock and
> indirect_lock may have no padding after them. Contention on phy_lock
> would corrupt indirect_lock making it appear locked and the driver
> would deadlock.

Nice find. I think it also affected the ahci driver, and some of the USB
ones - at least those I saw starting to hang with:

[ 79.317407] BUG: soft lockup - CPU#1 stuck for 22s! [modprobe:2244]

Do you have some of those messages by any chance?

Going to test this overnight to see if it fixes the mysterious rash of
32-bit v3.3-rc1 build dying and showing:

[<c1308681>] ? xen_poll_irq_timeout+0x41/0x60
[<c13086ac>] xen_poll_irq+0xc/0x10
[<c104447a>] xen_spin_lock_slow+0xca/0x210
[<c10447bc>] xen_spin_lock+0xec/0x100
[<c154a561>] _raw_spin_lock+0x21/0x30

Thanks!
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> ---
> arch/x86/xen/spinlock.c | 27 ++++++++++++++++++++++-----
> 1 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
> index cc9b1e1..d69cc6c 100644
> --- a/arch/x86/xen/spinlock.c
> +++ b/arch/x86/xen/spinlock.c
> @@ -116,9 +116,26 @@ static inline void spin_time_accum_blocked(u64 start)
> }
> #endif /* CONFIG_XEN_DEBUG_FS */
>
> +/*
> + * Size struct xen_spinlock so it's the same as arch_spinlock_t.
> + */
> +#if NR_CPUS < 256
> +typedef u8 xen_spinners_t;
> +# define inc_spinners(xl) \
> + asm(LOCK_PREFIX " incb %0" : "+m" ((xl)->spinners) : : "memory");
> +# define dec_spinners(xl) \
> + asm(LOCK_PREFIX " decb %0" : "+m" ((xl)->spinners) : : "memory");
> +#else
> +typedef u16 xen_spinners_t;
> +# define inc_spinners(xl) \
> + asm(LOCK_PREFIX " incw %0" : "+m" ((xl)->spinners) : : "memory");
> +# define dec_spinners(xl) \
> + asm(LOCK_PREFIX " decw %0" : "+m" ((xl)->spinners) : : "memory");
> +#endif
> +
> struct xen_spinlock {
> unsigned char lock; /* 0 -> free; 1 -> locked */
> - unsigned short spinners; /* count of waiting cpus */
> + xen_spinners_t spinners; /* count of waiting cpus */
> };
>
> static int xen_spin_is_locked(struct arch_spinlock *lock)
> @@ -164,8 +181,7 @@ static inline struct xen_spinlock *spinning_lock(struct xen_spinlock *xl)
>
> wmb(); /* set lock of interest before count */
>
> - asm(LOCK_PREFIX " incw %0"
> - : "+m" (xl->spinners) : : "memory");
> + inc_spinners(xl);
>
> return prev;
> }
> @@ -176,8 +192,7 @@ static inline struct xen_spinlock *spinning_lock(struct xen_spinlock *xl)
> */
> static inline void unspinning_lock(struct xen_spinlock *xl, struct xen_spinlock *prev)
> {
> - asm(LOCK_PREFIX " decw %0"
> - : "+m" (xl->spinners) : : "memory");
> + dec_spinners(xl);
> wmb(); /* decrement count before restoring lock */
> __this_cpu_write(lock_spinners, prev);
> }
> @@ -373,6 +388,8 @@ void xen_uninit_lock_cpu(int cpu)
>
> void __init xen_init_spinlocks(void)
> {
> + BUILD_BUG_ON(sizeof(struct xen_spinlock) > sizeof(arch_spinlock_t));
> +
> pv_lock_ops.spin_is_locked = xen_spin_is_locked;
> pv_lock_ops.spin_is_contended = xen_spin_is_contended;
> pv_lock_ops.spin_lock = xen_spin_lock;
> --
> 1.7.2.5


\
 
 \ /
  Last update: 2012-01-23 20:55    [W:0.063 / U:1.256 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site