Received-SPF: pass (google.com: domain of linux-kernel+bounces-83640-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 28 Feb 2024 00:45:08 +0800
From: Chen Yu <yu.c.chen@intel.com>
To: Shrikanth Hegde <sshegde@linux.ibm.com>
CC: <mingo@kernel.org>, <peterz@infradead.org>, <vincent.guittot@linaro.org>,
	<dietmar.eggemann@arm.com>, <linux-kernel@vger.kernel.org>,
	<nysal@linux.ibm.com>, <aboorvad@linux.ibm.com>, <srikar@linux.vnet.ibm.com>,
	<vschneid@redhat.com>, <pierre.gondois@arm.com>, <morten.rasmussen@arm.com>,
	<qyousef@layalina.io>
Subject: Re: [PATCH 1/2] sched/fair: Add EAS checks before updating
 overutilized
Message-ID: <Zd4RlJJfruTs4Kiu@chenyu5-mobl2>
References: <20240223150707.410417-1-sshegde@linux.ibm.com>
 <20240223150707.410417-2-sshegde@linux.ibm.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240223150707.410417-2-sshegde@linux.ibm.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?8Mhi+bNCQc10Grgde82oUlZ1zC5Hzs13CUaZUg7RxzOC98yay7m0QAE3EZyX?=
 =?us-ascii?Q?3DTG50FvEDp3gbXTtJNnz/Tfg+2IvvXYatx1zkBBKDM5VVPzAm32/38X7hXL?=
 =?us-ascii?Q?KlRSNHmQIowv5i7LsYem1VxEpumMkRYCnf0IkMUBUORxShNU222nF7kMW1Dn?=
 =?us-ascii?Q?Zt24h/9/MUnNbyuyHRmwIgQhtv9teHvu/xRSM/p9p7+AwVndtgB0+9Ps8Ofw?=
 =?us-ascii?Q?bw1R0U+NJctD2Lu278VprZK7iVYW2O4AHT0rty9sOmqGrraGVLM2LVNVVohX?=
 =?us-ascii?Q?5JwnW/HuBbKfwEv9S043SkpUerL+5c98g+UoSE/oOl53aAT0Z/acoa1TlUxh?=
 =?us-ascii?Q?xk4aUck5sjvtzf9aUsnGu3FkPxItOzaJ5Zs/BMhe1a/MMAh6DnVceJegzS8Z?=
 =?us-ascii?Q?ZhDuwcb5q4MfKw/3OdGEljKq4PyG4beqWFXLQzsiQX2TJBt85s5ec6j7FXgX?=
 =?us-ascii?Q?bU8gYCUd/4ICZLDf2kE6j1mXdKsS1ya8ir04GZWBOQZv3L4jW6ylFReYsjZy?=
 =?us-ascii?Q?kkW9AFP9PtKWiFAcNN/+Uk0TATmtPSUS7usrHXGjyabA2sDNILFdAkUzg+hi?=
 =?us-ascii?Q?xKTHIgn2uFzAe0ky5dhBfgfDNtiCzTeZ0ONtLpAL4eeFIue6CvZ4mnRdO93o?=
 =?us-ascii?Q?wtGTZqsR4zDicAWeLb0shm12s6VDe/Ku6+s9vDVEbqzseTQVvqGhMDx+Q059?=
 =?us-ascii?Q?QQh7cFdNkXhOR6H9toL2Z3tUzDWZaC+623+t2g5kAjdINjEqxqbHTVnNfVV/?=
 =?us-ascii?Q?5EMHFz7upELN5IUxuicZkUGjOKthOwKDKL1trPgIO86p+QbE1j9X30HkCaEz?=
 =?us-ascii?Q?k1tYbqBm1CX7OtZbaPA0y1+UVcaF2kDU7AMTBBWRwnv+7f/NUsG9z+waakwW?=
 =?us-ascii?Q?17mCYKEvNjNSPGlGIswc52CWOgZgeClDqzkUyWss3Tiu7QoAne8Gf0UkzWsX?=
 =?us-ascii?Q?ft1tCADIqFyxJm6FjancFGK5VfgmVgb8GS+NAsfvb+nVrdebIMcuq6BY3yDo?=
 =?us-ascii?Q?jRHfG2T73GIAPpaJKvogJiHDebIHKP1wJDcgbiK1RxtkF9m5N46oQ3MLV5El?=
 =?us-ascii?Q?oHmm39Vr7Dea9RHCBWerVHDGGay+0uxOFRxssIBR/CQUx77Lht9U7aTePFt8?=
 =?us-ascii?Q?2AcZyRa01OeuF/rN+CfKOMrCdXC7VWEvfonPeZUTvMGAPHitC3f6xTfwZxlm?=
 =?us-ascii?Q?m8rN5sW9ywyTRnzAxMQFPn1hnDUy+qm9pzWsUOL8sU5Szjdaratre5MDjXTa?=
 =?us-ascii?Q?LdK/XlkWGOjD46NqW4qhm+s0a9sYDDNlL4k6PeCuN6LdLSKS1QnBk/o3gG8x?=
 =?us-ascii?Q?Qgm9ZHNq1Q8yBh0C+8H286OosTQ+wbgwSONzjvr5xYqK0BKdRAmBNIIibGRx?=
 =?us-ascii?Q?lfWMlHW97ne2rc8yboB0rQKtqCN2Cmd1q56VCh11G1+rw0OyWzhcUh5khnGj?=
 =?us-ascii?Q?YV9EC+F3hnZsKsnBkNya7EvfPuRo82WbnilwWmFmZRAv+imLG+4rec6IUedr?=
 =?us-ascii?Q?mWBCUEJADGpJGCGvfpT4w27Ev+q/PJlrfl7PnCDyd4957jtRE+OSe+6Hike6?=
 =?us-ascii?Q?RX77MBs+Y9ZRzLjDl6thIO53uZsK0Qe+KZQeDk0q?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b385a321-ee5d-49e9-d76b-08dc37b3806a
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6020.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Feb 2024 16:45:27.4416
 (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: pGzsJwjFpdm0iZAcFF4jlclmPzMy8r4Q82UhPHvVzAqhIOvshVkU2ycNqJRFoE9CBiN3ui7utUU1RBfxjNuWuw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5956
X-OriginatorOrg: intel.com

On 2024-02-23 at 20:37:06 +0530, Shrikanth Hegde wrote:
> Overutilized field of root domain is only used for EAS(energy aware scheduler)
> to decide whether to do regular load balance or EAS aware load balance. It
> is not used if EAS not possible.
> 
> Currently enqueue_task_fair and task_tick_fair accesses, sometime updates
> this field. In update_sd_lb_stats it is updated often.
> Which causes cache contention due to load/store tearing and burns
> a lot of cycles.

Looks like a typical cache false sharing: CPU1 updates the rd->overutilized,
which invalid the cache line when CPU2 access adjacent rd->overload.
This changes looks good to me, just some minor questions:

> Hence add EAS check before updating this field.
> EAS check is optimized at compile time or it is static branch.
> Hence it shouldn't cost much.
> 
> With the patch, both enqueue_task_fair and newidle_balance don't show
> up as hot routines in perf profile.
> 
> 6.8-rc4:
> 7.18%  swapper          [kernel.vmlinux]              [k] enqueue_task_fair
> 6.78%  s                [kernel.vmlinux]              [k] newidle_balance
> +patch:
> 0.14%  swapper          [kernel.vmlinux]              [k] enqueue_task_fair
> 0.00%  swapper          [kernel.vmlinux]              [k] newidle_balance
> 
> While here, Fix updating overutilized as either SG_OVERUTILIZED or 0
> instead. Current code can make it 0, 1 or 2. This shouldn't alter the
> functionality.

Just wonder where 1 comes from? In current code we either write SG_OVERUTILIZED
or sg_status & SG_OVERUTILIZED.

> 
> Fixes: 2802bf3cd936 ("sched/fair: Add over-utilization/tipping point indicator")
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>  kernel/sched/fair.c | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8e30e2bb77a0..9529d9ef2c5b 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6670,15 +6670,30 @@ static inline bool cpu_overutilized(int cpu)
>  	return !util_fits_cpu(cpu_util_cfs(cpu), rq_util_min, rq_util_max, cpu);
>  }
> 
> -static inline void update_overutilized_status(struct rq *rq)
> +static inline void update_rd_overutilized_status(struct root_domain *rd,
> +						 int status)
>  {
> -	if (!READ_ONCE(rq->rd->overutilized) && cpu_overutilized(rq->cpu)) {
> -		WRITE_ONCE(rq->rd->overutilized, SG_OVERUTILIZED);
> -		trace_sched_overutilized_tp(rq->rd, SG_OVERUTILIZED);
> +	if (sched_energy_enabled()) {
> +		WRITE_ONCE(rd->overutilized, status);
> +		trace_sched_overutilized_tp(rd, !!status);

Is this !!status intentional? The original one is SG_OVERUTILIZED = 2,
now it is either 0 or 1.

thanks,
Chenyu