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

[John Stills <johnstills191@gmail.com>]

> On Jun 21, 2024, at 21:53, Chunxin Zang <spring.cxz@gmail.com> wrote:
>
>
>
>> 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
>
>
Hi Chunxin

The latency test results look great. Have you conducted tests in other 
scenarios, such
as performance testing in production networks or machine learning?

-- 
thanks,
John