Received-SPF: pass (google.com: domain of linux-kernel+bounces-213149-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 13 Jun 2024 19:54:02 +0800
From: Chen Yu <yu.c.chen@intel.com>
To: Chunxin Zang <spring.cxz@gmail.com>
CC: Honglei Wang <jameshongleiwang@126.com>, <mingo@redhat.com>, "Peter
 Zijlstra" <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>, Mike Galbraith <efault@gmx.de>,
	K Prateek Nayak <kprateek.nayak@amd.com>, <linux-kernel@vger.kernel.org>,
	<yangchen11@lixiang.com>, Jerry Zhou <zhouchunhua@lixiang.com>, Chunxin Zang
	<zangchunxin@lixiang.com>
Subject: Re: [PATCH v2] sched/fair: Reschedule the cfs_rq when current is
 ineligible
Message-ID: <Zmrd2hwqHB8RQjqB@chenyu5-mobl2>
References: <20240529141806.16029-1-spring.cxz@gmail.com>
 <ZmKVjmuC4kGrUH5V@chenyu5-mobl2>
 <2E6EB0D6-D913-4205-B7DD-35EF4FA25667@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <2E6EB0D6-D913-4205-B7DD-35EF4FA25667@gmail.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?7KKzsznY1jRzq87vB7d/fn3lgGchaId/efLPfwxPbFRvGWUXeBzrZSZMK3M9?=
 =?us-ascii?Q?hqYBoL1AgtuNfrOKcQF1nUd7E64KwwZiiuFsz71susJGOUTyVli2/6COD6HA?=
 =?us-ascii?Q?YyqHKhFTrlQPWqunMUgj85iQYjTcW17q6NcBX3kKsLWyLpzawof+pMX5Oa9M?=
 =?us-ascii?Q?j2HbNqInV2jxx4B8a0zu+cOeFDyVj0bubeR/U4KgogbXUgHb3l3UT1jpol3z?=
 =?us-ascii?Q?+Wwe6xAK1Olu2kiUv9WfysFvzptwQklZkvMqi448ih9PYDx8rA06Y5hpy/ZX?=
 =?us-ascii?Q?UINUW9coa/zhxjtu0triPRHzrFPfybtlxcXa4eOfFa7JvGwRexydEsGkzj1a?=
 =?us-ascii?Q?SrNTm0TVBPKAoZKdTWBSr3OyX8MlaUWMIwM2/eUOW+UDlCo1aRUdFbmqpWxn?=
 =?us-ascii?Q?xa7OvMmR3O1jIvzCtYJ4Q5iA4DIxx3DPkYrK61Ffee3DuKsA4BmgkG41DL3B?=
 =?us-ascii?Q?Ij2nD6w4iRhdEzV2W9Eqm2enWEpZy2r1iSW6dU0M9R3McD2MwuqZKxYS5/Lm?=
 =?us-ascii?Q?TCbAd2Wo+3CW9P7F9GcZuZ771sOqGFaBWQuxvqTxueK/YqqgPSTR/ttB0d0k?=
 =?us-ascii?Q?JC+eWRbvQJjqOoRd0O4luOHYBlVXku6S8EIl6JjtzfnV/PamMh9rTYy0txuw?=
 =?us-ascii?Q?bkV6zphhD8QLhSzRUavOYT8tV+ISQGp3esmepJ/BZ3NJ6RJVmhXcUlKRjMRW?=
 =?us-ascii?Q?hNgAgkrIXTB/Wu3f4W5MKH6NJ2Bmxsx/Jz+peFe7yXj+Nxm3M3xBNL6rnuYN?=
 =?us-ascii?Q?dagbPj89riQd7n1CjGpFrW7WdHezIcUvJtjq+DDYECEmBa4ANN0Dn4/bkpPI?=
 =?us-ascii?Q?QO/M8xIb9Ji2X0IUZACZLapeELuraCtzNHXCH+4U1vkLZMRM4Q5VBJkLo9jL?=
 =?us-ascii?Q?UR9vzwo6f56mWv7wli0CI3UVPyfR0uI6ZWoUDF+s2zibJPdoE5iwkwGLfCKl?=
 =?us-ascii?Q?n6wKG5GiBpz0MyNDErLMrzSKZePY8z3YEW/gG2M9w6+AJyHBgsZMZIjw+wYR?=
 =?us-ascii?Q?yLWN4D07OEnxXgRQCWObA+6VZYHvXmTOyyjQt6bXLudmcujzDz/Z7WDEH7AN?=
 =?us-ascii?Q?8RT8oe5J0blXL50I+iCKgw+ecL1JQDfrFq79r6b20tAFh7Iha58HGT4X4loe?=
 =?us-ascii?Q?r9DOM8XJDoFp12R4rVsKSRHhqgm3lCbGoAJog84y70oDIa59dN2JT5lHfY8h?=
 =?us-ascii?Q?S7hf3I4LXx4rJwHMfK52QU7V++7Tcc3cc6gXr7nqKyMV2tuCQ/cnQxOWaSdx?=
 =?us-ascii?Q?k3mqew9VUGwFXeNRwPsGPc2VHrWSg5mioJJSs5wE/McG7dT2RvTimh0VVMrO?=
 =?us-ascii?Q?JD6xUbCuagEZBXCKcr7Z6u7G85KiCHOXwArB+zJiQIna6mu4EpMXifWUHF+p?=
 =?us-ascii?Q?Giey08Ce9IA4hw2GT7/f6Vj8NV7s4l5oVJ4ese6pxhDhTZ4ozOfScHYEQmai?=
 =?us-ascii?Q?3qen+zl78zenz+5AE0njVXneSU1p4Ma76y01+JxGpaMBVGlaJgWmkMvX5Wad?=
 =?us-ascii?Q?QUYGMrhZGWL67isUo5lgH0HO72vvDZhmtgEjWNbRr0zQB9Y2phnxXQoit6wy?=
 =?us-ascii?Q?jBhjiIcXCzTEdTRS+Lqsjy1Wbwj6i64HqOdeOTd4?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 03757dc9-7b64-4ed5-1c05-08dc8b9f8d68
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6020.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jun 2024 11:54:16.9554
 (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: ckOysxQ1B6zsFRaBMIR9oL2clCQ+PJIynaOuI6OkVIMWHjbDeKLpQsUNPas9DQW0ovL31/pLk6/a0+yyjnSs4w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB4988
X-OriginatorOrg: intel.com

On 2024-06-12 at 18:39:11 +0800, Chunxin Zang wrote:
> 
> 
> > On Jun 7, 2024, at 13:07, Chen Yu <yu.c.chen@intel.com> wrote:
> > 
> > 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.
> > 
> 
> The purpose of the modification is to increase preemption opportunities without breaking the
> RUN_TO_PARITY rule. However, it clearly introduces some additional preemptions, or perhaps
> there should be a check for the eligibility of the se. Also, to avoid overwriting the scheduling
> strategy in entity_tick, would a modification like the following be more appropriate?
>

I wonder if we can only take care of the NO_RUN_TO_PARITY case? Something like this,
 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 03be0d1330a6..5e49a15bbdd3 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -745,6 +745,21 @@ 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) || cfs_rq->nr_running <= 1 ||
	    !entity_eligible(cfs_rq, se))
		return false;

	return true;

Thoughts?

thanks,
Chenyu

> +       if (cfs_rq->nr_running <= 1)
> +               return false;
> +
> +       if (sched_feat(RUN_TO_PARITY) && se->vlag != se->deadline
> +                                     && !entity_eligible(cfs_rq, se))
> +               return true;
> +
> +       if (!sched_feat(RUN_TO_PARITY) && !entity_eligible(cfs_rq, se))
> +               return true;
> +
> +       return false;
> +}
> +