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, 18 May 2023 12:17:01 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     K Prateek Nayak <kprateek.nayak@amd.com>
CC:     Mike Galbraith <efault@gmx.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Mel Gorman <mgorman@techsingularity.net>,
        Tim Chen <tim.c.chen@intel.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Abel Wu <wuyun.abel@bytedance.com>,
        Yicong Yang <yangyicong@hisilicon.com>,
        "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
        Len Brown <len.brown@intel.com>,
        Chen Yu <yu.chen.surf@gmail.com>,
        Arjan Van De Ven <arjan.van.de.ven@intel.com>,
        Aaron Lu <aaron.lu@intel.com>, Barry Song <baohua@kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH] sched/fair: Introduce SIS_PAIR to wakeup task on
 local idle core first
Message-ID: <ZGWmveL2YTiXp2XR@chenyu5-mobl1>
References: <20230516011159.4552-1-yu.c.chen@intel.com>
 <19664c68f77f5b23a86e5636a17ad2cbfa073f78.camel@gmx.de>
 <ZGNBt7vWJ3fDs5Sc@chenyu5-mobl1>
 <795a6d9475ecb444d219a9d36dc93b48a69e960e.camel@gmx.de>
 <ZGUHa+Si4dJbdsZN@chenyu5-mobl1>
 <0ac968e3-cd80-6339-970d-37005876b145@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <0ac968e3-cd80-6339-970d-37005876b145@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?AycOTBc5E6a9l/CDoLvJqgXdbtzwlJxNlXEyYLEE5IfSXJkL5alINSZFAAGX?=
 =?us-ascii?Q?Od6FM02+wbbvgbB1PAnBHgqS0BF04q1NlunKGuGRKppdmuUHkN8vTWA6Nkcg?=
 =?us-ascii?Q?Skctml1r7ndaXRcmjZhZIQzRVEYM5V7XJ8Sh1bNnSYs2Gzh62Jz/2ZNOMf37?=
 =?us-ascii?Q?Q9IWiSeyT/Q4lMPmzZUJYKTXyAiTRn/rPFcM2uq0lshiYo2YnZlR5MM04QIg?=
 =?us-ascii?Q?1rHAVTGdFVIpiAYMczTCgFG/rF3w8E8H3K4YqeJ07jJR3D6E28+dl+ymrcya?=
 =?us-ascii?Q?AMH7Ylw7DACU+kThXKXsPvhPDoWalNeTqvRXnnfFEjxrWWNQiXF3gxA1sIxV?=
 =?us-ascii?Q?qk9O6FDfDXkzpIcnG+tcGOsnW2220Tkxqg9KOMrNXth107LeitL8FKgrpKa+?=
 =?us-ascii?Q?VBwi4m+iGShuHd+ol7WWjvI/QNhKU67n3KTFotPjFaKwra92V5zLXwjdfrtz?=
 =?us-ascii?Q?k/AqLMxzg0RH5ue6gB76+WYhuMDTMhEiWLUadupQT+NQlm07VziRkJDN3xaw?=
 =?us-ascii?Q?P3DVs9+eGktZRNzL1zmAn4iCPC1UoAItvkMO37UlcK5SPvL5omdPuxKqghpF?=
 =?us-ascii?Q?ihkvt7fHqMrWbST/H/kCQ0LP6EePeVa+U9CnY+FyepOcIDza6MteAdJF1jsY?=
 =?us-ascii?Q?3xWl4FAgvdSqJHsqCs3ajVRCLxB0o4a863CSFcNUcOsEhmUyzpfw6ilpt7ra?=
 =?us-ascii?Q?bTIoP4m/BojfFHZDdTtuEvab7ib7YHc3wr3wSKB6GmbAqJLhDVAxMghV5hjn?=
 =?us-ascii?Q?YNMs5m8eMhw5b09DiTDVf74/lRjzycvVDvENt9Eesf/Eeb3AxNEfz6Qwy8TE?=
 =?us-ascii?Q?MYzurqxFHLsEQGE8P5Eajgi65CZfiL4Osw35NYOsWsvqu+8q9grjAjQkwgtp?=
 =?us-ascii?Q?sNEVpo4zLdjSxhMc53+MYI8Jq4saRN3dUgjs+SavFtx9qg7MB4AMu8CB5Kt+?=
 =?us-ascii?Q?XSB6xybBXjgvz44NI2rqjQHpOdm+Lwx19qGIXs3yebev7Yxx7AaQT9GE86x8?=
 =?us-ascii?Q?B31+xrOlyFwxI+13kwU1MtaGn+LbMG7GWkenbB/1yUlf+pclKMhfIXE7lHaU?=
 =?us-ascii?Q?VnJ4Jr39Agctqs3ujw4ZBO+xPeOERgj5Hcm3loQX2PRFlHvrIydz6Rtlp+aA?=
 =?us-ascii?Q?ssm42xFmGnJYFgJwPSSx8SEvxyvYn50Ti/lesx2htU2OR4DDegpklk5pbD0w?=
 =?us-ascii?Q?HYDl5U5tkTekQSyQ4s3CbdOyK69x7oLHkRNbzofWziZSdjFJFehqPKmkRRC+?=
 =?us-ascii?Q?0LdrKEKhbyvgqOW/962CWABELXI1aS6NQO5sxt9GMmqwiLxjVHw5zIRFTENP?=
 =?us-ascii?Q?dzXNcBvYodM2ErEzBocrLAkCGal8KcmMK24b3wmJHKsPS1ECpYyFvTWUI4ob?=
 =?us-ascii?Q?N1XqYZ0/m/VMYEGpPN6Gmdbj47ffCpWEJ47I3rC0c7rBpm4jR6sRDjKErQYa?=
 =?us-ascii?Q?fRGHb2bJuyAzTVgcYScsJ/rhw6oQUh3+BPrBRJddjRSaWHL7A7od3IN3b5Cn?=
 =?us-ascii?Q?Rtsb5EyrazDZ+Fug4iMyPUA9yoUR1e9GEPA6AIWOa6GnDgNpL0CLFu8FLHMo?=
 =?us-ascii?Q?MEQaJiZRVuRqiSH/Tc5ZkYB8j6vrW+7WEWW/GZJe?=
X-MS-Exchange-CrossTenant-Network-Message-Id: cfb74f2f-0c82-46c7-fdf1-08db5756c7cd
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 May 2023 04:17:23.6568
 (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: 7yMxlnxuWcIjj+m9ERD3yWcK/SzNH9vXkrWjdpkyVsfEoOW/xVnhCZsmTDJ30dzStGn3KLXNjfC1lOTI0b0vcw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB8239
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

Hi Prateek,
On 2023-05-18 at 09:00:38 +0530, K Prateek Nayak wrote:
> Hello Chenyu,
> 
> I'll do some light testing with some benchmarks and share results on the
> thread but meanwhile I have a few observations with the implementation.
>
Thanks for reviewing this change.
> On 5/17/2023 10:27 PM, Chen Yu wrote:
> > On 2023-05-16 at 13:51:26 +0200, Mike Galbraith wrote:
> >> On Tue, 2023-05-16 at 16:41 +0800, Chen Yu wrote:
> >> [..snip..]
> > 
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 7d2613ab392c..572d663065e3 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -7126,6 +7126,23 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
> >  	    asym_fits_cpu(task_util, util_min, util_max, target))
> >  		return target;
> >  
> > +	/*
> > +	 * If the waker and the wakee are good friends to each other,
> > +	 * putting them within the same SMT domain could reduce C2C
> > +	 * overhead. But this only applies when there is no idle core
> > +	 * available. SMT idle sibling should be prefered to wakee's
> > +	 * previous CPU, because the latter could still have the risk of C2C
> > +	 * overhead.
> > +	 *
> > +	 */
> > +	if (sched_feat(SIS_PAIR) && sched_smt_active() && !has_idle_core &&
> 
> "has_idle_core" is not populated at this point and will always be false
> from the initialization. Should there be a:
> 
> 	has_idle_core = test_idle_cores(? /* Which CPU? */);
Yes you are right, I have 2 patches, the first one is to check has_idle_core
in the beginning but I forgot to send it out but only the second one.
> 	if (sched_feat(SIS_PAIR) ...) {
> 		...
> 	}
> 	has_idle_core = false;
> 
> ?: "has_idle_core" is currently used in select_idle_sibling() from the
> perspective of the target MC. Does switching target to current core
> (which may not be on the same MC) if target MC does not have an idle core
> make sense in all scenarios?
Right, we should check whether target equals to current CPU. Since I tested
with 1 socket online, this issue did not expose
> 
> > +	    current->last_wakee == p && p->last_wakee == current) {
> > +		i = select_idle_smt(p, smp_processor_id());
> 
> Also wondering if asym_fits_cpu() check is needed in some way here.
> Consider a case where waker is on a weaker capacity CPU but wakee
> previously ran on a stronger capacity CPU. It might be worthwhile
> to wake the wakee on previous CPU if the current CPU does not fit
> the task's utilization and move the pair to the CPU with larger
> capacity during the next wakeup. wake_affine_weight() would select
> a target based on load and capacity consideration but here we
> switch the wakeup target to a thread on the current core.
> 
> Wondering if the capacity details already considered in the path?
> 
Good point, I guess what you mean is that, target could be other CPU rather than
the current one, there should be a check if the target equals to current CPU.
Let me refine the patch and have a test.

thanks,
Chenyu
> > +
> > +		if ((unsigned int)i < nr_cpumask_bits)
> > +			return i;
> > +	}
> > +
> >  	/*
> >  	 * If the previous CPU is cache affine and idle, don't be stupid:
> >  	 */
> > diff --git a/kernel/sched/features.h b/kernel/sched/features.h
> > index ee7f23c76bd3..86b5c4f16199 100644
> > --- a/kernel/sched/features.h
> > +++ b/kernel/sched/features.h
> > @@ -62,6 +62,7 @@ SCHED_FEAT(TTWU_QUEUE, true)
> >   */
> >  SCHED_FEAT(SIS_PROP, false)
> >  SCHED_FEAT(SIS_UTIL, true)
> > +SCHED_FEAT(SIS_PAIR, true)
> >  
> >  /*
> >   * Issue a WARN when we do multiple update_rq_clock() calls
> 
> --
> Thanks and Regards,
> Prateek