Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.160 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.160; helo=mail.nvidia.com; pr=C
Message-ID: <1634ca8f-2b22-712e-15f9-9980ba8a4e64@nvidia.com>
Date:   Fri, 16 Jun 2023 15:35:04 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH] mm: Move mm_count into its own cache line
To:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Andrew Morton <akpm@linux-foundation.org>
CC:     Peter Zijlstra <peterz@infradead.org>,
        <linux-kernel@vger.kernel.org>,
        kernel test robot <yujie.liu@intel.com>,
        Aaron Lu <aaron.lu@intel.com>,
        Olivier Dion <odion@efficios.com>,
        <michael.christie@oracle.com>, Feng Tang <feng.tang@intel.com>,
        Jason Gunthorpe <jgg@nvidia.com>, Peter Xu <peterx@redhat.com>,
        <linux-mm@kvack.org>
References: <20230515143536.114960-1-mathieu.desnoyers@efficios.com>
 <20230616131639.992998157fe696eb0e0589aa@linux-foundation.org>
 <efb2e71a-9b18-4ccd-bdbc-3014fae032fc@efficios.com>
Content-Language: en-US
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <efb2e71a-9b18-4ccd-bdbc-3014fae032fc@efficios.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jun 2023 22:35:18.7659
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 96e87d0c-b17f-4592-c67b-08db6eb9f69d
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.160];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: MWH0EPF000971E4.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB6682
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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