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

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


  • To: Julien Grall <julien@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 13 Dec 2023 11:04:45 +0100
  • Autocrypt: addr=jbeulich@xxxxxxxx; keydata= xsDiBFk3nEQRBADAEaSw6zC/EJkiwGPXbWtPxl2xCdSoeepS07jW8UgcHNurfHvUzogEq5xk hu507c3BarVjyWCJOylMNR98Yd8VqD9UfmX0Hb8/BrA+Hl6/DB/eqGptrf4BSRwcZQM32aZK 7Pj2XbGWIUrZrd70x1eAP9QE3P79Y2oLrsCgbZJfEwCgvz9JjGmQqQkRiTVzlZVCJYcyGGsD /0tbFCzD2h20ahe8rC1gbb3K3qk+LpBtvjBu1RY9drYk0NymiGbJWZgab6t1jM7sk2vuf0Py O9Hf9XBmK0uE9IgMaiCpc32XV9oASz6UJebwkX+zF2jG5I1BfnO9g7KlotcA/v5ClMjgo6Gl MDY4HxoSRu3i1cqqSDtVlt+AOVBJBACrZcnHAUSuCXBPy0jOlBhxPqRWv6ND4c9PH1xjQ3NP nxJuMBS8rnNg22uyfAgmBKNLpLgAGVRMZGaGoJObGf72s6TeIqKJo/LtggAS9qAUiuKVnygo 3wjfkS9A3DRO+SpU7JqWdsveeIQyeyEJ/8PTowmSQLakF+3fote9ybzd880fSmFuIEJldWxp Y2ggPGpiZXVsaWNoQHN1c2UuY29tPsJgBBMRAgAgBQJZN5xEAhsDBgsJCAcDAgQVAggDBBYC AwECHgECF4AACgkQoDSui/t3IH4J+wCfQ5jHdEjCRHj23O/5ttg9r9OIruwAn3103WUITZee e7Sbg12UgcQ5lv7SzsFNBFk3nEQQCACCuTjCjFOUdi5Nm244F+78kLghRcin/awv+IrTcIWF hUpSs1Y91iQQ7KItirz5uwCPlwejSJDQJLIS+QtJHaXDXeV6NI0Uef1hP20+y8qydDiVkv6l IreXjTb7DvksRgJNvCkWtYnlS3mYvQ9NzS9PhyALWbXnH6sIJd2O9lKS1Mrfq+y0IXCP10eS FFGg+Av3IQeFatkJAyju0PPthyTqxSI4lZYuJVPknzgaeuJv/2NccrPvmeDg6Coe7ZIeQ8Yj t0ARxu2xytAkkLCel1Lz1WLmwLstV30g80nkgZf/wr+/BXJW/oIvRlonUkxv+IbBM3dX2OV8 AmRv1ySWPTP7AAMFB/9PQK/VtlNUJvg8GXj9ootzrteGfVZVVT4XBJkfwBcpC/XcPzldjv+3 HYudvpdNK3lLujXeA5fLOH+Z/G9WBc5pFVSMocI71I8bT8lIAzreg0WvkWg5V2WZsUMlnDL9 mpwIGFhlbM3gfDMs7MPMu8YQRFVdUvtSpaAs8OFfGQ0ia3LGZcjA6Ik2+xcqscEJzNH+qh8V m5jjp28yZgaqTaRbg3M/+MTbMpicpZuqF4rnB0AQD12/3BNWDR6bmh+EkYSMcEIpQmBM51qM EKYTQGybRCjpnKHGOxG0rfFY1085mBDZCH5Kx0cl0HVJuQKC+dV2ZY5AqjcKwAxpE75MLFkr wkkEGBECAAkFAlk3nEQCGwwACgkQoDSui/t3IH7nnwCfcJWUDUFKdCsBH/E5d+0ZnMQi+G0A nAuWpQkjM1ASeQwSHEeAWPgskBQL
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 13 Dec 2023 10:04:59 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

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