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;
Date:   Thu, 20 Apr 2023 22:18:52 +0800
From:   Aaron Lu <aaron.lu@intel.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC:     Peter Zijlstra <peterz@infradead.org>,
        <linux-kernel@vger.kernel.org>, Olivier Dion <odion@efficios.com>,
        <michael.christie@oracle.com>
Subject: Re: [RFC PATCH v9 2/2] sched: Fix performance regression introduced
 by mm_cid
Message-ID: <20230420141852.GA154815@ziqianlu-desk2>
References: <20230419155012.63901-1-mathieu.desnoyers@efficios.com>
 <20230419155012.63901-2-mathieu.desnoyers@efficios.com>
 <20230420095610.GA153295@ziqianlu-desk2>
 <c01ddfc5-9410-14e1-55f7-c24f44447f8a@efficios.com>
 <20230420125048.GA154262@ziqianlu-desk2>
 <721f4b8b-c238-53b1-9085-a9dae6a961e1@efficios.com>
 <20230420133519.GA154479@ziqianlu-desk2>
 <7a0c1db1-103d-d518-ed96-1584a28fbf32@efficios.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <7a0c1db1-103d-d518-ed96-1584a28fbf32@efficios.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MHJqdUFzaFBhS2IwVzlVR2ZtSERuMjlTUk4zQndnS0lhMTRJVWxUOTFUSHps?=
 =?utf-8?B?aThXaERXbDllQ2NjZkZRMWx4NFZ1NE9EcnFjdnBBS2JPbHVEb3U3c0VXN2F3?=
 =?utf-8?B?dHdGdXgvUUo4bVExVnNYYzl5aDlaSHBaT0ptc0swVThGMEN1Q0w4OHNuYWs2?=
 =?utf-8?B?S1c1MDNlT0p5eVVTVFFkNmJDWWhCR1UxdG9wUDQ0SW11N082T0VvT083SnMx?=
 =?utf-8?B?V1UxeDlqVWZIK3l2bHIxdjJqSm02bTFRZHU5RnVyNFNPZjl4SGlBTnY4Umgx?=
 =?utf-8?B?Mk9jYTgrL1N6Vm9BdUROZGVLUUZBL3R3VXhQbmJYNlViSWhTTjF3M29UTmNu?=
 =?utf-8?B?VDRBdXYxMVVFaENLa2diU3FoWkZITzlycE1yakhBYi9Ea2NYZDArMzJFb2Yr?=
 =?utf-8?B?ZGZUTVhmc0FrZXV6ZTdGUklkV1FLUHlxRUIrbjFGSHBKd3VBRTNHMm1IME80?=
 =?utf-8?B?MnUxUHB0eWZwY1hLWGRvUzNUUG80aWVXMVI4TDdwTEZMS2w0SlQweTdnSkli?=
 =?utf-8?B?YzQ2eGh2Vy9EOFVrM2RkVytuME1iL2VSV1dzUHZRQnFOVTkxWmgrMTVOSkY3?=
 =?utf-8?B?UVNVQmpCdjR4OXIxSWtXQlk4NGZBQlliY3AvSER2VVVEYU9POVhYU0I4clUw?=
 =?utf-8?B?SlpZQ3NLNnBUeUZJNXZ6SWx6enlEcC9ZN1E4RnNkdmFVbThncGhEZTlscHdH?=
 =?utf-8?B?eDNjNEY3Rm1JdEFtUnlkQk1SMWRERDhCZ0tzd0hXa2pPK3U2NW9qKzlrNlds?=
 =?utf-8?B?M1dNeitrQUFqdVdyaEF2T2JWZ21qbHd5NXdzZkJsSEtCWUp5VVhKaUN4VnZx?=
 =?utf-8?B?M1V4cS9lMUdvMGFzUm9sR2pmUHlZQUJ6emdHN3pKcjUzTEhhSXRDUVdjakxG?=
 =?utf-8?B?MHBkeUtxQ2gwVkJMV3VaMEM5S3AzZlFPSURKVm1TL1hLNkJWNnY2VDlWTkNM?=
 =?utf-8?B?eG9GT0o4VlZhMFplcVRQRkUwa3cxNlc0eHdSZVBpSTI2QU5MQ2g4UC9vcmUv?=
 =?utf-8?B?RW15Q3FDTktQdVdSWFN4ZzVMZmFPc2xCWXV4NFhJQTI4cnV0SUNKUVpKVkRM?=
 =?utf-8?B?cFl5bVN4WFhZeEMrQ0k4MnpHRnc0b0NMdml0SVhTRENHaHcvUlJkb2RJZGRQ?=
 =?utf-8?B?N2tvbFZJNFp1cy9PVmhadzFxUWgySXYyanp4Zi9WNFBLRmc3MnRHKzRjeDBE?=
 =?utf-8?B?MitSRzFzWDNlNzZ1ZVp2aHFtZmczOEZXQlhBc0NLUjNpTkRqMFhJMXJ0L2hE?=
 =?utf-8?B?cjNCQlR1LzZqSUlvRTBJQURmKzlGRVEvOVdBeUVvQ2FEcndNVDFGQnpmZHZv?=
 =?utf-8?B?aFkzT1BsTkxXKzgwNWwzSzdCeVR4eFVDQXdHOTZPOW8zOUxXdmp2NkJ3L0g5?=
 =?utf-8?B?U0lBSlJBMWxvblltUzU1VzdCaTBwejUzSzI4NzZ2dDR0bUtCNTZQaDIzNE02?=
 =?utf-8?B?Z2Zwb2J0WFRiL1JyK0R4dEFSTHA3MUZ2MjJmM2FsNWVrNkgvamlwR04xTW9h?=
 =?utf-8?B?MUhzSTRncE0wcWJyM0laeTZwQ1ZQdnl4dUt4bEh4aFBiVDZzaXVJUUZBSkpT?=
 =?utf-8?B?bW1nSTVXNVZhY1VzUlA1UEZvbGRYNkRxaFhSampELy80amluT01qWFJub0Q4?=
 =?utf-8?B?eWxYSW9HaWpaWmpseHUzUm9uRS91MnVHV0RoSTE4UjdERVZQRDRUZHZ3NnBH?=
 =?utf-8?B?cU1DS3pXU1V3YnNtNnNkYS85azZ3dVdqYjhxZ280OVVYZGdOblZOREZKdVg5?=
 =?utf-8?B?M3JaMXd5NnZGNFZlNlBqLytZUzhBbEsrbUJSa3FGR203L2FIZEw3aVh5THVO?=
 =?utf-8?B?RWZzUGo3SG5Fd0QvWGtXWk5BcTRMU055SkFSRE9pTFZRQWFvTUt4V0FTRWtJ?=
 =?utf-8?B?dCtvZUFBSVl4Z09qM2dHUGZKK045SEozaU5zMnU5Zk9XZDNOdXNCcU13T3Qx?=
 =?utf-8?B?SXhrY0RmV01MUDJ4NzN1SUJnUTh0UkxieXI0MEFVM2R5SWJaUzd5dWhLWWYx?=
 =?utf-8?B?WW5nRjdFUXdva2lTQktoQVdiWkhNMVpPcHlybzU1aE42N1JOVnl4YzFma2do?=
 =?utf-8?B?UjF4T3FFbUplaGFQblV0LzdxbHc5ZXZuWkVERWVOLzVhUWJtRnZsVjhBWW9G?=
 =?utf-8?Q?rsgOu+M8EOmRgugIxGBvOAzff?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a3fc46e9-94d2-44c5-55dc-08db41aa2f9c
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3062.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Apr 2023 14:19:00.4725
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pjTWp7JLD2c2vsG+gp98PEzvbk1WxLtXA22ZwVq+MDSBaZwaMvdQtidjfUl+C1rDj+RHB9SpHUl3DbzDmHN/1A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB7564
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        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 Thu, Apr 20, 2023 at 09:54:29AM -0400, Mathieu Desnoyers wrote:
> On 2023-04-20 09:35, Aaron Lu wrote:
> [...]
> > > > > 
> > > > > Then we clearly have another member of mm_struct on the same cache line as
> > > > > pcpu_cid which is bouncing all over the place and causing false-sharing. Any
> > > > > idea which field(s) are causing this ?
> > > > 
> > > > That's my first reaction too but as I said in an earlier reply:
> > > > https://lore.kernel.org/lkml/20230419080606.GA4247@ziqianlu-desk2/
> > > > I've tried to place pcpu_cid into a dedicate cacheline with no other
> > > > fields sharing a cacheline with it in mm_struct but it didn't help...
> > > 
> > > I see two possible culprits there:
> > > 
> > > 1) The mm_struct pcpu_cid field is suffering from false-sharing. I would be
> > >     interested to look at your attempt to move it to a separate cache line to
> > >     try to figure out what is going on.
> > 
> > Brain damaged...my mistake, I only made sure its following fields not
> > share the same cacheline but forgot to exclude its preceding fields and
> > turned out it's one(some?) of the preceeding fields that caused false
> > sharing. When I did:
> > 
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index 5eab61156f0e..a6f9d815991c 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -606,6 +606,7 @@ struct mm_struct {
> >                   */
> >                  atomic_t mm_count;
> >   #ifdef CONFIG_SCHED_MM_CID
> > +               CACHELINE_PADDING(_pad1_);
> >                  /**
> >                   * @pcpu_cid: Per-cpu current cid.
> >                   *
> > mm_cid_get() dropped to 0.0x% when running hackbench :-)
> 
> Now we are talking! :)
> 
> > 
> > sched_mm_cid_migrate_to() is about 4% with most cycles spent on
> > accessing mm->mm_users:
> > 
> >         │     dst_cid = READ_ONCE(dst_pcpu_cid->cid);
> >    0.03 │       mov     0x8(%r12),%r15d
> >         │     if (!mm_cid_is_unset(dst_cid) &&
> >    0.07 │       cmp     $0xffffffff,%r15d
> >         │     ↓ je      87
> >         │     arch_atomic_read():
> >         │     {
> >         │     /*
> >         │     * Note for KASAN: we deliberately don't use READ_ONCE_NOCHECK() here,
> >         │     * it's non-inlined function that increases binary size and stack usage.
> >         │     */
> >         │     return __READ_ONCE((v)->counter);
> >   76.13 │       mov     0x54(%r13),%eax
> >         │     sched_mm_cid_migrate_to():
> >         │       cmp     %eax,0x410(%rdx)
> >   21.71 │     ↓ jle     1d8
> >         │     atomic_read(&mm->mm_users) >= t->nr_cpus_allowed)
> > 
> > With this info, it should be mm_users that caused false sharing for
> > pcpu_cid previously. Looks like mm_users is bouncing.
> 
> I suspect that the culprit here is mm_count rather than mm_users. mm_users
> just happens to share the same cache line as mm_count.
> 
> mm_count is incremented/decremented with mmgrab()/mmdrop() during
> context switch.
> 
> This is likely causing other issues, for instance, the
> membarrier_state field is AFAIR read-mostly, used for
> membarrier_mm_sync_core_before_usermode() to issue core
> sync before every return to usermode if needed.
> 
> Other things like mm_struct pgd pointer appear to be likely
> read-mostly variables.
> 
> I suspect it's mm_count which should be moved to its own cache line
> to eliminate false-sharing with all the other read-mostly fields
> of mm_struct.
> 
> Thoughts ?

Makes sesne, I was wondering where the write side of mm_user is. Let me
see how that goes by placing mm_count aside from other read mostly fields.