linux-kernel - Re: [PATCH v2] sched/fair: Reschedule the cfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <CAE80459-55EE-4BE5-B41D-8F68337782A4@gmail.com>
Date: Thu, 13 Jun 2024 21:46:13 +0800
From: Chunxin Zang <spring.cxz@...il.com>
To: Chen Yu <yu.c.chen@...el.com>
Cc: Honglei Wang <jameshongleiwang@....com>,
 mingo@...hat.com,
 Peter Zijlstra <peterz@...radead.org>,
 juri.lelli@...hat.com,
 vincent.guittot@...aro.org,
 dietmar.eggemann@....com,
 rostedt@...dmis.org,
 bsegall@...gle.com,
 mgorman@...e.de,
 bristot@...hat.com,
 vschneid@...hat.com,
 Mike Galbraith <efault@....de>,
 K Prateek Nayak <kprateek.nayak@....com>,
 linux-kernel@...r.kernel.org,
 yangchen11@...iang.com,
 Jerry Zhou <zhouchunhua@...iang.com>,
 Chunxin Zang <zangchunxin@...iang.com>
Subject: Re: [PATCH v2] sched/fair: Reschedule the cfs_rq when current is
 ineligible



> On Jun 13, 2024, at 21:23, Chen Yu <yu.c.chen@...el.com> wrote:
> 
> On 2024-06-13 at 20:02:37 +0800, Chunxin Zang wrote:
>> 
>> 
>>> On Jun 13, 2024, at 19:54, Chen Yu <yu.c.chen@...el.com> wrote:
>>> 
>>> On 2024-06-12 at 18:39:11 +0800, Chunxin Zang wrote:
>>>> 
>>>> 
>>>>> On Jun 7, 2024, at 13:07, Chen Yu <yu.c.chen@...el.com> wrote:
>>>>> 
>>>>> 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?
>>>> 
>>> 
>>> 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?
>>> 
>> 
>> This does indeed look better. In that case, do I need to make the changes this way and send
>> out a version 3?
> 
> 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 
Chunxin

> 
> 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);
> }
> 
> +static bool check_curr_preempt(struct cfs_rq *cfs_rq, struct sched_entity *curr)
> +{
> + if (sched_feat(RUN_TO_PARITY) || cfs_rq->nr_running <= 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 = 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));
> }
> 
> 
> @@ -8415,7 +8427,7 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
> /*
> * XXX pick_eevdf(cfs_rq) != se ?
> */
> - if (pick_eevdf(cfs_rq) == pse)
> + if (check_curr_preempt(cfs_rq, se) || pick_eevdf(cfs_rq) == pse)
> goto preempt;
> 
> return;
> -- 
> 2.25.1
> 
> 
>