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 17:56:10 +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: <20230420095610.GA153295@ziqianlu-desk2>
References: <20230419155012.63901-1-mathieu.desnoyers@efficios.com>
 <20230419155012.63901-2-mathieu.desnoyers@efficios.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230419155012.63901-2-mathieu.desnoyers@efficios.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?K2psOXR5bVJObHd5bng4Zkowd210ZmR4ZkttRzQwbElNYXFiTHU5MGdQbWVa?=
 =?utf-8?B?cjEyVjJhVzhYbFE4ZmFYS2FHbDlvL1YwRTdic3hWUmVVWHBQMEpuNkVFbHpI?=
 =?utf-8?B?Yjd0eHIrYWZoNnNta2JSV1VwUUhvOGM4Yk1DQXEwT0pOYjhURHQzYWc4UWNk?=
 =?utf-8?B?TXJtWFRBdkp0aHNPK2U3QUNyMmpkMUJTZ28wYVBkMTJCR2VwS3hpak1kS3My?=
 =?utf-8?B?VHNDZW5RQWxFRk44a2xNNFgzUnpBeUtBSHZXcGhySWx6UnZLSVhtTnZxVzU1?=
 =?utf-8?B?SUtOU2pQcERHY0lJaUF4T0RSQVhhMXcyQ2xaS2I3UnpLZzR5aEJUY0ViOTBU?=
 =?utf-8?B?T29TVDhIV2cxRVFKWFV1Rk10d2xzUUJJeStFOXcrYVJQdXpHb1l2cjdGN05p?=
 =?utf-8?B?TTBFVkkyWmUrOG5XZWZTWW9tenEyZ3lEVXFsNW5HWjJrdCtxaGpBUDk5VS9Z?=
 =?utf-8?B?eU5KUWlURVVmSHRCSVFsTFdOMFp4bDFVdmJRZlk5VW9uRTBBNndBc0wvMzd1?=
 =?utf-8?B?TXIycm1xRjR5bzFxaGZDL1FUU2VLeEplVG1rMk1HeTRsMVdnR3IwQXlnTUZQ?=
 =?utf-8?B?bFg0dlNDeTVXOEVPT25UQWxSR0FDdUNOZy8rSTFlVVMrZ3RXTEpQWUhrS2Vm?=
 =?utf-8?B?NFFCVy8yRmp5K24yZGR2MTFtZEVMZURwU29odW51aHdvYnBuZmVnMk1Ea1J6?=
 =?utf-8?B?T1J5SFdJdytOYmxMMnB2dVV4NVhmUksyTE5hZk9DRVZkQ3Ywd3BsYjgwMU0w?=
 =?utf-8?B?RFZLdmtDdFY2Z01JaXZTaEVJV3RvaE0zQTg1VW85YWdyZlRYTTZ4L2JpZlND?=
 =?utf-8?B?RDAzcGJkaG5iWTlrNENWTXlZY0pKTGEvcTIwWHdHd1hQcVVNbnZMSjFObUt1?=
 =?utf-8?B?UDZIeXNySEJ2ZnBSVjVkZVBWejZDM3RGZnQvdGNUV2F4Q3VNVm0vUTFqM21w?=
 =?utf-8?B?QVhSN1RxenRmNDI3aFZ6RlV0ZEhKUHI1NzRrakwraUpwOHdIc2tUenp1dlRr?=
 =?utf-8?B?WlI2VXRMTUgrWGhiV0toTXB5OHpNejVFSUs0SEFtY3VWaWVYakJJU1Rkc2pm?=
 =?utf-8?B?SFlyTVJ1a3lHN1NGYkJ2S1ZxSkNXOHkyRzVVZVgvNkZqZ05oUmgwSm1yaHcz?=
 =?utf-8?B?bzBUWm8vRXpGdnBnWVozQSthcmNLaG9OL095Y0JJSS8xc0Q4enhCeEhibWNu?=
 =?utf-8?B?UkZ5YUN1RmU5NDBTU29KSjkxanNLRzZZNnhFcXhkNVU2S1ZvYmVjWnIrYXRr?=
 =?utf-8?B?Ujc2bnZZMG1tcDdURm8ybXpwdlVZVkkyV3hIVnhnWEVERUlJOCs0b3FXdkpz?=
 =?utf-8?B?VE9SQi81MkRhRHBJVUFFSmlXS3UxekRKMm1LK1h6c3lHTzBoSW5ZUjNHMzha?=
 =?utf-8?B?ME8xVE85cjhvd3Vzbkd3aStLL3VZcXA3ZW9yYUg0WXBuMVNpL2c4Q2MvdjVy?=
 =?utf-8?B?SERnYU5aK2gzeFcvUThtY0h4NXFuM1NWODhRRjUxOWRuaEZDNzVxUjdoa3kx?=
 =?utf-8?B?dG9zYlBFN2UyUXhFaHUyMENOWGlha2tXUHVOTnA4VE9XZzFmSFM1eXZzK3Rk?=
 =?utf-8?B?bzltaEtoRnpyVCtSeWt4NmdBbG5UeTVhYzd3RjBOM2JYeEl3RkRWbW1NSVM3?=
 =?utf-8?B?MEV2SzBLM1B4MVJQRjhST1NQUGtlbzNTZEpKUDBqSXRNaG14Snl0YVZiTW5C?=
 =?utf-8?B?Y1VMdHhKbmVQYXY0bThaL3VYVTBTRzhGL2FINVgzdzEwdzROcWpSSXM1RFNP?=
 =?utf-8?B?NUJObDd6VHR5SkZkbmY1TTNmUG4rTEw4QXRCam1nVkNvZ2lLQVBkcDJqVVdi?=
 =?utf-8?B?ZFhJUWF5Sjh2THJVYmJ3ZDlvS2VVTlZxT1ptRlB5bWswcVZwSTg5SXpNemlI?=
 =?utf-8?B?a0Y1UFN0NHdEQUlxcEUvUHlwRGhNdFlsN1ZrV2pzWWc5WmZZZEN1c0g0TkV0?=
 =?utf-8?B?MEwxMjZwZU1VcTJlVHNmcnRhOHNxUFJ3d3hmcHBBQ29BOWg0cGZIOGkxVFEr?=
 =?utf-8?B?L0ZqVU5QTXVqOGNjeThuS29jUHJRQzAvUzc2VWVxYnFjM3kvSS9HNGNnY0p6?=
 =?utf-8?B?d1ZpazlFSlR6VXFxMzFuMU1ZS3dCekVLdi9LaHhLaVJyWjJIRW9qRmMxb3VO?=
 =?utf-8?Q?g+w1YnvxwJgh+cwPGav4nAnIG?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0ba0400b-6ef1-4746-8fd8-08db41857ea2
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3062.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Apr 2023 09:56:21.7783
 (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: gPDY7aMYyvKtRsCr/s7YLhCBURpO8NpxhLMVBmk9E1uH7L584Unum2VIAeCWtHo4b5U+ENPW68IgqrwhDNZ50A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB4860
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 Wed, Apr 19, 2023 at 11:50:12AM -0400, Mathieu Desnoyers wrote:
> Introduce per-mm/cpu current concurrency id (mm_cid) to fix a PostgreSQL
> sysbench regression reported by Aaron Lu.

mm_cid_get() dropped to 5.x% after I disable CONFIG_DEBUG_PREEMPT, using
__this_cpu_X() doesn't help, I suppose that is because __this_cpu_X()
still needs to fetch mm->pcpu_cid.

Annotate mm_cid_get():

       │     static inline int mm_cid_get(struct mm_struct *mm)
       │     {
  0.05 │       push   %rbp
  0.02 │       mov    %rsp,%rbp
       │       push   %r15
       │       push   %r14
       │       push   %r13
       │       push   %r12
       │       push   %rbx
  0.02 │       sub    $0x10,%rsp
       │     struct mm_cid __percpu *pcpu_cid = mm->pcpu_cid;
 71.30 │       mov    0x60(%rdi),%r12
       │     struct cpumask *cpumask;
       │     int cid;
       │
       │     lockdep_assert_rq_held(rq);
       │     cpumask = mm_cidmask(mm);
       │     cid = __this_cpu_read(pcpu_cid->cid);
 28.44 │       mov    %gs:0x8(%r12),%edx
       │     if (mm_cid_is_valid(cid)) {


sched_mm_cid_migrate_to() is 4.x% and its annotation :

       │     dst_pcpu_cid = per_cpu_ptr(mm->pcpu_cid, cpu_of(dst_rq));
       │       mov     -0x30(%rbp),%rax
 54.53 │       mov     0x60(%r13),%rbx
 19.61 │       movslq  0xaf0(%rax),%r15

The reason why accessing mm->pcpu_cid is so costly is still a myth to
me...

BTW, I used below diff to mitigate the incorrect rq lock issue I
described in my reply to v8:

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index c6e2dd8f4ee3..f16418731866 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -11662,7 +11662,7 @@ void sched_mm_cid_migrate_to(struct rq *dst_rq, struct task_struct *t)
 		return;
 	}
 	/* Move src_cid to dst cpu. */
-	mm_cid_snapshot_time(mm);
+	mm_cid_snapshot_time(mm, cpu_of(dst_rq));
 	WRITE_ONCE(dst_pcpu_cid->cid, src_cid);
 }
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index d1d470441422..8b6a0c8ed3d1 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -3348,12 +3348,13 @@ static inline int __mm_cid_try_get(struct mm_struct *mm)
  * Save a snapshot of the current runqueue time of this cpu
  * with the per-cpu cid value, allowing to estimate how recently it was used.
  */
-static inline void mm_cid_snapshot_time(struct mm_struct *mm)
+static inline void mm_cid_snapshot_time(struct mm_struct *mm, int cpu)
 {
-	struct rq *rq = this_rq();
+	struct mm_cid *pcpu_cid = per_cpu_ptr(mm->pcpu_cid, cpu);
+	struct rq *rq = cpu_rq(cpu);
 
 	lockdep_assert_rq_held(rq);
-	__this_cpu_write(mm->pcpu_cid->time, rq->clock);
+	WRITE_ONCE(pcpu_cid->time, rq->clock);
 }
 
 static inline int __mm_cid_get(struct mm_struct *mm)
@@ -3404,7 +3405,7 @@ static inline int __mm_cid_get(struct mm_struct *mm)
 unlock:
 	raw_spin_unlock(&cid_lock);
 end:
-	mm_cid_snapshot_time(mm);
+	mm_cid_snapshot_time(mm, raw_smp_processor_id());
 	return cid;
 }
 
@@ -3419,7 +3420,7 @@ static inline int mm_cid_get(struct mm_struct *mm)
 	cpumask = mm_cidmask(mm);
 	cid = __this_cpu_read(pcpu_cid->cid);
 	if (mm_cid_is_valid(cid)) {
-		mm_cid_snapshot_time(mm);
+		mm_cid_snapshot_time(mm, raw_smp_processor_id());
 		return cid;
 	}
 	if (mm_cid_is_lazy_put(cid)) {
@@ -3467,7 +3468,7 @@ static inline void switch_mm_cid(struct task_struct *prev,
 		 */
 	}
 	if (prev->mm_cid_active) {
-		mm_cid_snapshot_time(prev->mm);
+		mm_cid_snapshot_time(prev->mm, raw_smp_processor_id());
 		mm_cid_put_lazy(prev);
 		prev->mm_cid = -1;
 	}