Re: [Xen-devel] [PATCH 1/4] xen/spinlocks: in debug builds store cpu holding the lock

[Jan Beulich <JBeulich@xxxxxxxx>]

On 07.08.2019 16:31, Juergen Gross wrote:
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -13,9 +13,9 @@
>   
>   static atomic_t spin_debug __read_mostly = ATOMIC_INIT(0);
>   
> -static void check_lock(struct lock_debug *debug)
> +static void check_lock(union lock_debug *debug)
>   {
> -    int irq_safe = !local_irq_is_enabled();
> +    unsigned short irq_safe = !local_irq_is_enabled();

bool? Seeing your heavy use of "unsigned short" I realize that
my CodingStyle change committed yesterday still wasn't precise
enough.

> @@ -43,18 +43,21 @@ static void check_lock(struct lock_debug *debug)
>      */
>      if ( unlikely(debug->irq_safe != irq_safe) )
>      {
> -        int seen = cmpxchg(&debug->irq_safe, -1, irq_safe);
> +        union lock_debug seen, new = { 0 };
>   
> -        if ( seen == !irq_safe )
> +        new.irq_safe = irq_safe;
> +        seen.val = cmpxchg(&debug->val, 0xffff, new.val);

This 0xffff should be connected (by way of at least a #define) to
the one used in _LOCK_DEBUG.

> +        if ( !seen.unused && seen.irq_safe == !irq_safe )

While "unused" makes sufficient sense here, ...

> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -7,14 +7,20 @@
>   #include <xen/percpu.h>
>   
>   #ifndef NDEBUG
> -struct lock_debug {
> -    s16 irq_safe; /* +1: IRQ-safe; 0: not IRQ-safe; -1: don't know yet */
> +union lock_debug {
> +    unsigned short val;
> +    struct {
> +        unsigned short unused:1;

... it gives a rather misleading impression of this being an unused
field here. How about e.g. "unseen" instead?

> +        unsigned short irq_safe:1;
> +        unsigned short pad:2;
> +        unsigned short cpu:12;
> +    };
>   };

Do we have an implied assumption somewhere that unsigned short is
exactly 16 bits wide? I think "val" wants to be uint16_t here (as
you really mean "exactly 16 bits"), the two boolean fields want
to be bool, and the remaining two ones unsigned int.

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