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

From: Juergen Gross <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Alejandro Vallejo <alejandro.vallejo@cloud.com>,
	Julien Grall <julien@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v4 06/12] xen/spinlock: make struct lock_profile rspinlock_t aware
Date: Wed, 13 Dec 2023 10:07:42 +0100	[thread overview]
Message-ID: <b838696e-d5b2-4d48-8261-ca24a86cdbde@suse.com> (raw)
In-Reply-To: <3f5f7df0-d7e5-4540-b01b-9b2cde9dcc55@suse.com>

[-- Attachment #1.1.1: Type: text/plain, Size: 1941 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]