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, 13 Apr 2023 21:49:12 +0800
From:   Aaron Lu <aaron.lu@intel.com>
To:     Peter Zijlstra <peterz@infradead.org>
CC:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        <linux-kernel@vger.kernel.org>, Olivier Dion <odion@efficios.com>,
        <michael.christie@oracle.com>
Subject: Re: [RFC PATCH v4] sched: Fix performance regression introduced by
 mm_cid
Message-ID: <20230413134912.GB214119@ziqianlu-desk2>
References: <20230410150150.2179062-1-mathieu.desnoyers@efficios.com>
 <20230411045225.GA3509@ziqianlu-desk2>
 <20230411131221.GA7356@ziqianlu-desk2>
 <20230412091043.GC4253@hirez.programming.kicks-ass.net>
 <20230412114240.GA155547@ziqianlu-desk2>
 <20230412142616.GI628377@hirez.programming.kicks-ass.net>
 <20230412143934.GB162902@ziqianlu-desk2>
 <20230413111047.GB83892@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230413111047.GB83892@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?OyXSsVnXT/yIMJ9ZL0WwNplYADFbE5VcF5/wi1M9BmQLxDzRmXFlfEe9bti1?=
 =?us-ascii?Q?ZMCukqFjws78g2p13sCvUin4I6O9RJTs4Ebv+L5lRbR6Z8RNnXlELESPUJpE?=
 =?us-ascii?Q?wVg6jWrJWIjezBSLiPTmPo6v6Rb3FG1LZ/v1GbogbAEZYnBVkCGB2MrImuH1?=
 =?us-ascii?Q?fjpsrblyTsUBV9Ut9HF8WGSqH9oZqitiyhVE/k20AbvEn7YvAnhIYgTaGNmj?=
 =?us-ascii?Q?Fls4E3lzIMD4/fyX2eMLZiXYp/vylGMx3LrGQUHcfRMdnGD3iwRibWy8ftqk?=
 =?us-ascii?Q?s9PB+ZQRnW5qL9b//BqXUrrEjD8kgT8QK1u4IA57OjSNOiRxV2y3Mblz/la0?=
 =?us-ascii?Q?mzksHz7yHJkl0G072M92JW/XxVWKC4265xAu3rbOa8t7ZozJko07vIjM1vSA?=
 =?us-ascii?Q?SuvFRGuUY3TlHFG7o1U3wli93ZHwE0tuyUJcFzmTOvevcRnpbzjNhF2a5Kk5?=
 =?us-ascii?Q?dw1Yl/XYen5fTBU2fGrJaAoEBRqrF8XD4uMe55cyFJGEcj61S+PC3rHuZUgE?=
 =?us-ascii?Q?oY5JVSY0SGB5NX4H8UQfuLjpz7UImYO+d6SizU7twnLmSPUOZ812m/bfU4lC?=
 =?us-ascii?Q?Ak9x5VJ8PaAJN8cU0y9k+vwq22z/p/A0VG5dG9giHDHaBl1f4Bxnccw9kS7M?=
 =?us-ascii?Q?lAhaqilVHXgcxsdrSY5V49IM3uhVSFXclQEucHIU968AsVIdGbxSDNU44asr?=
 =?us-ascii?Q?P5Vhk44JJWCCUS8BcU6c6yfJxj8FimenfKJElP8vQrz3Qu5VYy4AWUHnEm4s?=
 =?us-ascii?Q?c2KWQjoQx04UzxDr8Ou2Pd4hQ9D+kh3HZ0RNBq28bbAYaljbcaJt7b1j4EIz?=
 =?us-ascii?Q?PxIr4GOFIgRH5edmm8N7mbFs91fcoOHAcr+Di3MtHpYOyOkK/uINPNXEvyod?=
 =?us-ascii?Q?X2xe1azopYBbhQfs0QvyKlRMeZocbxp4zEvEjq2H4NOEGi5LxeGx68nx0H0Q?=
 =?us-ascii?Q?6e2srGEJX6Pev1fKaROqCXS2s88+VSdFjwLtiArfLPpe6CMlQLCX1cD0TH5n?=
 =?us-ascii?Q?WFC3Sva+dDo0VDCWcHmfizUF4XEkoaZ1lfXYom+XTEIJLY4TE4ptU/UtWb71?=
 =?us-ascii?Q?WQ8/iA+IYaPkDdnM92WdHeQB7/BkwYo2Gze6Ags8wbXWJbEosY6P1gzLYIz9?=
 =?us-ascii?Q?OoxkjLaMFHryfmrnloKsDmvyD3T3iFzvYDLoxw8TMRgbGHLIVViWnIQZB4N3?=
 =?us-ascii?Q?iPW3hFRoDURGV2HsN10SGiYGpXX82DdroyTO2tJ0KhdaG4FWKNRhZrqOsyzf?=
 =?us-ascii?Q?/i8Bn62F5cbfClaPbdy/KIlF0aQ4SOUJk2+Wz0EPFQ/2VHGzpfLvHdT01Jg1?=
 =?us-ascii?Q?tDgfZjgGKMeaCrfVjkzuU3ftsNPPiWxe2LtmykLO6U0OVMFlGV8FAnpSxZyd?=
 =?us-ascii?Q?JMj4tJqUE5YJJlY/qTqlMxQAFWShpOUCuQ+zQtHwHx2gpOgV1RJ5UZ7KAf96?=
 =?us-ascii?Q?FJWAn1NgAixvQ5HAbezrOr1MZDi6jjalM7ZeE46urifMegH2GOlZKLHIOZdb?=
 =?us-ascii?Q?dPzryHyPXkt/cRCIHw9qo1chLfhcih2wdedHiCZ9VLI5Tangu4ddHlEpeoaG?=
 =?us-ascii?Q?LziqdLevq1J8SkxqryI/VQQFmS+20Rlx5sHmzPzT?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 33de9afa-ea95-45d4-6a46-08db3c25e31a
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3062.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Apr 2023 13:49:22.6535
 (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: kuXRX5Wj/WaNiEIrNoqv5AeDei1n/eB3L/NnQSksKD0mcIMlAyKRRi/BpWX52+H/BvZPVGXJ9MkBCtotQX6apg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4642
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,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        URIBL_BLOCKED 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 13, 2023 at 01:10:47PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 12, 2023 at 10:39:34PM +0800, Aaron Lu wrote:
> > On Wed, Apr 12, 2023 at 04:26:16PM +0200, Peter Zijlstra wrote:
> > > On Wed, Apr 12, 2023 at 07:42:40PM +0800, Aaron Lu wrote:
> > > 
> > > > I *guess* you might be able to see some contention with hackbench on
> > > > that HSW-EX system with v4.
> > > 
> > > Indeed! Notably it seems to be the wakeup from idle that trips it
> > > hardest:
> > 
> > Could it because for idle cpus, the per-cpu/mm cid is no longer valid for
> > the sake of compacting and when task wakes there, it will have to
> > re-allocate a new cid through mm_get_cid() which needs to acquire
> > mm->cid_lock?
> 
> Yup. And I'm thinking it is futile (and counter productive) to strive
> for compactness in this (nr_threads >= nr_cpus) case.
> 
> The below on v4 solves the contention I see with hackbench (which runs
> 400 threads which is well above the 144 cpu count on that machine).

I also tested on Icelake and SPR using hackbench and the contention is
also gone :-)

I then tested with sysbench_postgres, the contention is still there on
SPR:

    11.63%    11.62%  [kernel.vmlinux]        [k]     native_queued_spin_lock_slowpath
    7.91%     native_queued_spin_lock_slowpath;_raw_spin_lock;mm_cid_get;__schedule;schedule_idle;do_idle;cpu_startup_entry;start_secondary;secondary_startup_64_no_verify
    3.17%     native_queued_spin_lock_slowpath;_raw_spin_lock;mm_cid_get;__schedule;schedule;schedule_hrtimeout_range_clock;schedule_hrtimeout_range;do_epoll_wait;__x64_sys_epoll_wait;do_syscall_64;entry_SYSCALL_64;epoll_wait

> 
> This obviously leaves a problem with the nr_threads = nr_cpus - 1 case,
> but I'm thinking we can add some fuzz (nr_cpu_ids - ilog2(nr_cpus_ids+1)
> perhaps). Also, I would be thinking that's not something that typically
> happens.

Let me see if adding some fuzz helps here.

For sysbench_postgres, the server uses process model while the client
uses thread model and the client(sysbench) started nr_cpus threads so
it's likely didn't trigger the (nr_cpus-1) bar here. I doubt the bar
may have to be as low as 1/2 nr_cpus considering that some of the
cpus are running the server processes.

> 
> Mathieu, WDYT? -- other than that the patch is an obvious hack :-)
> 
> ---
>  include/linux/mm_types.h | 8 ++++++++
>  kernel/fork.c            | 4 +++-
>  kernel/sched/core.c      | 9 +++++++++
>  kernel/sched/sched.h     | 2 ++
>  4 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 4160ff5c6ebd..598d1b657afa 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -609,6 +609,7 @@ struct mm_struct {
>  		 * were being concurrently updated by the updaters.
>  		 */
>  		raw_spinlock_t cid_lock;
> +		int cid_saturated;
>  		/**
>  		 * @pcpu_cid: Per-cpu current cid.
>  		 *
> @@ -912,6 +913,12 @@ static inline int mm_cid_clear_lazy_put(int cid)
>  	return cid & ~MM_CID_LAZY_PUT;
>  }
>  
> +static inline void mm_cid_desaturate(struct mm_struct *mm)
> +{
> +	if (mm->cid_saturated && atomic_read(&mm->mm_users) < nr_cpu_ids)
> +		mm->cid_saturated = 0;
> +}
> +
>  /* Accessor for struct mm_struct's cidmask. */
>  static inline cpumask_t *mm_cidmask(struct mm_struct *mm)
>  {
> @@ -928,6 +935,7 @@ static inline void mm_init_cid(struct mm_struct *mm)
>  	int i;
>  
>  	raw_spin_lock_init(&mm->cid_lock);
> +	mm->cid_saturated = 0;
>  	for_each_possible_cpu(i)
>  		*per_cpu_ptr(mm->pcpu_cid, i) = MM_CID_UNSET;
>  	cpumask_clear(mm_cidmask(mm));
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 3832bea713c4..a5233e450435 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1233,7 +1233,9 @@ void mmput(struct mm_struct *mm)
>  	might_sleep();
>  
>  	if (atomic_dec_and_test(&mm->mm_users))
> -		__mmput(mm);
> +		return __mmput(mm);
> +
> +	mm_cid_desaturate(mm);
>  }
>  EXPORT_SYMBOL_GPL(mmput);
>  
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 425766cc1300..d5004d179531 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -11550,6 +11550,15 @@ void sched_mm_cid_migrate_from(struct task_struct *t)
>  	if (last_mm_cid == -1)
>  		return;
>  
> +	/*
> +	 * When nr_threads > nr_cpus, there is no point in moving anything
> +	 * around to keep it compact.
> +	 */
> +	if (mm->cid_saturated) {
> +		t->last_mm_cid = -1;
> +		return;
> +	}
> +
>  	src_rq = task_rq(t);
>  	src_pcpu_cid = per_cpu_ptr(mm->pcpu_cid, cpu_of(src_rq));
>  	src_cid = READ_ONCE(*src_pcpu_cid);
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index f3e7dc2cd1cc..6c4af2992e79 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3347,6 +3347,8 @@ static inline int mm_cid_get(struct mm_struct *mm)
>  	}
>  	raw_spin_lock(&mm->cid_lock);
>  	cid = __mm_cid_get_locked(mm);
> +	if (cid == nr_cpu_ids - 1)
> +		mm->cid_saturated = 1;
>  	raw_spin_unlock(&mm->cid_lock);
>  	WRITE_ONCE(*pcpu_cid, cid);
>  	return cid;