Re: [PATCH v3] sched/fair: Preempt if the current process is ineligible

[Chunxin Zang <spring.cxz@gmail.com>]

> On Jun 20, 2024, at 20:51, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Thu, Jun 13, 2024 at 09:14:37PM +0800, Chunxin Zang wrote:
>> ---
>> kernel/sched/fair.c | 28 +++++++++++++++++++++-------
>> 1 file changed, 21 insertions(+), 7 deletions(-)
>> 
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 03be0d1330a6..21ef610ddb14 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -745,6 +745,15 @@ 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;
>> +}
>> +
>> static u64 __update_min_vruntime(struct cfs_rq *cfs_rq, u64 vruntime)
>> {
>> u64 min_vruntime = cfs_rq->min_vruntime;
>> @@ -974,11 +983,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 +1006,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 +1165,7 @@ static void update_curr(struct cfs_rq *cfs_rq)
>> {
>> struct sched_entity *curr = cfs_rq->curr;
>> s64 delta_exec;
>> + bool need_preempt;
>> 
>> if (unlikely(!curr))
>> return;
>> @@ -1166,12 +1175,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);
>> }
> 
> Yeah sorry no. This will mess up the steady state schedule. At best we
> can do something like the below which will make PREEMPT_SHORT a little
> more effective I suppose.
> 
> 
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -985,10 +985,10 @@ static void clear_buddies(struct cfs_rq
>  * XXX: strictly: vd_i += N*r_i/w_i such that: vd_i > ve_i
>  * this is probably good enough.
>  */
> -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.
> @@ -1005,10 +1005,7 @@ static void update_deadline(struct cfs_r
> /*
> * 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"
> @@ -1168,6 +1165,8 @@ static void update_curr(struct cfs_rq *c
> {
> struct sched_entity *curr = cfs_rq->curr;
> s64 delta_exec;
> + struct rq *rq;
> + bool resched;
> 
> if (unlikely(!curr))
> return;
> @@ -1177,13 +1176,23 @@ static void update_curr(struct cfs_rq *c
> return;
> 
> curr->vruntime += calc_delta_fair(delta_exec, curr);
> - update_deadline(cfs_rq, curr);
> + resched = update_deadline(cfs_rq, curr);
> update_min_vruntime(cfs_rq);
> 
> if (entity_is_task(curr))
> update_curr_task(task_of(curr), delta_exec);
> 
> account_cfs_rq_runtime(cfs_rq, delta_exec);
> +
> + rq = rq_of(cfs_rq);
> + if (rq->nr_running == 1)
> + return;
> +
> + if (resched ||
> +    (curr->vlag != curr->deadline && !entity_eligible(cfs_rq, curr))) {
> + resched_curr(rq);
> + clear_buddies(cfs_rq, curr);
> + }
> }
> 
> static void update_curr_fair(struct rq *rq)

Hi peter

Got it. If I understand correctly, modifications to basic interfaces like update_curr
should be appropriate and not too aggressive. Additionally, these changes have
already shown significant improvements in scheduling delay (test results are at the
end). How about we limit this patch to these changes for now? Actually, I also want
to try a more aggressive preemption under NO_RUN_TO_PARITY, but it might be
better to consider this comprehensively after integrating the changes from your
latest branch.


Comparison of this modification with the mainline EEVDF in cyclictest.

                                 EEVDF      PATCH  EEVDF-NO_PARITY  PATCH-NO_PARITY

  LNICE(-19)    # Avg Latencies: 00191      00162      00089      00080

  LNICE(0)      # Avg Latencies: 00466      00404      00289      00285

  LNICE(19)     # Avg Latencies: 37151      38781      18293      19315

thanks 
Chunxin