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

[Julien Grall <julien@xxxxxxx>]

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.
Cheers,

--
Julien Grall