Received-SPF: pass (google.com: domain of linux-kernel+bounces-205412-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: Fri, 7 Jun 2024 13:07:26 +0800
From: Chen Yu <yu.c.chen@intel.com>
To: Chunxin Zang <spring.cxz@gmail.com>
CC: <mingo@redhat.com>, <peterz@infradead.org>, <juri.lelli@redhat.com>,
	<vincent.guittot@linaro.org>, <dietmar.eggemann@arm.com>,
	<rostedt@goodmis.org>, <bsegall@google.com>, <mgorman@suse.de>,
	<bristot@redhat.com>, <vschneid@redhat.com>, <jameshongleiwang@126.com>,
	<efault@gmx.de>, <kprateek.nayak@amd.com>, <linux-kernel@vger.kernel.org>,
	<yangchen11@lixiang.com>, <zhouchunhua@lixiang.com>,
	<zangchunxin@lixiang.com>
Subject: Re: [PATCH v2] sched/fair: Reschedule the cfs_rq when current is
 ineligible
Message-ID: <ZmKVjmuC4kGrUH5V@chenyu5-mobl2>
References: <20240529141806.16029-1-spring.cxz@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240529141806.16029-1-spring.cxz@gmail.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?pB4vbJseY1Fjzg3zLg/HPxKrb+oTx/2B8zfDBj0muape19iYNuG6gWlkIB0K?=
 =?us-ascii?Q?zSvhgmYZE0ksxGPIjPLnMbVFbDVZurvYm5846do+5b1iUQRebfs90HaxNazH?=
 =?us-ascii?Q?Z6uTU4TTBJ9WW5AzhlJSIgQCDhRm1yUy0pTqasYOFBw78s9ePDwpmFZdSr73?=
 =?us-ascii?Q?JvzCGfMd1Q0yVZt9tgeU12Yd39hgRJlfiBnolEqJo7zS2uC9Iistq8/KrwML?=
 =?us-ascii?Q?6AzfvZ6ZY8ts+5M4vdYV3BTry/VB6YNIGthONlXtSOJnylnQmSKX6+NGCmsL?=
 =?us-ascii?Q?fsfawYDA7O479TDTzwDudNq6CnTQdc5QLww9FitubAf4GhqKORX3HHSMmLzU?=
 =?us-ascii?Q?MqBC3zdAXgcNMXdW+mzCHmHgbzIVF4/qoGyp7k1S9P4tnSCnrtiy87Md6kTk?=
 =?us-ascii?Q?ChoF1mG3Hb4k9YluP6+aCiisPdt7pEbprvwCzeMEm0bbh60i73qqGaKK9vGz?=
 =?us-ascii?Q?0ftQIs1r50k11lzPoam2WMOHrTPa/1RlPp45hgceApKWffyvzrg0qE+WZ37f?=
 =?us-ascii?Q?GEiglkiVcfROYmuiy/eNx/4dhEYaTZgIu/VTV9kKP0hAjVK+Ft0PYQJ5eEm6?=
 =?us-ascii?Q?BrcFYtwd0RfNkx3CwJjjHnZyA5m1tyQJ9lNTqVc7WsEutvHKZQPU49pfAPBv?=
 =?us-ascii?Q?BJ1K8V2hZGksN3mId4WTeKajEI3aYuswLKHuMMD2sED8uFp/5htZTt8AnNEK?=
 =?us-ascii?Q?PIRlNzJdNxzOOGo0EdvS1sIFn9KXuybqYGDI5QuaVHpbfdUa6S5HHFAdAl42?=
 =?us-ascii?Q?E8RS9ceHp3Ho4ZdGVRXLnapBKfpCRUZikz50DEjq7fzEZRHQIcxJIFs1NjuV?=
 =?us-ascii?Q?mvaMbdcFacTY3kUDyysQxne6CLDCVDs+zlpEf16FfiO7+GFjLHfCWSrqqRPN?=
 =?us-ascii?Q?3nwF3mOkVISEi86ZiqmClS+otsCQE0vgAYKphklQ4LJDb1gBOtmXZ5ZKIQDQ?=
 =?us-ascii?Q?4mZdRKiwleERGDNbuwmKKCF2m7ubXMOqH1ogqAL2MjdcmHjdUWpCEWCoo+mJ?=
 =?us-ascii?Q?kJREIeE2dtAoW/R7/Q9ZyRJTJ165hq6TpVW8HXJah9opDvMt91xl0I5bK1yn?=
 =?us-ascii?Q?UO5nvO7azlZevS1ojP2w/gF+g+sbIeFE98MiCyB9hg0CKzYg/80hgMmNK8Lm?=
 =?us-ascii?Q?qVbAjuE9z0u/Rqqq8Kx3vonKP05BLLRg8IDgV9YFVb3E6gQRtcmuFezP+xwZ?=
 =?us-ascii?Q?tblqNLPSFMMCeN0+GmbmHZ6KzJKGHAgmYTHHAhgRur6O2pDnQRlJfYUDR2JH?=
 =?us-ascii?Q?5zRcG9BI39llDHsHE8AXMLMYtEJKuKPRlqfkBVWlUbVK/Y1dBQ5mresDybdM?=
 =?us-ascii?Q?S70tZssSphYsnSTlVEOiYMHE8rWrM3wNPnf0SqNLKORu8TcqLigloruwVGI7?=
 =?us-ascii?Q?pDy1SCmdFpFl0yaXCh9zorL6N3ORn+Pc7TxaDNZtE+9MLzTj/Y4rAxwQ8oOj?=
 =?us-ascii?Q?g2RVN4nVuiCvkQ1o9vxb/NAaFfLyeW70V3gVJJ40LlaIMgcnt5ZCCs/2DuAm?=
 =?us-ascii?Q?TjlMFc8gEmAh9CLQmv0XNcjiDJUtViXDCvz4R2YW05ZZulpRCwQR4lcejkXG?=
 =?us-ascii?Q?/BTfY2H2Q2CYsHRhS7xAAti0TR73b0repIlv6jJy?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6a8fb83f-efb3-4436-b595-08dc86afc1e2
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6020.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jun 2024 05:07:41.1129
 (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: kL0oSyOWTA4FzV18cPtFygsqGBYakpuWzDAGFWFloMfDuA6W3lc9Y7FHKIkdpbvKw9L+wInErPzxmZS7QFMbTA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4887
X-OriginatorOrg: intel.com

On 2024-05-29 at 22:18:06 +0800, Chunxin Zang wrote:
> I found that some tasks have been running for a long enough time and
> have become illegal, but they are still not releasing the CPU. This
> will increase the scheduling delay of other processes. Therefore, I
> tried checking the current process in wakeup_preempt and entity_tick,
> and if it is illegal, reschedule that cfs queue.
> 
> When RUN_TO_PARITY is enabled, its behavior essentially remains
> consistent with the original process. When NO_RUN_TO_PARITY is enabled,
> some additional preemptions will be introduced, but not too many.
> 
> I have pasted some test results below.
> I isolated four cores for testing and ran hackbench in the background,
> and observed the test results of cyclictest.
> 
> hackbench -g 4 -l 100000000 &
> cyclictest --mlockall -D 5m -q
> 
>                                  EEVDF      PATCH  EEVDF-NO_PARITY  PATCH-NO_PARITY
> 
>                 # Min Latencies: 00006      00006      00006      00006
>   LNICE(-19)    # Avg Latencies: 00191      00133      00089      00066
>                 # Max Latencies: 15442      08466      14133      07713
> 
>                 # Min Latencies: 00006      00010      00006      00006
>   LNICE(0)      # Avg Latencies: 00466      00326      00289      00257
>                 # Max Latencies: 38917      13945      32665      17710
> 
>                 # Min Latencies: 00019      00053      00010      00013
>   LNICE(19)     # Avg Latencies: 37151      25852      18293      23035
>                 # Max Latencies: 2688299    4643635    426196     425708
> 
> I captured and compared the number of preempt occurrences in wakeup_preempt
> to see if it introduced any additional overhead.
> 
> Similarly, hackbench is used to stress the utilization of four cores to
> 100%, and the method for capturing the number of PREEMPT occurrences is
> referenced from [1].
> 
> schedstats                          EEVDF       PATCH   EEVDF-NO_PARITY  PATCH-NO_PARITY  CFS(6.5)
> .stats.check_preempt_count          5053054     5045388    5018589    5029585
> .stats.patch_preempt_count          -------     0020495    -------    0700670    -------
> .stats.need_preempt_count           0570520     0458947    3380513    3116966    1140821
> 
> From the above test results, there is a slight increase in the number of
> preempt occurrences in wakeup_preempt. However, the results vary with each
> test, and sometimes the difference is not that significant.
> 
> [1]: https://lore.kernel.org/all/20230816134059.GC982867@hirez.programming.kicks-ass.net/T/#m52057282ceb6203318be1ce9f835363de3bef5cb
> 
> Signed-off-by: Chunxin Zang <zangchunxin@lixiang.com>
> Reviewed-by: Chen Yang <yangchen11@lixiang.com>
> 
> ------
> Changes in v2:
> - Make the logic that determines the current process as ineligible and
>   triggers preemption effective only when NO_RUN_TO_PARITY is enabled.
> - Update the commit message
> ---
>  kernel/sched/fair.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 03be0d1330a6..fa2c512139e5 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -745,6 +745,17 @@ int entity_eligible(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  	return vruntime_eligible(cfs_rq, se->vruntime);
>  }
>  
> +static bool check_entity_need_preempt(struct cfs_rq *cfs_rq, struct sched_entity *se)
> +{
> +	if (sched_feat(RUN_TO_PARITY) && se->vlag != se->deadline)
> +		return true;

If I understand correctly, here it intends to check if the current se
has consumed its 1st slice after been picked at set_next_entity(), and if yes do a reschedule.
check_entity_need_preempt() is added at the end of entity_tick(), which could overwrite
the police to reschedule current: (entity_tick()->update_curr()->update_deadline()), only there
are more than 1 runnable tasks will the current be preempted, even if it has expired the 1st
requested slice.

> +
> +	if (!sched_feat(RUN_TO_PARITY) && !entity_eligible(cfs_rq, se))
> +		return true;
> +
> +	return false;
> +}
> +
>  static u64 __update_min_vruntime(struct cfs_rq *cfs_rq, u64 vruntime)
>  {
>  	u64 min_vruntime = cfs_rq->min_vruntime;
> @@ -5523,6 +5534,9 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
>  			hrtimer_active(&rq_of(cfs_rq)->hrtick_timer))
>  		return;
>  #endif
> +
> +	if (check_entity_need_preempt(cfs_rq, curr))
> +		resched_curr(rq_of(cfs_rq));
>  }
>  
>  
> @@ -8343,6 +8357,9 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>  	cfs_rq = cfs_rq_of(se);
>  	update_curr(cfs_rq);
>  
> +	if (check_entity_need_preempt(cfs_rq, se))
> +		goto preempt;
> +

As we changes the preemption policy for current in two places, the tick preemption and wakeup preemption,
do you have statistics that shows which one brings the most benefit?

thanks,
Chenyu