Re: [PATCH v2] sched/fair: Reschedule the cfs_rq when current is ineligible

From: K Prateek Nayak
Date: Thu Jun 13 2024 - 23:17:16 EST


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