Re: [PATCH v4 08/12] xen/spinlock: add another function level

From: Julien Grall <julien@xen.org>
To: Juergen Gross <jgross@suse.com>, xen-devel@lists.xenproject.org
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>
Subject: Re: [PATCH v4 08/12] xen/spinlock: add another function level
Date: Wed, 13 Dec 2023 09:48:25 +0000	[thread overview]
Message-ID: <0f922ab8-ecad-40f5-a63c-c3be9006a5be@xen.org> (raw)
In-Reply-To: <ea73d863-6ead-4b03-98f4-7ab873fa614b@suse.com>

On 13/12/2023 09:17, Juergen Gross wrote:
> On 13.12.23 09:43, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 13/12/2023 06:23, Juergen Gross wrote:
>>> On 12.12.23 20:10, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 12/12/2023 09:47, Juergen Gross wrote:
>>>>> Add another function level in spinlock.c hiding the spinlock_t layout
>>>>> from the low level locking code.
>>>>>
>>>>> This is done in preparation of introducing rspinlock_t for recursive
>>>>> locks without having to duplicate all of the locking code.
>>>>
>>>> So all the fields you pass are the one from spinlock.
>>>>
>>>> Looking at pahole after this series is applid, we have:
>>>>
>>>> ==== Debug + Lock profile ====
>>>>
>>>> struct spinlock {
>>>>          spinlock_tickets_t         tickets;              /*     0 4 */
>>>>          union lock_debug           debug;                /*     4 4 */
>>>>          struct lock_profile *      profile;              /*     8 8 */
>>>>
>>>>          /* size: 16, cachelines: 1, members: 3 */
>>>>          /* last cacheline: 16 bytes */
>>>> };
>>>> struct rspinlock {
>>>>          spinlock_tickets_t         tickets;              /*     0 4 */
>>>>          uint16_t                   recurse_cpu;          /*     4 2 */
>>>>          uint8_t                    recurse_cnt;          /*     6 1 */
>>>>
>>>>          /* XXX 1 byte hole, try to pack */
>>>>
>>>>          union lock_debug           debug;                /*     8 4 */
>>>>
>>>>          /* XXX 4 bytes hole, try to pack */
>>>>
>>>>          struct lock_profile *      profile;              /*    16 8 */
>>>>
>>>>          /* size: 24, cachelines: 1, members: 5 */
>>>>          /* sum members: 19, holes: 2, sum holes: 5 */
>>>>          /* last cacheline: 24 bytes */
>>>> };
>>>>
>>>>
>>>> ==== Debug ====
>>>>
>>>> struct spinlock {
>>>>          spinlock_tickets_t         tickets;              /*     0 4 */
>>>>          union lock_debug           debug;                /*     4 4 */
>>>>
>>>>          /* size: 8, cachelines: 1, members: 2 */
>>>>          /* last cacheline: 8 bytes */
>>>> };
>>>> struct rspinlock {
>>>>          spinlock_tickets_t         tickets;              /*     0 4 */
>>>>          uint16_t                   recurse_cpu;          /*     4 2 */
>>>>          uint8_t                    recurse_cnt;          /*     6 1 */
>>>>
>>>>          /* XXX 1 byte hole, try to pack */
>>>>
>>>>          union lock_debug           debug;                /*     8 4 */
>>>>
>>>>          /* size: 12, cachelines: 1, members: 4 */
>>>>          /* sum members: 11, holes: 1, sum holes: 1 */
>>>>          /* last cacheline: 12 bytes */
>>>> };
>>>>
>>>> ==== Prod ====
>>>>
>>>> struct spinlock {
>>>>          spinlock_tickets_t         tickets;              /*     0 4 */
>>>>          union lock_debug           debug;                /*     4 0 */
>>>>
>>>>          /* size: 4, cachelines: 1, members: 2 */
>>>>          /* last cacheline: 4 bytes */
>>>> };
>>>> struct rspinlock {
>>>>          spinlock_tickets_t         tickets;              /*     0 4 */
>>>>          uint16_t                   recurse_cpu;          /*     4 2 */
>>>>          uint8_t                    recurse_cnt;          /*     6 1 */
>>>>          union lock_debug           debug;                /*     7 0 */
>>>>
>>>>          /* size: 8, cachelines: 1, members: 4 */
>>>>          /* padding: 1 */
>>>>          /* last cacheline: 8 bytes */
>>>> };
>>>>
>>>>
>>>> I think we could embed spinlock_t in rspinlock_t without increasing 
>>>> rspinlock_t. Have you considered it? This could reduce the number of 
>>>> function level introduced in this series.
>>>
>>> That was the layout in the first version of this series. Jan 
>>> requested to change
>>> it to the current layout [1].
>>
>> Ah... Looking through the reasoning, I have to disagree with Jan 
>> argumentations.
> 
> I would _really_ have liked you to mention this disagreement back then 
> (you've
> been on Cc: in the thread, too).

Sorry for that. My e-mails backlog is quite large and I can't keep up 
with all the series.

> Letting me do a major rework and then after 2 more iterations of the series
> requesting to undo most of the work isn't great.

Indeed. But I note you continued without any additional feedback [1]. If 
you were not sure about the approach suggested by Jan, then why did you 
post two new versions? Shouldn't you have pinged the maintainers to make 
sure there is a consensus?

> 
>> At least with the full series applied, there is no increase of 
>> rspinlock_t in debug build (if we compare to the version you provided 
>> in this series).
> 
> That wasn't his sole reasoning, right?

I guess you mean the non-optional fields should always be at the same 
position?

> 
>> Furthermore, this is going to remove at least patch #6 and #8. We 
>> would still need nrspinlock_* because they can just be wrapper to
>> spin_barrier(&lock->lock).
>>
>> This should also solve his concern of unwieldy code:
>>
>>  > +    spin_barrier(&p2m->pod.lock.lock.lock);
> 
> Yes, but the demand to have optional fields at the end of the struct isn't
> covered by your request.

I note this was a preference and weight against code duplication. It is 
not clear to me whether Jan agrees with this extra work now.

Anyway, I am not against this approach and if this is what Jan much 
prefers then so be it. But I thought I would point out the additional 
complexity which doesn't seem to be worth it.

Cheers,

[1] https://lists.xen.org/archives/html/xen-devel/2022-12/msg01065.html

-- 
Julien Grall