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

[Juergen Gross <jgross@xxxxxxxx>]

On 13.12.23 10:48, Julien Grall wrote:
> 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?
https://lists.xen.org/archives/html/xen-devel/2023-10/msg01221.html

> > > 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?
Yes.

> > > 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.
Thanks for the clarification.

Jan?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature