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
