[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.