[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 10:48, Julien Grall wrote: > On 13/12/2023 09:17, Juergen Gross wrote: >> On 13.12.23 09:43, Julien Grall wrote: >>> On 13/12/2023 06:23, Juergen Gross wrote: >>>> On 12.12.23 20:10, Julien Grall wrote: >>>>> 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? I think this is unfair to Jürgen. We use the lazy consensus model generally, and hence no replies generally mean consensus. Also note that it has been very close to a fully year between my review comments back then and now. It has been well over a year from the original posting of the RFC. That said, I also understand that in particular RFCs receive less attention, no matter that this is entirely contrary to their purpose. That's all the same for me - I hardly ever look at RFCs as long as there are still non-RFC patches pending review. Which in reality means it is close to impossible to ever look at RFCs. >>> 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? I consider this at least desirable, 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. Well, at the time I said I think "that's a reasonable price to pay", to further state "with some de-duplication potential". > 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. It's not "much", I would say, but some of the earlier oddities (like also the .lock.lock.lock) would be really nice if they went away. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |