Re: [PATCH v4 06/12] xen/spinlock: make struct lock_profile rspinlock_t aware

[Jürgen Groß <jgross@xxxxxxxx>]

On 28.02.24 16:19, Jan Beulich wrote:
> On 12.12.2023 10:47, Juergen Gross wrote:
> > --- a/xen/common/spinlock.c
> > +++ b/xen/common/spinlock.c
> > @@ -538,19 +538,31 @@ static void spinlock_profile_iterate(lock_profile_subfunc 
> > *sub, void *par)
> >   static void cf_check spinlock_profile_print_elem(struct lock_profile *data,
> >       int32_t type, int32_t idx, void *par)
> >   {
> > -    struct spinlock *lock = data->lock;
> > +    unsigned int cpu;
> > +    uint32_t lockval;
> Any reason for this not being unsigned int as well? The more that ...
>
> > +    if ( data->is_rlock )
> > +    {
> > +        cpu = data->rlock->debug.cpu;
> > +        lockval = data->rlock->tickets.head_tail;
> > +    }
> > +    else
> > +    {
> > +        cpu = data->lock->debug.cpu;
> > +        lockval = data->lock->tickets.head_tail;
> > +    }
I've used the same type as tickets.head_tail.

> >   
> >       printk("%s ", lock_profile_ancs[type].name);
> >       if ( type != LOCKPROF_TYPE_GLOBAL )
> >           printk("%d ", idx);
> > -    printk("%s: addr=%p, lockval=%08x, ", data->name, lock,
> > -           lock->tickets.head_tail);
> > -    if ( lock->debug.cpu == SPINLOCK_NO_CPU )
> > +    printk("%s: addr=%p, lockval=%08x, ", data->name, data->lock, lockval);
> ... it's then printed with plain x as the format char.
Which hasn't been changed by the patch. I can change it to PRIx32 if you want.

> > +    if ( cpu == SPINLOCK_NO_CPU )
> >           printk("not locked\n");
> >       else
> > -        printk("cpu=%d\n", lock->debug.cpu);
> > -    printk("  lock:%" PRId64 "(%" PRI_stime "), block:%" PRId64 "(%" PRI_stime 
> > ")\n",
> > -           data->lock_cnt, data->time_hold, data->block_cnt, data->time_block);
> > +        printk("cpu=%u\n", cpu);
> > +    printk("  lock:%" PRIu64 "(%" PRI_stime "), block:%" PRIu64 "(%" PRI_stime 
> > ")\n",
> > +           data->lock_cnt, data->time_hold, (uint64_t)data->block_cnt,
> I think I know why the cast is suddenly / unexpectedly needed, but imo
> such wants stating in the description, when generally we aim at avoiding
> casts where possible.
Okay, will add a sentence.

> > --- a/xen/include/xen/spinlock.h
> > +++ b/xen/include/xen/spinlock.h
> > @@ -76,13 +76,19 @@ union lock_debug { };
> >   */
> >   
> >   struct spinlock;
> > +/* Temporary hack until a dedicated struct rspinlock is existing. */
> > +#define rspinlock spinlock
> >   
> >   struct lock_profile {
> >       struct lock_profile *next;       /* forward link */
> >       const char          *name;       /* lock name */
> > -    struct spinlock     *lock;       /* the lock itself */
> > +    union {
> > +        struct spinlock *lock;       /* the lock itself */
> > +        struct rspinlock *rlock;     /* the recursive lock itself */
> > +    };
> _LOCK_PROFILE() wants to initialize this field, unconditionally using
> .lock. While I expect that problem to be taken care of in one of the
> later patches, use of the macro won't work anymore with this union in
> use with very old gcc that formally we still support. While a road to
> generally raising the baseline requirements is still pretty unclear to
> me, an option might be to require (and document) that to enable
> DEBUG_LOCK_PROFILE somewhat newer gcc needs using.
Patch 8 is using either .lock or .rlock depending on the lock type.

What is the problem with the old gcc version? Static initializers of
anonymous union members?

> >       uint64_t            lock_cnt;    /* # of complete locking ops */
> > -    uint64_t            block_cnt;   /* # of complete wait for lock */
> > +    uint64_t            block_cnt:63; /* # of complete wait for lock */
> > +    uint64_t            is_rlock:1;  /* use rlock pointer */
> bool?
Yes.


Juergen