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

From: "Jürgen Groß" <jgross@suse.com>
To: Jan Beulich <jbeulich@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 17:22:43 +0100	[thread overview]
Message-ID: <9256b285-22b5-4c0c-8577-2b8832fb6e19@suse.com> (raw)
In-Reply-To: <8a0397c8-4ce6-4f4b-8a50-b0deaa30f291@suse.com>

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