Re: [RFC PATCH 2/2] sched/fair: skip the cache hot CPU in select_idle_cpu()

[Chen Yu]

Hi Prateek,

On 2023-09-12 at 08:35:12 +0530, K Prateek Nayak wrote:
> Hello Chenyu,
> 
> On 9/11/2023 3:49 PM, Chen Yu wrote:
> > Hi Prateek,
> > 
> > thanks for your review,
> > 
> > On 2023-09-11 at 13:59:10 +0530, K Prateek Nayak wrote:
> >> Hello Chenyu,
> >>
> >> On 9/11/2023 8:20 AM, Chen Yu wrote:
> >>>  [..snip..]
> >>>  kernel/sched/fair.c     | 30 +++++++++++++++++++++++++++---
> >>>  kernel/sched/features.h |  1 +
> >>>  kernel/sched/sched.h    |  1 +
> >>>  3 files changed, 29 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> >>> index e20f50726ab8..fe3b760c9654 100644
> >>> --- a/kernel/sched/fair.c
> >>> +++ b/kernel/sched/fair.c
> >>> @@ -6629,6 +6629,21 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> >>>  	hrtick_update(rq);
> >>>  	now = sched_clock_cpu(cpu_of(rq));
> >>>  	p->se.prev_sleep_time = task_sleep ? now : 0;
> >>> +#ifdef CONFIG_SMP
> >>> +	/*
> >>> +	 * If this rq will become idle, and dequeued task is
> >>> +	 * a short sleeping one, check if we can reserve
> >>> +	 * this idle CPU for that task for a short while.
> >>> +	 * During this reservation period, other wakees will
> >>> +	 * skip this 'idle' CPU in select_idle_cpu(), and this
> >>> +	 * short sleeping task can pick its previous CPU in
> >>> +	 * select_idle_sibling(), which brings better cache
> >>> +	 * locality.
> >>> +	 */
> >>> +	if (sched_feat(SIS_CACHE) && task_sleep && !rq->nr_running &&
> >>> +	    p->se.sleep_avg && p->se.sleep_avg < sysctl_sched_migration_cost)
> >>> +		rq->cache_hot_timeout = now + p->se.sleep_avg;
> >>> +#endif
> >>>  }
> >>>  
> >>>  #ifdef CONFIG_SMP
> >>> @@ -6982,8 +6997,13 @@ static inline int find_idlest_cpu(struct sched_domain *sd, struct task_struct *p
> >>>  static inline int __select_idle_cpu(int cpu, struct task_struct *p)
> >>>  {
> >>>  	if ((available_idle_cpu(cpu) || sched_idle_cpu(cpu)) &&
> >>> -	    sched_cpu_cookie_match(cpu_rq(cpu), p))
> >>> +	    sched_cpu_cookie_match(cpu_rq(cpu), p)) {
> >>> +		if (sched_feat(SIS_CACHE) &&
> >>> +		    sched_clock_cpu(cpu) < cpu_rq(cpu)->cache_hot_timeout)
> >>> +			return -1;
> >>
> >> Just wondering,
> >>
> >> Similar to how select_idle_core() caches the "idle_cpu" if it ends up
> >> finding one in its search for an idle core, would returning a "cache-hot
> >> idle CPU" be better than returning previous CPU / current CPU if all
> >> idle CPUs found during the search in select_idle_cpu() are marked
> >> cache-hot?
> >>
> > 
> > This is a good point, we can optimize this further. Currently I only
> > send a simple version to desmonstrate how we can leverage the task's
> > sleep time.
> > 
> >> Speaking of cache-hot idle CPU, is netperf actually more happy with
> >> piling on current CPU?
> > 
> > Yes. Per my previous test, netperf of TCP_RR/UDP_RR really likes to
> > put the waker and wakee together.
> > 
> >> I ask this because the logic seems to be
> >> reserving the previous CPU for a task that dislikes migration but I
> >> do not see anything in the wake_affine_idle() path that would make the
> >> short sleeper proactively choose the previous CPU when the wakeup is
> >> marked with the WF_SYNC flag. Let me know if I'm missing something?
> >>
> > 
> > If I understand correctly, WF_SYNC is to let the wakee to woken up
> > on the waker's CPU, rather than the wakee's previous CPU, because
> > the waker goes to sleep after wakeup. SIS_CACHE mainly cares about
> > wakee's previous CPU. We can only restrict that other wakee does not
> > occupy the previous CPU, but do not enhance the possibility that
> > wake_affine_idle() chooses the previous CPU.
> 
> Correct me if I'm wrong here,
> 
> Say a short sleeper, is always woken up using WF_SYNC flag. When the
> task is dequeued, we mark the previous  CPU where it ran as "cache-hot"
> and restrict any wakeup happening until the "cache_hot_timeout" is
> crossed. Let us assume a perfect world where the task wakes up before
> the "cache_hot_timeout" expires. Logically this CPU was reserved all
> this while for the short sleeper but since the wakeup bears WF_SYNC
> flag, the whole reservation is ignored and waker's LLC is explored.
> 

Ah, I see your point. Do you mean, because the waker has a WF_SYNC, wake_affine_idle()
forces the short sleeping wakee to be woken up on waker's CPU rather the
wakee's previous CPU, but wakee's previous has been marked as cache hot
for nothing?

> Should the timeout be cleared if the wakeup decides to not target the
> previous CPU? (The default "sysctl_sched_migration_cost" is probably
> small enough to curb any side effect that could possibly show here but
> if a genuine use-case warrants setting "sysctl_sched_migration_cost" to
> a larger value, the wakeup path might be affected where lot of idle
> targets are overlooked since the CPUs are marked cache-hot forr longer
> duration)
> 
> Let me know what you think.
> 

This makes sense. In theory the above logic can be added in
select_idle_sibling(), if target CPU is chosen rather than
the previous CPU, the previous CPU's cache hot flag should be
cleared.

But this might bring overhead. Because we need to grab the rq
lock and write to other CPU's rq, which could be costly. It
seems to be a trade-off of current implementation. On the other
hand, if the user sets the sysctl_sched_migration_cost to a quite
large value:
1. Without SIS_CACHE, there is no task migration.
2. With SIS_CACHE enabled, all idle CPUs are cache hot and be skipped
   in select_idle_cpu(), the wakee will be woken up locally.
It seems to be of the same effect, so there is no much impact
to wakeup behavior I suppose.

thanks,
Chenyu