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

From: Jan Beulich <jbeulich@suse.com>
To: Juergen Gross <jgross@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Alejandro Vallejo <alejandro.vallejo@cloud.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v4 06/12] xen/spinlock: make struct lock_profile rspinlock_t aware
Date: Wed, 28 Feb 2024 16:19:23 +0100	[thread overview]
Message-ID: <b76bff63-80de-4227-bcd9-8d3aa1275858@suse.com> (raw)
In-Reply-To: <20231212094725.22184-7-jgross@suse.com>

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;
> +    }
>  
>      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.

> +    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.

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

>      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?

Jan