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

[Juergen Gross <jgross@xxxxxxxx>]

On 08.08.19 08:58, Jan Beulich wrote:
> 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.
I wanted to be consistent with the type used in the structure.
I can switch to bool if you like that better.

> > @@ -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.
Okay.

> > +        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?
Yes, that seems to be the better choice.

> > +        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.
But that would increase the size of the union to 4 bytes instead of 2.
So at least pad and cpu must be unsigned short or (better) uint16_t.


Juergen


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