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:   Sat, 1 Oct 2022 01:26:25 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     K Prateek Nayak <kprateek.nayak@amd.com>
CC:     "Gautham R. Shenoy" <gautham.shenoy@amd.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        "Tim Chen" <tim.c.chen@intel.com>,
        Mel Gorman <mgorman@techsingularity.net>,
        "Juri Lelli" <juri.lelli@redhat.com>,
        Rik van Riel <riel@surriel.com>,
        Aaron Lu <aaron.lu@intel.com>,
        Abel Wu <wuyun.abel@bytedance.com>,
        Yicong Yang <yangyicong@hisilicon.com>,
        Ingo Molnar <mingo@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Valentin Schneider <vschneid@redhat.com>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH] sched/fair: Choose the CPU where short task is
 running during wake up
Message-ID: <YzcmwQCedKPLUsfi@chenyu5-mobl1>
References: <20220915165407.1776363-1-yu.c.chen@intel.com>
 <7975dcbe-97b3-7e6c-4697-5f316731c287@amd.com>
 <YzG5iAyJv3YCDKcp@BLR-5CG11610CF.amd.com>
 <6c626e8d-4133-00ba-a765-bafe08034517@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6c626e8d-4133-00ba-a765-bafe08034517@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?cHXc1RYJFNwg3X8alcVOscCqKwFOYSgNFDj9U+C/GdQ29A5lIjz/YgjH1VGS?=
 =?us-ascii?Q?qTvCHCzdr0uy1k+7t/fHx6mWwlxaUlFfHmwIEAXYzPuEfJR1mbmfsMFwbNCt?=
 =?us-ascii?Q?J76I23RMu9qB/grH08pg5V2NFK5diAl4ISqUuTVEbsBsomDJwQ7j0BaV0DHu?=
 =?us-ascii?Q?xCTym3i5g790pYoHI3rNsikMt7etjyJEX4eG9YiSdkjVWs67EhjZc/bDQFKa?=
 =?us-ascii?Q?Hed4rdIFi+u01AHo4xZzYkqfo2FJgs6lJ0KGfvaT6hEQ+dEKL8jqq7Nd32XE?=
 =?us-ascii?Q?K6UhRiJzwcu/LBc2yo4YRS57akYaDzjvohyAMdtpEGUtI3RrHPaBlSUm1zJX?=
 =?us-ascii?Q?hOTRPMTRzCE34O31w58LGhmxZF1IoBrVz2wVrsT3MMC1xALRIpZnyp9VnQlo?=
 =?us-ascii?Q?NV5VneKY3D5A9TW3lg8snOmLPrIXGfN0hLrKhcwqCF9uD698qGL6gprwC6Pi?=
 =?us-ascii?Q?vxO6PHea7x9X4WhauBLlC96kErCx4q6me7VZAmFV0+d0tCSaxJOa/BfwZkof?=
 =?us-ascii?Q?lJNJuzx59WR6Zs0ZZz7bKnBLPre780t1yLfDtwxutrekVDgr0AoRcELgCg6v?=
 =?us-ascii?Q?Opz6Z7aSyHDyC11QvExeXqvSF9nkHf190aWxdGBGil5oOWyF2G87Sl91kEON?=
 =?us-ascii?Q?xubLOx7lsTvOV0jMeQ+4KAKDWeddcm2jb0naspg+XJXx5m9ye4H9fXzdh8Xj?=
 =?us-ascii?Q?AMGRpgNwiEHEeSCjqoaResUac7hQpgJDGDS34HhmEcx1Mp0K0PglFRMPq74+?=
 =?us-ascii?Q?mbPPlPRI1/TKOTcED8o9kqE2JHnnKZwtbmdvZ8ZcbvHoPZaD6nmyNdwV90jh?=
 =?us-ascii?Q?PPGY/7gE6SXwhCpCyZV9QIoecscCkscMGYIQpR4JKjHGisszOhzErHqFePdG?=
 =?us-ascii?Q?f/LacfqoNLVV9Q9CFhI+eFMvpQQRk/ehkcknJnAD+rn5f2RwMW8Omt6UGpN3?=
 =?us-ascii?Q?e4B4Zzw2O+bLTqniHx4Lwxik0vaYC2+I+34miMD7xq+gbrNYahWFxK1lfq4u?=
 =?us-ascii?Q?ltDL15Qe1U4l9gjQwIfrbXD9sfFsd8Rx5jwZtuvzj0nwIK2CNFqmC2UW8kXg?=
 =?us-ascii?Q?mthj1DxbcJdSPG7ZqBIchXfJDltrMvpIngiqgzSlge1hnW1nDyVaXuLJQQV6?=
 =?us-ascii?Q?sHrXg8K5IdK4A8Z8O9KJszPdHF1sLBE8tRcd9iyC1R4pBEt0QmegqihbSjdd?=
 =?us-ascii?Q?UoBuRir7voW85+0N/UeNm4ECiuzts6q7hqcLliSKJvFM8DN+qzA7PiSqftAo?=
 =?us-ascii?Q?OxojKykdkDzsG9C840WNJJja4Uwn3AYLxpuS5mqdWZDRx+gXKTuPXrqzOz5C?=
 =?us-ascii?Q?uVww0PEjHnn/yYUz+yy2XT0Opu5n9fSGr0JCezaP17IGKPC2AnzBzNCcp5vh?=
 =?us-ascii?Q?EWsdv3Ui+RHCKxJLMUxVtJLugNGSQjcYDqRBB1bm7FraiZ+1sUJH7A4uIET1?=
 =?us-ascii?Q?z+sp/9csfPrtO/Utkz/ua0rYIq0LMLVm1l/wuhkn9tsNLG5e5v+LqyvxdVp7?=
 =?us-ascii?Q?aVdQpWCEhsZR5Uc+G96nbb4hwqRFQltTEi9PxEU673RGcamEMDW/OFx8HJRO?=
 =?us-ascii?Q?V/pbvh0dva36Ewuv0hGQ2Lp4u6E1w3wZriYKgkyN?=
X-MS-Exchange-CrossTenant-Network-Message-Id: dad92700-72d5-433f-201e-08daa308f490
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Sep 2022 17:26:48.5904
 (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: Jq4Unh+Cf/zeclYiMm2fQaJwi4eeHzIexU95+Lt1V4cy0ogGWO8/LJkI54D8u8IO5SugJ/Bgr4rYSS4usqC/9Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5980
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 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 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

Hi Prateek,
On 2022-09-29 at 22:28:38 +0530, K Prateek Nayak wrote:
> Hello Gautham and Chenyu,
> 
> On 9/26/2022 8:09 PM, Gautham R. Shenoy wrote:
> > Hello Prateek,
> > 
> > On Mon, Sep 26, 2022 at 11:20:16AM +0530, K Prateek Nayak wrote:[
> > 
> > [..snip..]
> > 
> >>> @@ -6050,7 +6063,8 @@ wake_affine_idle(int this_cpu, int prev_cpu, int sync)
> >>>  	if (available_idle_cpu(this_cpu) && cpus_share_cache(this_cpu, prev_cpu))
> >>>  		return available_idle_cpu(prev_cpu) ? prev_cpu : this_cpu;
> >>>  
> >>> -	if (sync && cpu_rq(this_cpu)->nr_running == 1)
> >>> +	if ((sync && cpu_rq(this_cpu)->nr_running == 1) ||
> >>> +	    is_short_task(cpu_curr(this_cpu)))
> >>
> >> This change seems to optimize for affine wakeup which benefits
> >> tasks with producer-consumer pattern but is not ideal for Stream.
> >> Currently the logic ends will do an affine wakeup even if sync
> >> flag is not set:
> >>
> >>           stream-4135    [029] d..2.   353.580953: sched_waking: comm=stream pid=4129 prio=120 target_cpu=082
> >>           stream-4135    [029] d..2.   353.580957: select_task_rq_fair: wake_affine_idle: Select this_cpu: sync(0) rq->nr_running(1) is_short_task(1)
> >>           stream-4135    [029] d..2.   353.580960: sched_migrate_task: comm=stream pid=4129 prio=120 orig_cpu=82 dest_cpu=30
> >>           <idle>-0       [030] dNh2.   353.580993: sched_wakeup: comm=stream pid=4129 prio=120 target_cpu=030
> >>
> >> I believe a consideration should be made for the sync flag when
> >> going for an affine wakeup. Also the check for short running could
> >> be at the end after checking if prev_cpu is an available_idle_cpu.
> > 
> > We need to check if moving the is_short_task() to a later point after
> > checking the availability of the previous CPU solve the problem for
> > the workloads which showed regressions on AMD EPYC systems.
> 
> I've done some testing with moving the condition check for short
> running task to the end of wake_affine_idle and checking if the
> length of run queue is 1 similar to what Tim suggested in the thread
> but doing it upfront in wake_affine_idle.
Thanks for the investigation. After a second thought, for will-it-scale
context_switch test case, all the tasks have SYNC flag, so I wonder if
putting the check to the end of wake_affine_idle() would make any
difference for will-it-scale test. Because will-it-scale might have
already returned this_cpu via 'if (sync && cpu_rq(this_cpu)->nr_running == 1)'
I'll do some test tomorrow on this.
> There are a few variations I've tested:
> 
> v1: move the check for short running task on current CPU to end of wake_affine_idle
> 
> v2: move the check for short running task on current CPU to end of wake_affine_idle
>     + remove entire hunk in select_idle_cpu
> 
> v3: move the check for short running task on current CPU to end of wake_affine_idle
>     + check if run queue of current CPU only has 1 task
> 
> v4: move the check for short running task on current CPU to end of wake_affine_idle
>     + check if run queue of current CPU only has 1 task
>     + remove entire hunk in select_idle_cpu
> 
> Adding diff for v3 below:
> --
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 0ad8e7183bf2..dad9bfb0248d 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6074,13 +6074,15 @@ wake_affine_idle(int this_cpu, int prev_cpu, int sync)
>  	if (available_idle_cpu(this_cpu) && cpus_share_cache(this_cpu, prev_cpu))
>  		return available_idle_cpu(prev_cpu) ? prev_cpu : this_cpu;
>  
> -	if ((sync && cpu_rq(this_cpu)->nr_running == 1) ||
> -	    is_short_task(cpu_curr(this_cpu)))
> +	if (sync && cpu_rq(this_cpu)->nr_running == 1)
>  		return this_cpu;
>  
>  	if (available_idle_cpu(prev_cpu))
>  		return prev_cpu;
>  
> +	if (cpu_rq(this_cpu)->nr_running == 1 && is_short_task(cpu_curr(this_cpu)))
> +		return this_cpu;
> +
I'm also thinking of adding this check in SIS and also the ttwu_pending flag
check in SIS.
>  	return nr_cpumask_bits;
>  }
>  
> --
>
[cut] 
> 
> We still see a pileup with v1 and v2 but not with v3 and v4 suggesting
> that the second hunk is not the reason for the pileup but rather
> choosing the current CPU in wake_affine_idle on the basis that the
> current running task is the short running task. To prevent a pileup, we
> must only choose the current rq if the short running task is the only
> task running there.
>
OK, I see. 

[cut]
> 
> A point to note is Stream is more sensitive initially when tasks have not
> run for long enough where, if a kworker or another short running task
> is running on the previous CPU during wakeup, the logic will favor an
> affine wakeup as initially as scheduler might not realize Stream is a
> long running task.
Maybe we can add restriction that only after the task has run for a while
we start the short_task() check?
> 
> Let me know if you would like me to gather more data on the test system
> for the modified kernels discussed above.
While waiting for Vincent's feedback, I'll refine the patch per your experiment
and modify the code in SIS per Tim's suggestion.

thanks,
Chenyu 
> --
> Thanks and Regards,
> Prateek