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/[email protected]/T/#m52057282ceb6203318be1ce9f835363de3bef5cb
Signed-off-by: Chunxin Zang <[email protected]>
Reviewed-by: Chen Yang <[email protected]>
------
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 (!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;
+
/*
* XXX pick_eevdf(cfs_rq) != se ?
*/
--
2.34.1
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/[email protected]/T/#m52057282ceb6203318be1ce9f835363de3bef5cb
>
> Signed-off-by: Chunxin Zang <[email protected]>
> Reviewed-by: Chen Yang <[email protected]>
>
> ------
> 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
> On Jun 7, 2024, at 13:07, Chen Yu <[email protected]> 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/[email protected]/T/#m52057282ceb6203318be1ce9f835363de3bef5cb
>>
>> Signed-off-by: Chunxin Zang <[email protected]>
>> Reviewed-by: Chen Yang <[email protected]>
>>
>> ------
>> 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?
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 (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;
+}
+
static u64 __update_min_vruntime(struct cfs_rq *cfs_rq, u64 vruntime)
{
u64 min_vruntime = cfs_rq->min_vruntime;
@@ -974,11 +989,13 @@ static void clear_buddies(struct cfs_rq *cfs_rq, struct sched_entity *se);
/*
* XXX: strictly: vd_i += N*r_i/w_i such that: vd_i > ve_i
* this is probably good enough.
+ *
+ * return true if se need preempt
*/
-static void update_deadline(struct cfs_rq *cfs_rq, struct sched_entity *se)
+static bool update_deadline(struct cfs_rq *cfs_rq, struct sched_entity *se)
{
if ((s64)(se->vruntime - se->deadline) < 0)
- return;
+ return false;
/*
* For EEVDF the virtual time slope is determined by w_i (iow.
@@ -995,10 +1012,7 @@ static void update_deadline(struct cfs_rq *cfs_rq, struct sched_entity *se)
/*
* The task has consumed its request, reschedule.
*/
- if (cfs_rq->nr_running > 1) {
- resched_curr(rq_of(cfs_rq));
- clear_buddies(cfs_rq, se);
- }
+ return true;
}
#include "pelt.h"
@@ -1157,6 +1171,7 @@ static void update_curr(struct cfs_rq *cfs_rq)
{
struct sched_entity *curr = cfs_rq->curr;
s64 delta_exec;
+ bool need_preempt = false;
if (unlikely(!curr))
return;
@@ -1166,12 +1181,17 @@ static void update_curr(struct cfs_rq *cfs_rq)
return;
curr->vruntime += calc_delta_fair(delta_exec, curr);
- update_deadline(cfs_rq, curr);
+ need_preempt = update_deadline(cfs_rq, curr);
update_min_vruntime(cfs_rq);
if (entity_is_task(curr))
update_curr_task(task_of(curr), delta_exec);
+ if (need_preempt || check_entity_need_preempt(cfs_rq, curr)) {
+ resched_curr(rq_of(cfs_rq));
+ clear_buddies(cfs_rq, curr);
+ }
+
account_cfs_rq_runtime(cfs_rq, delta_exec);
}
>> +
>> + 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?
This modification no longer involves both wakeup and tick but is consolidated in 'update_curr', and it completes
the preemption decision along with 'update_deadline'. This approach seems more elegant and achieves the
same performance benefits as before.
thanks
Chunxin
>
> thanks,
> Chenyu
On 2024-06-12 at 18:39:11 +0800, Chunxin Zang wrote:
>
>
> > On Jun 7, 2024, at 13:07, Chen Yu <[email protected]> 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/[email protected]/T/#m52057282ceb6203318be1ce9f835363de3bef5cb
> >>
> >> Signed-off-by: Chunxin Zang <[email protected]>
> >> Reviewed-by: Chen Yang <[email protected]>
> >>
> >> ------
> >> 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;
> +}
> +
> On Jun 13, 2024, at 19:54, Chen Yu <[email protected]> wrote:
>
> On 2024-06-12 at 18:39:11 +0800, Chunxin Zang wrote:
>>
>>
>>> On Jun 7, 2024, at 13:07, Chen Yu <[email protected]> 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/[email protected]/T/#m52057282ceb6203318be1ce9f835363de3bef5cb
>>>>
>>>> Signed-off-by: Chunxin Zang <[email protected]>
>>>> Reviewed-by: Chen Yang <[email protected]>
>>>>
>>>> ------
>>>> 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?
>
This does indeed look better. In that case, do I need to make the changes this way and send
out a version 3?
thanks
Chunxin
> 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;
>> +}
>> +
On 2024-06-13 at 20:02:37 +0800, Chunxin Zang wrote:
>
>
> > On Jun 13, 2024, at 19:54, Chen Yu <[email protected]> wrote:
> >
> > On 2024-06-12 at 18:39:11 +0800, Chunxin Zang wrote:
> >>
> >>
> >>> On Jun 7, 2024, at 13:07, Chen Yu <[email protected]> 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.
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
> On Jun 13, 2024, at 21:23, Chen Yu <[email protected]> wrote:
>
> On 2024-06-13 at 20:02:37 +0800, Chunxin Zang wrote:
>>
>>
>>> On Jun 13, 2024, at 19:54, Chen Yu <[email protected]> wrote:
>>>
>>> On 2024-06-12 at 18:39:11 +0800, Chunxin Zang wrote:
>>>>
>>>>
>>>>> On Jun 7, 2024, at 13:07, Chen Yu <[email protected]> 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/[email protected]/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
>
>
>
Hello Chenyu,
Sorry, I'm a bit late to the thread but ...
On 6/13/2024 6:53 PM, Chen Yu wrote:
> [..snip..]
>>>
>>> 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.
>
> 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))
Shouldn't this return false if "entity_eligible(cfs_rq, curr)" returns
true since curr is still vruntime eligible on cfs_rq?
Would it better to have check_curr_preempt() as follows:
if (sched_feat(RUN_TO_PARITY) || cfs_rq->nr_running <= 1)
return false;
return entity_eligible(cfs_rq, curr);
which returns false if curr is ineligible and scheduler can go ahead and
and call schedule to evaluate the next best step?
Please let me know if I'm missing something.
> + return false;
> +
> + return true;
> +}
> +
> static u64 __update_min_vruntime(struct cfs_rq *cfs_rq, u64 vruntime)
> {
> u64 min_vruntime = cfs_rq->min_vruntime;
> [..snip..]
--
Thanks and Regards,
Prateek
Hello Chenyu,
On 6/14/2024 8:41 AM, K Prateek Nayak wrote:
> Hello Chenyu,
>
> Sorry, I'm a bit late to the thread but ...
>
> On 6/13/2024 6:53 PM, Chen Yu wrote:
>> [..snip..]
>>>>
>>>> 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.
>>
>> 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))
>
> Shouldn't this return false if "entity_eligible(cfs_rq, curr)" returns
> true since curr is still vruntime eligible on cfs_rq?
>
> Would it better to have check_curr_preempt() as follows:
>
> if (sched_feat(RUN_TO_PARITY) || cfs_rq->nr_running <= 1)
> return false;
>
> return entity_eligible(cfs_rq, curr);
The above return should have been:
return !entity_eligible(cfs_rq, curr);
Which returns true once curr is not vruntime eligible on cfs_rq.
Sorry for the oversight.
>
> which returns false if curr is ineligible and scheduler can go ahead and
> and call schedule to evaluate the next best step?
>
> Please let me know if I'm missing something.
>
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> static u64 __update_min_vruntime(struct cfs_rq *cfs_rq, u64 vruntime)
>> {
>> u64 min_vruntime = cfs_rq->min_vruntime;
>> [..snip..]
>
--
Thanks and Regards,
Prateek
Hi Prateek,
On 2024-06-14 at 08:46:54 +0530, K Prateek Nayak wrote:
> Hello Chenyu,
>
> > > +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))
> >
> > Shouldn't this return false if "entity_eligible(cfs_rq, curr)" returns
> > true since curr is still vruntime eligible on cfs_rq?
> >
> > Would it better to have check_curr_preempt() as follows:
> >
> > ????if (sched_feat(RUN_TO_PARITY) || cfs_rq->nr_running <= 1)
> > ??????? return false;
> >
> > ????return entity_eligible(cfs_rq, curr);
>
> The above return should have been:
>
> return !entity_eligible(cfs_rq, curr);
>
> Which returns true once curr is not vruntime eligible on cfs_rq.
> Sorry for the oversight.
>
Yes, you are right, thanks for catching this.
thanks,
Chenyu