diff mbox series

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

Message ID 20231212094725.22184-7-jgross@suse.com (mailing list archive)
State New, archived
Headers show
Series xen/spinlock: make recursive spinlocks a dedicated type | expand

Commit Message

Jürgen Groß Dec. 12, 2023, 9:47 a.m. UTC
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@suse.com>
Acked-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
V2:
- new patch
---
 xen/common/spinlock.c      | 26 +++++++++++++++++++-------
 xen/include/xen/spinlock.h | 10 ++++++++--
 2 files changed, 27 insertions(+), 9 deletions(-)

Comments

Julien Grall Dec. 12, 2023, 6:42 p.m. UTC | #1
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@suse.com>
> Acked-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> 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.

Acked-by: Julien Grall <jgrall@amazon.com>

>       s_time_t            time_hold;   /* cumulated lock time */
>       s_time_t            time_block;  /* cumulated wait time */
>       s_time_t            time_locked; /* system time of last locking */

Cheers,
Jürgen Groß Dec. 13, 2023, 6:05 a.m. UTC | #2
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@suse.com>
>> Acked-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>> ---
>> 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@amazon.com>

Thanks,


Juergen
Julien Grall Dec. 13, 2023, 8:32 a.m. UTC | #3
Hi Juergen,

On 13/12/2023 06:05, Juergen Gross wrote:
> 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@suse.com>
>>> Acked-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>>> ---
>>> 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).

I looked through our codebase and I could already find some use. Such as:

typedef union
{
     struct
     {
         uint8_t  vector;
         uint8_t  type:3;
         bool     ev:1;
         uint32_t resvd1:19;
         bool     v:1;
         uint32_t ec;
     };
     uint64_t raw;
} intinfo_t;

So I would assume that a mix is possible or we would have problem in 
other places.

Anyway, this is not a critical comment. It is just a preference of using 
true/false when a field can only be 0 or 1.

Cheers,
Jan Beulich Dec. 13, 2023, 8:36 a.m. UTC | #4
On 13.12.2023 07:05, Juergen Gross wrote:
> On 12.12.23 19:42, Julien Grall wrote:
>> On 12/12/2023 09:47, Juergen Gross wrote:
>>> --- 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).

I thought in a similar way as you did when Andrew introduced similar
patterns (see Julien's reply for an example), and was then convinced
that the compiler really is supposed to be doing what we want here.
So yes, I second Julien's desire to have bool used when boolean is
meant.

Jan
Jürgen Groß Dec. 13, 2023, 9:07 a.m. UTC | #5
On 13.12.23 09:36, Jan Beulich wrote:
> On 13.12.2023 07:05, Juergen Gross wrote:
>> On 12.12.23 19:42, Julien Grall wrote:
>>> On 12/12/2023 09:47, Juergen Gross wrote:
>>>> --- 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).
> 
> I thought in a similar way as you did when Andrew introduced similar
> patterns (see Julien's reply for an example), and was then convinced
> that the compiler really is supposed to be doing what we want here.
> So yes, I second Julien's desire to have bool used when boolean is
> meant.

Okay, fine with me.


Juergen
Jan Beulich Feb. 28, 2024, 3:19 p.m. UTC | #6
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
Jürgen Groß Feb. 28, 2024, 3:43 p.m. UTC | #7
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
Jan Beulich Feb. 28, 2024, 4:02 p.m. UTC | #8
On 28.02.2024 16:43, Jürgen Groß wrote:
> 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.

As per ./CODING_STYLE unsigned int is preferred.

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

Yes.

Jan
Jürgen Groß Feb. 28, 2024, 4:22 p.m. UTC | #9
On 28.02.24 17:02, Jan Beulich wrote:
> On 28.02.2024 16:43, Jürgen Groß wrote:
>> 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.
> 
> As per ./CODING_STYLE unsigned int is preferred.

Okay, I'll switch to unsigned int then.

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

The easiest solution might then be to give the union a name.


Juergen
diff mbox series

Patch

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 */
     s_time_t            time_hold;   /* cumulated lock time */
     s_time_t            time_block;  /* cumulated wait time */
     s_time_t            time_locked; /* system time of last locking */