linux-kernel - Re: [PATCH] mm: Move mm_count into its own cache line

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1634ca8f-2b22-712e-15f9-9980ba8a4e64@nvidia.com>
Date:   Fri, 16 Jun 2023 15:35:04 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Andrew Morton <akpm@...ux-foundation.org>
CC:     Peter Zijlstra <peterz@...radead.org>,
        <linux-kernel@...r.kernel.org>,
        kernel test robot <yujie.liu@...el.com>,
        Aaron Lu <aaron.lu@...el.com>,
        Olivier Dion <odion@...icios.com>,
        <michael.christie@...cle.com>, Feng Tang <feng.tang@...el.com>,
        Jason Gunthorpe <jgg@...dia.com>, Peter Xu <peterx@...hat.com>,
        <linux-mm@...ck.org>
Subject: Re: [PATCH] mm: Move mm_count into its own cache line

On 6/16/23 13:38, Mathieu Desnoyers wrote:
...
>>> This comment is rather odd for a few reasons:
>>>
>>> - It requires addition/removal of mm_struct fields to carefully consider
>>>    field alignment of _other_ fields,
>>> - It expresses the wish to keep an "optimal" alignment for a specific
>>>    kernel config.
>>>
>>> I suspect that the author of this comment may want to revisit this topic
>>> and perhaps introduce a split-struct approach for struct rw_semaphore,
>>> if the need is to place various fields of this structure in different
>>> cache lines.
>>>

Agreed. The whole thing is far too fragile, but when reviewing this I
wasn't sure what else to suggest. Now looking at it again with your
alignment suggestion, there is an interesting conflicting set of
desires:

a) Here: Feng Tang discovered that .count and .owner are best put in
separate cache lines for the contended case for mmap_lock, and

b) rwsem.h, which specifies precisely the opposite for the uncontended
case:

  * For an uncontended rwsem, count and owner are the only fields a task
  * needs to touch when acquiring the rwsem. So they are put next to each
  * other to increase the chance that they will share the same cacheline.

I suspect that overall, it's "better" to align rw_semaphore's .count and
.owner field so that the lock is optimized for the contended case,
because it's reasonable to claim that the benefit of having those two
fields in the same cacheline for the uncontended case is far less than
the cost to the contended case, of keeping them close to each other.

However, it's still not unlikely that someone will measure a performance
regression if such a change is made.

Thoughts?

...
>> If the plan is to put mm_count in "its own" cacheline then padding will
>> be needed?
> 
> It's taken care of by the anonymous structure trick. Here is an quick example showing the difference between alignment attribute applied to an integer type vs to an anonymous structure:

Thanks for explaining very clearly how that works, that's really
helpful!
thanks,
-- 
John Hubbard
NVIDIA