Received-SPF: pass (google.com: domain of linux-kernel+bounces-189351-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <572bef0a-727c-4922-93e9-ad29c385120e@126.com>
Date: Sat, 25 May 2024 19:48:32 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] sched/fair: Reschedule the cfs_rq when current is
 ineligible
To: Chunxin Zang <spring.cxz@gmail.com>
Cc: dietmar.eggemann@arm.com, rostedt@goodmis.org, bsegall@google.com,
 mgorman@suse.de, bristot@redhat.com, vschneid@redhat.com,
 linux-kernel@vger.kernel.org, yu.c.chen@intel.com, yangchen11@lixiang.com,
 zhouchunhua@lixiang.com, zangchunxin@lixiang.com, mingo@redhat.com,
 peterz@infradead.org, juri.lelli@redhat.com, vincent.guittot@linaro.org
References: <20240524134011.270861-1-spring.cxz@gmail.com>
Content-Language: en-US
From: Honglei Wang <jameshongleiwang@126.com>
In-Reply-To: <20240524134011.270861-1-spring.cxz@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 2024/5/24 21:40, 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.
> 
> The modification can reduce the scheduling delay by about 30% when
> RUN_TO_PARITY is enabled.
> So far, it has been running well in my test environment, and I have
> pasted some test results below.
> 
> I isolated four cores for testing. I 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      00122      00089      00066
>                  # Max Latencies: 15442      07648      14133      07713
> 
>                  # Min Latencies: 00006      00010      00006      00006
>    LNICE(0)      # Avg Latencies: 00466      00277      00289      00257
>                  # Max Latencies: 38917      32391      32665      17710
> 
>                  # Min Latencies: 00019      00053      00010      00013
>    LNICE(19)     # Avg Latencies: 37151      31045      18293      23035
>                  # Max Latencies: 2688299    7031295    426196     425708
> 
> I'm actually a bit hesitant about placing this modification under the
> NO_PARITY feature. This is because the modification conflicts with the
> semantics of RUN_TO_PARITY. So, I captured and compared the number of
> resched 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     5057286    5003806    5018589    5031908
> stats.patch_cause_preempt_count    -------     858044     -------    765726     -------
> stats.need_preempt_count           570520      858684     3380513    3426977    1140821
> 
>  From the above test results, there is a slight increase in the number of
> resched occurrences in wakeup_preempt. However, the results vary with each
> test, and sometimes the difference is not that significant. But overall,
> the count of reschedules remains lower than that of CFS and is much less
> than that of NO_PARITY.
> 
> [1]: https://lore.kernel.org/all/20230816134059.GC982867@hirez.programming.kicks-ass.net/T/#m52057282ceb6203318be1ce9f835363de3bef5cb
> 
> Signed-off-by: Chunxin Zang <zangchunxin@lixiang.com>
> Reviewed-by: Chen Yang <yangchen11@lixiang.com>
> ---
>   kernel/sched/fair.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 03be0d1330a6..a0005d240db5 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5523,6 +5523,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 (!entity_eligible(cfs_rq, curr))
> +		resched_curr(rq_of(cfs_rq));
>   }
>   
>   
> @@ -8325,6 +8328,9 @@ static void check_preempt_wakeup_fair(struct rq *rq, struct task_struct *p, int
>   	if (unlikely(p->policy != SCHED_NORMAL) || !sched_feat(WAKEUP_PREEMPTION))
>   		return;
>   
> +	if (!entity_eligible(cfs_rq, se))
> +		goto preempt;
> +
>   	find_matching_se(&se, &pse);
>   	WARN_ON_ONCE(!pse);
>   
Hi Chunxin,

Did you run a comparative test to see which modification is more helpful 
on improve the latency? Modification at tick point makes more sense to 
me. But, seems just resched arbitrarily in wakeup might introduce too 
much preemption (and maybe more context switch?) in complex environment 
such as cgroup hierarchy.

Thanks,
Honglei