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

[Jan Beulich <jbeulich@xxxxxxxx>]

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