Received-SPF: pass (google.com: domain of linux-kernel+bounces-213359-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 v2] sched/fair: Reschedule the cfs_rq when current is
 ineligible
From: Chunxin Zang <spring.cxz@gmail.com>
In-Reply-To: <ZmryuDqw2fEhHNni@chenyu5-mobl2>
Date: Thu, 13 Jun 2024 21:46:13 +0800
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>
Content-Transfer-Encoding: quoted-printable
Message-Id: <CAE80459-55EE-4BE5-B41D-8F68337782A4@gmail.com>
References: <20240529141806.16029-1-spring.cxz@gmail.com>
 <ZmKVjmuC4kGrUH5V@chenyu5-mobl2>
 <2E6EB0D6-D913-4205-B7DD-35EF4FA25667@gmail.com>
 <Zmrd2hwqHB8RQjqB@chenyu5-mobl2>
 <112FECA8-5D21-406F-814C-ACBE63351CBB@gmail.com>
 <ZmryuDqw2fEhHNni@chenyu5-mobl2>
To: Chen Yu <yu.c.chen@intel.com>


> On Jun 13, 2024, at 21:23, Chen Yu <yu.c.chen@intel.com> wrote:
>=20
> On 2024-06-13 at 20:02:37 +0800, Chunxin Zang wrote:
>>=20
>>=20
>>> On Jun 13, 2024, at 19:54, Chen Yu <yu.c.chen@intel.com> wrote:
>>>=20
>>> On 2024-06-12 at 18:39:11 +0800, Chunxin Zang wrote:
>>>>=20
>>>>=20
>>>>> On Jun 7, 2024, at 13:07, Chen Yu <yu.c.chen@intel.com> wrote:
>>>>>=20
>>>>> On 2024-05-29 at 22:18:06 +0800, Chunxin Zang wrote:
>>>> 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?
>>>>=20
>>>=20
>>> I wonder if we can only take care of the NO_RUN_TO_PARITY case? =
Something like this,
>>>=20
>>>> 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);
>>>> }
>>>>=20
>>>> +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 <=3D 1 ||
>>>    !entity_eligible(cfs_rq, se))
>>> return false;
>>>=20
>>> return true;
>>>=20
>>> Thoughts?
>>>=20
>>=20
>> This does indeed look better. In that case, do I need to make the =
changes this way and send
>> out a version 3?
>=20
> If you mean the following changes, maybe we can continue the =
discussion here.
> This is just my 2 cents, not sure what others think of it. Anyway, I =
can launch some tests.

Since the latest changes are completely different from version 2, so I =
have just released version 3.
Additionally, my testing environment is limited, and it indeed requires =
more testing to verify the
benefits it can provide. If possible, could you conduct tests using =
version 3?
=
https://lore.kernel.org/lkml/20240613131437.9555-1-spring.cxz@gmail.com/T/=
#u

thanks=20
Chunxin

>=20
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8a5b1ae0aa55..c0fdb25f0695 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -744,6 +744,15 @@ int entity_eligible(struct cfs_rq *cfs_rq, struct =
sched_entity *se)
> return vruntime_eligible(cfs_rq, se->vruntime);
> }
>=20
> +static bool check_curr_preempt(struct cfs_rq *cfs_rq, struct =
sched_entity *curr)
> +{
> + if (sched_feat(RUN_TO_PARITY) || cfs_rq->nr_running <=3D 1 ||
> +    !entity_eligible(cfs_rq, curr))
> + return false;
> +
> + return true;
> +}
> +
> static u64 __update_min_vruntime(struct cfs_rq *cfs_rq, u64 vruntime)
> {
> u64 min_vruntime =3D cfs_rq->min_vruntime;
> @@ -5536,6 +5545,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_curr_preempt(cfs_rq, curr))
> + resched_curr(rq_of(cfs_rq));
> }
>=20
>=20
> @@ -8415,7 +8427,7 @@ static void check_preempt_wakeup_fair(struct rq =
*rq, struct task_struct *p, int
> /*
> * XXX pick_eevdf(cfs_rq) !=3D se ?
> */
> - if (pick_eevdf(cfs_rq) =3D=3D pse)
> + if (check_curr_preempt(cfs_rq, se) || pick_eevdf(cfs_rq) =3D=3D pse)
> goto preempt;
>=20
> return;
> --=20
> 2.25.1
>=20
>=20
>=20