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

[Juergen Gross <jgross@xxxxxxxx>]

On 12.12.23 19:42, Julien Grall wrote:
> Hi,
>
> On 12/12/2023 09:47, Juergen Gross wrote:
> > Struct lock_profile contains a pointer to the spinlock it is associated
> > with. Prepare support of differing spinlock_t and rspinlock_t types by
> > adding a type indicator of the pointer. Use the highest bit of the
> > block_cnt member for this indicator in order to not grow the struct
> > while hurting only the slow path with slightly less performant code.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > Acked-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> > ---
> > V2:
> > - new patch
> > ---
> >   xen/common/spinlock.c      | 26 +++++++++++++++++++-------
> >   xen/include/xen/spinlock.h | 10 ++++++++--
> >   2 files changed, 27 insertions(+), 9 deletions(-)
> >
> > diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
> > index c1a9ba1304..7d611d3d7d 100644
> > --- 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;
> > +
> > +    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;
> > +    }
> >       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);
> > +    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,
> > +           data->time_block);
> >   }
> >   void cf_check spinlock_profile_printall(unsigned char key)
> > diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h
> > index 05b97c1e03..ac3bef267a 100644
> > --- 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 */
> > +    };
> >       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 */
> This is meant to act like a bool. So I would prefer if we use:
>
> bool is_rwlock:1;
>
> And then use true/false when set.
Do we want to do that? AFAIK it would depend on the compiler what the size of
the struct is when mixing types in bitfields (in this case: bool and uint64_t).

> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
Thanks,


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature