Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1673681 > unrolled thread

[PATCH 4/4] sched,fair: remove effective_load

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/4] sched,fair: remove effective_load riel@redhat.com - 2017-06-23 19:00 +0200
    [tip:sched/core] sched/fair: Remove effective_load() tip-bot for Rik van Riel <tipbot@zytor.com> - 2017-06-24 09:30 +0200
    Re: [PATCH 4/4] sched,fair: remove effective_load Peter Zijlstra <peterz@infradead.org> - 2017-06-26 16:50 +0200
      Re: [PATCH 4/4] sched,fair: remove effective_load Peter Zijlstra <peterz@infradead.org> - 2017-06-26 16:50 +0200
        Re: [PATCH 4/4] sched,fair: remove effective_load Rik van Riel <riel@redhat.com> - 2017-06-26 17:00 +0200
          Re: [PATCH 4/4] sched,fair: remove effective_load Peter Zijlstra <peterz@infradead.org> - 2017-06-26 17:10 +0200
            Re: [PATCH 4/4] sched,fair: remove effective_load Rik van Riel <riel@redhat.com> - 2017-06-26 17:30 +0200
              Re: [PATCH 4/4] sched,fair: remove effective_load Peter Zijlstra <peterz@infradead.org> - 2017-06-26 18:20 +0200
                Re: [PATCH 4/4] sched,fair: remove effective_load Rik van Riel <riel@redhat.com> - 2017-06-26 21:40 +0200
                  Re: [PATCH 4/4] sched,fair: remove effective_load Peter Zijlstra <peterz@infradead.org> - 2017-06-27 07:40 +0200
                    Re: [PATCH 4/4] sched,fair: remove effective_load Rik van Riel <riel@redhat.com> - 2017-06-27 17:00 +0200
                Re: [PATCH 4/4] sched,fair: remove effective_load Rik van Riel <riel@redhat.com> - 2017-06-27 20:30 +0200

#1673681 — [PATCH 4/4] sched,fair: remove effective_load

From: Rik van Riel <riel@redhat.com>

The function effective_load was only used by the NUMA balancing
code, and not by the regular load balancing code. Now that the
NUMA balancing code no longer uses it either, get rid of it.

Signed-off-by: Rik van Riel <riel@redhat.com>
---
 kernel/sched/fair.c | 124 +---------------------------------------------------
 1 file changed, 1 insertion(+), 123 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index d03a21e6627d..5d98836d9f73 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1387,7 +1387,6 @@ static unsigned long weighted_cpuload(const int cpu);
 static unsigned long source_load(int cpu, int type);
 static unsigned long target_load(int cpu, int type);
 static unsigned long capacity_of(int cpu);
-static long effective_load(struct task_group *tg, int cpu, long wl, long wg);
 
 /* Cached statistics for all CPUs within a node */
 struct numa_stats {
@@ -3048,8 +3047,7 @@ __update_load_avg(u64 now, int cpu, struct sched_avg *sa,
  * differential update where we store the last value we propagated. This in
  * turn allows skipping updates if the differential is 'small'.
  *
- * Updating tg's load_avg is necessary before update_cfs_share() (which is
- * done) and effective_load() (which is not done because it is too costly).
+ * Updating tg's load_avg is necessary before update_cfs_share().
  */
 static inline void update_tg_load_avg(struct cfs_rq *cfs_rq, int force)
 {
@@ -5251,126 +5249,6 @@ static unsigned long cpu_avg_load_per_task(int cpu)
 	return 0;
 }
 
-#ifdef CONFIG_FAIR_GROUP_SCHED
-/*
- * effective_load() calculates the load change as seen from the root_task_group
- *
- * Adding load to a group doesn't make a group heavier, but can cause movement
- * of group shares between cpus. Assuming the shares were perfectly aligned one
- * can calculate the shift in shares.
- *
- * Calculate the effective load difference if @wl is added (subtracted) to @tg
- * on this @cpu and results in a total addition (subtraction) of @wg to the
- * total group weight.
- *
- * Given a runqueue weight distribution (rw_i) we can compute a shares
- * distribution (s_i) using:
- *
- *   s_i = rw_i / \Sum rw_j						(1)
- *
- * Suppose we have 4 CPUs and our @tg is a direct child of the root group and
- * has 7 equal weight tasks, distributed as below (rw_i), with the resulting
- * shares distribution (s_i):
- *
- *   rw_i = {   2,   4,   1,   0 }
- *   s_i  = { 2/7, 4/7, 1/7,   0 }
- *
- * As per wake_affine() we're interested in the load of two CPUs (the CPU the
- * task used to run on and the CPU the waker is running on), we need to
- * compute the effect of waking a task on either CPU and, in case of a sync
- * wakeup, compute the effect of the current task going to sleep.
- *
- * So for a change of @wl to the local @cpu with an overall group weight change
- * of @wl we can compute the new shares distribution (s'_i) using:
- *
- *   s'_i = (rw_i + @wl) / (@wg + \Sum rw_j)				(2)
- *
- * Suppose we're interested in CPUs 0 and 1, and want to compute the load
- * differences in waking a task to CPU 0. The additional task changes the
- * weight and shares distributions like:
- *
- *   rw'_i = {   3,   4,   1,   0 }
- *   s'_i  = { 3/8, 4/8, 1/8,   0 }
- *
- * We can then compute the difference in effective weight by using:
- *
- *   dw_i = S * (s'_i - s_i)						(3)
- *
- * Where 'S' is the group weight as seen by its parent.
- *
- * Therefore the effective change in loads on CPU 0 would be 5/56 (3/8 - 2/7)
- * times the weight of the group. The effect on CPU 1 would be -4/56 (4/8 -
- * 4/7) times the weight of the group.
- */
-static long effective_load(struct task_group *tg, int cpu, long wl, long wg)
-{
-	struct sched_entity *se = tg->se[cpu];
-
-	if (!tg->parent)	/* the trivial, non-cgroup case */
-		return wl;
-
-	for_each_sched_entity(se) {
-		struct cfs_rq *cfs_rq = se->my_q;
-		long W, w = cfs_rq_load_avg(cfs_rq);
-
-		tg = cfs_rq->tg;
-
-		/*
-		 * W = @wg + \Sum rw_j
-		 */
-		W = wg + atomic_long_read(&tg->load_avg);
-
-		/* Ensure \Sum rw_j >= rw_i */
-		W -= cfs_rq->tg_load_avg_contrib;
-		W += w;
-
-		/*
-		 * w = rw_i + @wl
-		 */
-		w += wl;
-
-		/*
-		 * wl = S * s'_i; see (2)
-		 */
-		if (W > 0 && w < W)
-			wl = (w * (long)scale_load_down(tg->shares)) / W;
-		else
-			wl = scale_load_down(tg->shares);
-
-		/*
-		 * Per the above, wl is the new se->load.weight value; since
-		 * those are clipped to [MIN_SHARES, ...) do so now. See
-		 * calc_cfs_shares().
-		 */
-		if (wl < MIN_SHARES)
-			wl = MIN_SHARES;
-
-		/*
-		 * wl = dw_i = S * (s'_i - s_i); see (3)
-		 */
-		wl -= se->avg.load_avg;
-
-		/*
-		 * Recursively apply this logic to all parent groups to compute
-		 * the final effective load change on the root group. Since
-		 * only the @tg group gets extra weight, all parent groups can
-		 * only redistribute existing shares. @wl is the shift in shares
-		 * resulting from this level per the above.
-		 */
-		wg = 0;
-	}
-
-	return wl;
-}
-#else
-
-static long effective_load(struct task_group *tg, int cpu, long wl, long wg)
-{
-	return wl;
-}
-
-#endif
-
 static void record_wakee(struct task_struct *p)
 {
 	/*
-- 
2.9.4

[toc] | [next] | [standalone]

#1674042 — [tip:sched/core] sched/fair: Remove effective_load()

Commit-ID:  815abf5af45f04f759f12f3172afd15226fd7f71
Gitweb:     http://git.kernel.org/tip/815abf5af45f04f759f12f3172afd15226fd7f71
Author:     Rik van Riel <riel@redhat.com>
AuthorDate: Fri, 23 Jun 2017 12:55:30 -0400
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 24 Jun 2017 08:57:53 +0200

sched/fair: Remove effective_load()

The effective_load() function was only used by the NUMA balancing
code, and not by the regular load balancing code. Now that the
NUMA balancing code no longer uses it either, get rid of it.

Signed-off-by: Rik van Riel <riel@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: jhladky@redhat.com
Cc: linux-kernel@vger.kernel.org
Link: http://lkml.kernel.org/r/20170623165530.22514-5-riel@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/sched/fair.c | 124 +---------------------------------------------------
 1 file changed, 1 insertion(+), 123 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 79ac078..6f4f155 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -1382,7 +1382,6 @@ static unsigned long weighted_cpuload(const int cpu);
 static unsigned long source_load(int cpu, int type);
 static unsigned long target_load(int cpu, int type);
 static unsigned long capacity_of(int cpu);
-static long effective_load(struct task_group *tg, int cpu, long wl, long wg);
 
 /* Cached statistics for all CPUs within a node */
 struct numa_stats {
@@ -3045,8 +3044,7 @@ __update_load_avg_cfs_rq(u64 now, int cpu, struct cfs_rq *cfs_rq)
  * differential update where we store the last value we propagated. This in
  * turn allows skipping updates if the differential is 'small'.
  *
- * Updating tg's load_avg is necessary before update_cfs_share() (which is
- * done) and effective_load() (which is not done because it is too costly).
+ * Updating tg's load_avg is necessary before update_cfs_share().
  */
 static inline void update_tg_load_avg(struct cfs_rq *cfs_rq, int force)
 {
@@ -5298,126 +5296,6 @@ static unsigned long cpu_avg_load_per_task(int cpu)
 	return 0;
 }
 
-#ifdef CONFIG_FAIR_GROUP_SCHED
-/*
- * effective_load() calculates the load change as seen from the root_task_group
- *
- * Adding load to a group doesn't make a group heavier, but can cause movement
- * of group shares between cpus. Assuming the shares were perfectly aligned one
- * can calculate the shift in shares.
- *
- * Calculate the effective load difference if @wl is added (subtracted) to @tg
- * on this @cpu and results in a total addition (subtraction) of @wg to the
- * total group weight.
- *
- * Given a runqueue weight distribution (rw_i) we can compute a shares
- * distribution (s_i) using:
- *
- *   s_i = rw_i / \Sum rw_j						(1)
- *
- * Suppose we have 4 CPUs and our @tg is a direct child of the root group and
- * has 7 equal weight tasks, distributed as below (rw_i), with the resulting
- * shares distribution (s_i):
- *
- *   rw_i = {   2,   4,   1,   0 }
- *   s_i  = { 2/7, 4/7, 1/7,   0 }
- *
- * As per wake_affine() we're interested in the load of two CPUs (the CPU the
- * task used to run on and the CPU the waker is running on), we need to
- * compute the effect of waking a task on either CPU and, in case of a sync
- * wakeup, compute the effect of the current task going to sleep.
- *
- * So for a change of @wl to the local @cpu with an overall group weight change
- * of @wl we can compute the new shares distribution (s'_i) using:
- *
- *   s'_i = (rw_i + @wl) / (@wg + \Sum rw_j)				(2)
- *
- * Suppose we're interested in CPUs 0 and 1, and want to compute the load
- * differences in waking a task to CPU 0. The additional task changes the
- * weight and shares distributions like:
- *
- *   rw'_i = {   3,   4,   1,   0 }
- *   s'_i  = { 3/8, 4/8, 1/8,   0 }
- *
- * We can then compute the difference in effective weight by using:
- *
- *   dw_i = S * (s'_i - s_i)						(3)
- *
- * Where 'S' is the group weight as seen by its parent.
- *
- * Therefore the effective change in loads on CPU 0 would be 5/56 (3/8 - 2/7)
- * times the weight of the group. The effect on CPU 1 would be -4/56 (4/8 -
- * 4/7) times the weight of the group.
- */
-static long effective_load(struct task_group *tg, int cpu, long wl, long wg)
-{
-	struct sched_entity *se = tg->se[cpu];
-
-	if (!tg->parent)	/* the trivial, non-cgroup case */
-		return wl;
-
-	for_each_sched_entity(se) {
-		struct cfs_rq *cfs_rq = se->my_q;
-		long W, w = cfs_rq_load_avg(cfs_rq);
-
-		tg = cfs_rq->tg;
-
-		/*
-		 * W = @wg + \Sum rw_j
-		 */
-		W = wg + atomic_long_read(&tg->load_avg);
-
-		/* Ensure \Sum rw_j >= rw_i */
-		W -= cfs_rq->tg_load_avg_contrib;
-		W += w;
-
-		/*
-		 * w = rw_i + @wl
-		 */
-		w += wl;
-
-		/*
-		 * wl = S * s'_i; see (2)
-		 */
-		if (W > 0 && w < W)
-			wl = (w * (long)scale_load_down(tg->shares)) / W;
-		else
-			wl = scale_load_down(tg->shares);
-
-		/*
-		 * Per the above, wl is the new se->load.weight value; since
-		 * those are clipped to [MIN_SHARES, ...) do so now. See
-		 * calc_cfs_shares().
-		 */
-		if (wl < MIN_SHARES)
-			wl = MIN_SHARES;
-
-		/*
-		 * wl = dw_i = S * (s'_i - s_i); see (3)
-		 */
-		wl -= se->avg.load_avg;
-
-		/*
-		 * Recursively apply this logic to all parent groups to compute
-		 * the final effective load change on the root group. Since
-		 * only the @tg group gets extra weight, all parent groups can
-		 * only redistribute existing shares. @wl is the shift in shares
-		 * resulting from this level per the above.
-		 */
-		wg = 0;
-	}
-
-	return wl;
-}
-#else
-
-static long effective_load(struct task_group *tg, int cpu, long wl, long wg)
-{
-	return wl;
-}
-
-#endif
-
 static void record_wakee(struct task_struct *p)
 {
 	/*

[toc] | [prev] | [next] | [standalone]

#1674832

On Fri, Jun 23, 2017 at 12:55:30PM -0400, riel@redhat.com wrote:
> From: Rik van Riel <riel@redhat.com>
> 
> The function effective_load was only used by the NUMA balancing
> code, and not by the regular load balancing code. Now that the
> NUMA balancing code no longer uses it either, get rid of it.

Hmm,... funny. It used to be used by wake-affine. I'll have to go check
what happened.

[toc] | [prev] | [next] | [standalone]

#1674837

On Mon, Jun 26, 2017 at 04:44:37PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 23, 2017 at 12:55:30PM -0400, riel@redhat.com wrote:
> > From: Rik van Riel <riel@redhat.com>
> > 
> > The function effective_load was only used by the NUMA balancing
> > code, and not by the regular load balancing code. Now that the
> > NUMA balancing code no longer uses it either, get rid of it.
> 
> Hmm,... funny. It used to be used by wake-affine. I'll have to go check
> what happened.

Ah, it fell pray to that LLC == NUMA confusion from the previous patch.

That really looks buggered.

[toc] | [prev] | [next] | [standalone]

#1674845

On Mon, 2017-06-26 at 16:46 +0200, Peter Zijlstra wrote:
> On Mon, Jun 26, 2017 at 04:44:37PM +0200, Peter Zijlstra wrote:
> > On Fri, Jun 23, 2017 at 12:55:30PM -0400, riel@redhat.com wrote:
> > > From: Rik van Riel <riel@redhat.com>
> > > 
> > > The function effective_load was only used by the NUMA balancing
> > > code, and not by the regular load balancing code. Now that the
> > > NUMA balancing code no longer uses it either, get rid of it.
> > 
> > Hmm,... funny. It used to be used by wake-affine. I'll have to go
> > check
> > what happened.
> 
> Ah, it fell pray to that LLC == NUMA confusion from the previous
> patch.
> 
> That really looks buggered.

Do the changelog or comments of that patch need fixing,
to avoid LLC / NUMA confusion?

I remember us talking about that in the past, but I do
not remember whether or not I changed the comments after
that discussion...

[toc] | [prev] | [next] | [standalone]

#1674853

On Mon, Jun 26, 2017 at 10:55:41AM -0400, Rik van Riel wrote:
> On Mon, 2017-06-26 at 16:46 +0200, Peter Zijlstra wrote:
> > On Mon, Jun 26, 2017 at 04:44:37PM +0200, Peter Zijlstra wrote:
> > > On Fri, Jun 23, 2017 at 12:55:30PM -0400, riel@redhat.com wrote:
> > > > From: Rik van Riel <riel@redhat.com>
> > > > 
> > > > The function effective_load was only used by the NUMA balancing
> > > > code, and not by the regular load balancing code. Now that the
> > > > NUMA balancing code no longer uses it either, get rid of it.
> > > 
> > > Hmm,... funny. It used to be used by wake-affine. I'll have to go
> > > check
> > > what happened.
> > 
> > Ah, it fell pray to that LLC == NUMA confusion from the previous
> > patch.
> > 
> > That really looks buggered.
> 
> Do the changelog or comments of that patch need fixing,
> to avoid LLC / NUMA confusion?

Neither, I think the code is actually wrong for the case where LLC <
NUMA (a somewhat rare case these days, granted, but something that might
still happen on !x86 perhaps).

[toc] | [prev] | [next] | [standalone]

#1674877

On Mon, 2017-06-26 at 17:04 +0200, Peter Zijlstra wrote:
> On Mon, Jun 26, 2017 at 10:55:41AM -0400, Rik van Riel wrote:
> > On Mon, 2017-06-26 at 16:46 +0200, Peter Zijlstra wrote:
> > > On Mon, Jun 26, 2017 at 04:44:37PM +0200, Peter Zijlstra wrote:
> > > > On Fri, Jun 23, 2017 at 12:55:30PM -0400, riel@redhat.com
> > > > wrote:
> > > > > From: Rik van Riel <riel@redhat.com>
> > > > > 
> > > > > The function effective_load was only used by the NUMA
> > > > > balancing
> > > > > code, and not by the regular load balancing code. Now that
> > > > > the
> > > > > NUMA balancing code no longer uses it either, get rid of it.
> > > > 
> > > > Hmm,... funny. It used to be used by wake-affine. I'll have to
> > > > go
> > > > check
> > > > what happened.
> > > 
> > > Ah, it fell pray to that LLC == NUMA confusion from the previous
> > > patch.
> > > 
> > > That really looks buggered.
> > 
> > Do the changelog or comments of that patch need fixing,
> > to avoid LLC / NUMA confusion?
> 
> Neither, I think the code is actually wrong for the case where LLC <
> NUMA (a somewhat rare case these days, granted, but something that
> might
> still happen on !x86 perhaps).

Oh, indeed.  I guess in wake_affine() we should test
whether the CPUs are in the same NUMA node, rather than
doing cpus_share_cache() ?

Or, alternatively, have an update_numa_stats() variant
for numa_wake_affine() that works on the LLC level?

[toc] | [prev] | [next] | [standalone]

#1674916

On Mon, Jun 26, 2017 at 11:20:54AM -0400, Rik van Riel wrote:

> Oh, indeed.  I guess in wake_affine() we should test
> whether the CPUs are in the same NUMA node, rather than
> doing cpus_share_cache() ?

Well, since select_idle_sibling() is on LLC; the early test on
cpus_share_cache(prev,this) seems to actually make sense.

But then cutting out all the other bits seems wrong. Not in the least
because !NUMA_BALACING should also still keep working.

> Or, alternatively, have an update_numa_stats() variant
> for numa_wake_affine() that works on the LLC level?

I think we want to retain the existing behaviour for everything
larger than LLC, and when NUMA_BALANCING, smaller than NUMA.

Also note that your use of task_h_load() in the new numa thing suffers
from exactly the problem effective_load() is trying to solve.

[toc] | [prev] | [next] | [standalone]

#1675064

On Mon, 2017-06-26 at 18:12 +0200, Peter Zijlstra wrote:
> On Mon, Jun 26, 2017 at 11:20:54AM -0400, Rik van Riel wrote:
> 
> > Oh, indeed.  I guess in wake_affine() we should test
> > whether the CPUs are in the same NUMA node, rather than
> > doing cpus_share_cache() ?
> 
> Well, since select_idle_sibling() is on LLC; the early test on
> cpus_share_cache(prev,this) seems to actually make sense.
> 
> But then cutting out all the other bits seems wrong. Not in the least
> because !NUMA_BALACING should also still keep working.

Even when !NUMA_BALANCING, I suspect it makes little sense
to compare the loads just one the cores in question, since
select_idle_sibling() will likely move the task somewhere
else.

I suspect we want to compare the load on the whole LLC
for that reason, even with NUMA_BALANCING disabled.

> > Or, alternatively, have an update_numa_stats() variant
> > for numa_wake_affine() that works on the LLC level?
> 
> I think we want to retain the existing behaviour for everything
> larger than LLC, and when NUMA_BALANCING, smaller than NUMA.

What do you mean by this, exactly?

How does the "existing behaviour" of only looking at
the load on two cores make sense when doing LLC-level
task placement?

> Also note that your use of task_h_load() in the new numa thing
> suffers
> from exactly the problem effective_load() is trying to solve.

Are you saying task_h_load is wrong in task_numa_compare()
too, then?  Should both use effective_load()?

[toc] | [prev] | [next] | [standalone]

#1675301

On Mon, Jun 26, 2017 at 03:34:49PM -0400, Rik van Riel wrote:
> On Mon, 2017-06-26 at 18:12 +0200, Peter Zijlstra wrote:
> > On Mon, Jun 26, 2017 at 11:20:54AM -0400, Rik van Riel wrote:
> > 
> > > Oh, indeed.  I guess in wake_affine() we should test
> > > whether the CPUs are in the same NUMA node, rather than
> > > doing cpus_share_cache() ?
> > 
> > Well, since select_idle_sibling() is on LLC; the early test on
> > cpus_share_cache(prev,this) seems to actually make sense.
> > 
> > But then cutting out all the other bits seems wrong. Not in the least
> > because !NUMA_BALACING should also still keep working.
> 
> Even when !NUMA_BALANCING, I suspect it makes little sense
> to compare the loads just one the cores in question, since
> select_idle_sibling() will likely move the task somewhere
> else.
> 
> I suspect we want to compare the load on the whole LLC
> for that reason, even with NUMA_BALANCING disabled.

But we don't have that data around :/ One thing we could do is try and
keep a copy of the last s*_lb_stats around in the sched_domain_shared
stuff or something and try and use that.

That way we can strictly keep things at the LLC level and not confuse
things with NUMA.

Similarly, we could use that same data to then avoid re-computing things
for the NUMA domain as well and do away with numa_stats.

> > > Or, alternatively, have an update_numa_stats() variant
> > > for numa_wake_affine() that works on the LLC level?
> > 
> > I think we want to retain the existing behaviour for everything
> > larger than LLC, and when NUMA_BALANCING, smaller than NUMA.
> 
> What do you mean by this, exactly?

As you noted, when prev and this are in the same LLC, it doesn't matter
and select_idle_sibling() will do its thing. So anything smaller than
the LLC need not do anything.

When NUMA_BALANCING we have the numa_stats thing and we can, as you
propose use that.

If LLC < NUMA or !NUMA_BALANCING we have a region that needs to do
_something_.

> How does the "existing behaviour" of only looking at
> the load on two cores make sense when doing LLC-level
> task placement?

Right, might not be ideal, but its what we have now. Supposedly its
better than not doing anything at all.

But see above for other ideas.

> > Also note that your use of task_h_load() in the new numa thing
> > suffers
> > from exactly the problem effective_load() is trying to solve.
> 
> Are you saying task_h_load is wrong in task_numa_compare()
> too, then?  Should both use effective_load()?

I need more than the few minutes I currently have, but probably. The
question is of course, how much does it matter and how painful will it
be to do it better.

[toc] | [prev] | [next] | [standalone]

#1675858

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2017-06-27 at 07:39 +0200, Peter Zijlstra wrote:
> On Mon, Jun 26, 2017 at 03:34:49PM -0400, Rik van Riel wrote:
> > On Mon, 2017-06-26 at 18:12 +0200, Peter Zijlstra wrote:
> > > On Mon, Jun 26, 2017 at 11:20:54AM -0400, Rik van Riel wrote:
> > > 
> > > > Oh, indeed.  I guess in wake_affine() we should test
> > > > whether the CPUs are in the same NUMA node, rather than
> > > > doing cpus_share_cache() ?
> > > 
> > > Well, since select_idle_sibling() is on LLC; the early test on
> > > cpus_share_cache(prev,this) seems to actually make sense.
> > > 
> > > But then cutting out all the other bits seems wrong. Not in the
> > > least
> > > because !NUMA_BALACING should also still keep working.
> > 
> > Even when !NUMA_BALANCING, I suspect it makes little sense
> > to compare the loads just one the cores in question, since
> > select_idle_sibling() will likely move the task somewhere
> > else.
> > 
> > I suspect we want to compare the load on the whole LLC
> > for that reason, even with NUMA_BALANCING disabled.
> 
> But we don't have that data around :/ One thing we could do is try
> and
> keep a copy of the last s*_lb_stats around in the sched_domain_shared
> stuff or something and try and use that.
> 
> That way we can strictly keep things at the LLC level and not confuse
> things with NUMA.
> 
> Similarly, we could use that same data to then avoid re-computing
> things
> for the NUMA domain as well and do away with numa_stats.

That does seem like a useful optimization, though
I guess we would have to invalidate the cached data
every time we actually move a task?

The current code simply walks all the CPUs in the
cpumask_t, and adds up capacity and load. Doing
that appears to be better than poor task placement
(Jirka's numbers speak for themselves), but optimizing
this code path does seem like a worthwhile goal.

I'll look into it.

> > > > Or, alternatively, have an update_numa_stats() variant
> > > > for numa_wake_affine() that works on the LLC level?
> > > 
> > > I think we want to retain the existing behaviour for everything
> > > larger than LLC, and when NUMA_BALANCING, smaller than NUMA.
> > 
> > What do you mean by this, exactly?
> 
> As you noted, when prev and this are in the same LLC, it doesn't
> matter
> and select_idle_sibling() will do its thing. So anything smaller than
> the LLC need not do anything.
> 
> When NUMA_BALANCING we have the numa_stats thing and we can, as you
> propose use that.
> 
> If LLC < NUMA or !NUMA_BALANCING we have a region that needs to do
> _something_.

Agreed. I will fix this. Given that this is a bit
of a corner case, I guess I can fix this with follow-up
patches, to be merged into -tip before the whole series
is sent on to Linus?

> > > Also note that your use of task_h_load() in the new numa thing
> > > suffers
> > > from exactly the problem effective_load() is trying to solve.
> > 
> > Are you saying task_h_load is wrong in task_numa_compare()
> > too, then?  Should both use effective_load()?
> 
> I need more than the few minutes I currently have, but probably. The
> question is of course, how much does it matter and how painful will
> it
> be to do it better.

I suspect it does not matter at all currenly, since the 
load balancing code does not use effective_load, and
having the wake_affine logic calculate things differently
from the load balancer is likely to result in both pieces
of code fighting against each other.

I suspect we should either use task_h_load everywhere,
or effective_load everywhere, but not have a mix and
match situation where one is used in some places, and
the other in others.

-- 
All rights reversed

[toc] | [prev] | [next] | [standalone]

#1676062

[Multipart message — attachments visible in raw view] — view raw

On Mon, 2017-06-26 at 18:12 +0200, Peter Zijlstra wrote:

> Also note that your use of task_h_load() in the new numa thing
> suffers
> from exactly the problem effective_load() is trying to solve.

Thinking about this some more, I suspect that
using effective_load() in combination with
select_idle_sibling(), which will often place
the task on a different CPU than the one specified,
may not lead to entirely useful results...

-- 
All rights reversed

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web