Received-SPF: pass (google.com: domain of linux-kernel+bounces-191818-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Content-Type: text/plain;
	charset=us-ascii
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.700.6\))
Subject: Re: [PATCH] sched/fair: Reschedule the cfs_rq when current is
 ineligible
From: Chunxin Zang <spring.cxz@gmail.com>
In-Reply-To: <06649B84-DA1D-4360-B0C4-79C81A34BC08@gmail.com>
Date: Tue, 28 May 2024 14:41:34 +0800
Cc: 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,
 linux-kernel@vger.kernel.org,
 yangchen11@lixiang.com,
 Jerry Zhou <zhouchunhua@lixiang.com>,
 Chunxin Zang <zangchunxin@lixiang.com>,
 kprateek.nayak@amd.com
Content-Transfer-Encoding: quoted-printable
Message-Id: <A381B712-B0A2-4218-AAD4-956FB73D88CE@gmail.com>
References: <20240524134011.270861-1-spring.cxz@gmail.com>
 <ZlCyhDspcZQhxlNk@chenyu5-mobl2>
 <06649B84-DA1D-4360-B0C4-79C81A34BC08@gmail.com>
To: Chen Yu <yu.c.chen@intel.com>


> On May 28, 2024, at 10:42, Chunxin Zang <spring.cxz@gmail.com> wrote:
>=20
>>=20
>> On May 24, 2024, at 23:30, Chen Yu <yu.c.chen@intel.com> wrote:
>>=20
>> On 2024-05-24 at 21:40:11 +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.
>>>=20
>>> The modification can reduce the scheduling delay by about 30% when
>>> RUN_TO_PARITY is enabled.
>>> So far, it has been running well in my test environment, and I have
>>> pasted some test results below.
>>>=20
>>=20
>> Interesting, besides hackbench, I assume that you have workload in
>> real production environment that is sensitive to wakeup latency?
>=20
> Hi Chen
>=20
> Yes, my workload  are quite sensitive to wakeup latency .
>>=20
>>>=20
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index 03be0d1330a6..a0005d240db5 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -5523,6 +5523,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 (!entity_eligible(cfs_rq, curr))
>>> + resched_curr(rq_of(cfs_rq));
>>> }
>>>=20
>>=20
>> entity_tick() -> update_curr() -> update_deadline():
>> se->vruntime >=3D se->deadline ? resched_curr()
>> only current has expired its slice will it be scheduled out.
>>=20
>> So here you want to schedule current out if its lag becomes 0.
>>=20
>> In lastest sched/eevdf branch, it is controlled by two sched =
features:
>> RESPECT_SLICE: Inhibit preemption until the current task has =
exhausted it's slice.
>> RUN_TO_PARITY: Relax RESPECT_SLICE and only protect current until =
0-lag.
>> =
https://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git/commit/?h=
=3Dsched/eevdf&id=3De04f5454d68590a239092a700e9bbaf84270397c
>>=20
>> Maybe something like this can achieve your goal
>> if (sched_feat(RUN_TOPARITY) && !entity_eligible(cfs_rq, curr))
>> resched_curr
>>=20
>>>=20
>>> @@ -8325,6 +8328,9 @@ static void check_preempt_wakeup_fair(struct =
rq *rq, struct task_struct *p, int
>>> if (unlikely(p->policy !=3D SCHED_NORMAL) || =
!sched_feat(WAKEUP_PREEMPTION))
>>> return;
>>>=20
>>> + if (!entity_eligible(cfs_rq, se))
>>> + goto preempt;
>>> +
>>=20
>> Not sure if this is applicable, later in this function, pick_eevdf() =
checks
>> if the current is eligible, !entity_eligible(cfs_rq, curr), if not, =
curr will
>> be evicted. And this change does not consider the cgroup hierarchy.
>>=20
>> Besides, the check of current eligiblity can get false negative =
result,
>> if the enqueued entity has a positive lag. Prateek proposed to
>> remove the check of current's eligibility in pick_eevdf():
>> =
https://lore.kernel.org/lkml/20240325060226.1540-2-kprateek.nayak@amd.com/=

>=20
> Thank you for letting me know about Peter's latest updates and =
thoughts.
> Actually, the original intention of my modification was to minimize =
the
> traversal of the rb-tree as much as possible. For example, in the =
following
> scenario, if 'curr' is ineligible, the system would still traverse the =
rb-tree in
> 'pick_eevdf' to return an optimal 'se', and then trigger  =
'resched_curr'. After
> resched, the scheduler will call 'pick_eevdf' again, traversing the
> rb-tree once more. This ultimately results in the rb-tree being =
traversed
> twice. If it's possible to determine that 'curr' is ineligible within =
'wakeup_preempt'
> and directly trigger a 'resched', it would reduce the traversal of the =
rb-tree
> by one time.
>=20
>=20
> wakeup_preempt-> pick_eevdf                                      -> =
resched_curr
>                                                 |->'traverse the =
rb-tree'  |
> schedule->pick_eevdf
>                                   |->'traverse the rb-tree'
>=20
>=20
> Of course, this would break the semantics of RESPECT_SLICE as well as
> RUN_TO_PARITY. So, this might be considered a performance enhancement
> for scenarios without NO_RESPECT_SLICE/NO_RUN_TO_PARITY.
>=20
Sorry for the mistake. I mean it should be a performance enhancement for =
scenarios
with NO_RESPECT_SLICE/NO_RUN_TO_PARITY.

Maybe it should be like this

@@ -8325,6 +8328,9 @@ static void check_preempt_wakeup_fair(struct rq =
*rq, struct task_struct *p, int
if (unlikely(p->policy !=3D SCHED_NORMAL) || =
!sched_feat(WAKEUP_PREEMPTION))
return;

+ if (!sched_feat(RESPECT_SLICE) && !sched_feat(RUN_TO_PARITY) && =
!entity_eligible(cfs_rq, se))
+ 	goto preempt;
+

> thanks=20
> Chunxin
>=20
>=20
>> If I understand your requirement correctly, you want to reduce the =
wakeup
>> latency. There are some codes under developed by Peter, which could
>> customized task's wakeup latency via setting its slice:
>> https://lore.kernel.org/lkml/20240405110010.934104715@infradead.org/
>>=20
>> thanks,
>> Chenyu