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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
