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

[Juergen Gross <jgross@suse.com>]

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

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].


Juergen

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

[-- 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 --]