Re: [tip:sched/core] sched/numa: Delay retrying placement for automatic NUMA balance after wake_affine()

[Srikar Dronamraju]

Hi Mel,

I do see performance improving with this commit 7347fc87df "sched/numa:
Delay retrying placement for automatic NUMA balance after wake_affine()"
even on powerpc where we have SD_WAKE_AFFINE *disabled* on numa sched
domains. Ideally this commit should not have affected powerpc machines.
That made me to look a bit deeper.

> @@ -1876,7 +1877,18 @@ static void numa_migrate_preferred(struct task_struct *p)
>
>  	/* Periodically retry migrating the task to the preferred node */
>  	interval = min(interval, msecs_to_jiffies(p->numa_scan_period) / 16);
> -	p->numa_migrate_retry = jiffies + interval;
> +	numa_migrate_retry = jiffies + interval;
> +
> +	/*
> +	 * Check that the new retry threshold is after the current one. If
> +	 * the retry is in the future, it implies that wake_affine has
> +	 * temporarily asked NUMA balancing to backoff from placement.
> +	 */
> +	if (numa_migrate_retry > p->numa_migrate_retry)
> +		return;

The above check looks wrong. This check will most likely to be true,
numa_migrate_preferred() itself is called either when jiffies >
p->numa_migrate_retry or if the task's numa_preferred_nid has changed.

Hence we never end up calling task_numa_migrate() i.e we never go thro
the active cpu balancing path in numa balancing.

Reading the comments just above the check, makes me think the check
should have been

	if (numa_migrate_retry < p->numa_migrate_retry)
		return;

Here is perf stat output with 7347fc87df running perf bench numa mem
--no-data_rand_walk 96 -p 2 -t 48 -G 0 -P 3072 -T 0 -l 50 -c -s 1000

          2,13,898      cs                                                            ( +-  2.65% )
            10,228      migrations                                                    ( +- 14.61% )
         21,86,406      faults                                                        ( +-  9.69% )
   40,65,84,68,026      cache-misses                                                  ( +-  0.31% )
                 0      sched:sched_move_numa   <---------------
                 0      sched:sched_stick_numa   <---------------
                 0      sched:sched_swap_numa   <---------------
          1,41,780      migrate:mm_migrate_pages                                      ( +- 24.11% )
                 0      migrate:mm_numa_migrate_ratelimit

     778.331602169 seconds time elapsed


If you look at sched_move_numa, sched_stick_numa, sched_swap_numa
numbers, its very clear that we did try any active cpu migrations.

Same command with the commit reverted

       2,38,685      cs                                                            ( +-  2.93% )
            25,127      migrations                                                    ( +- 13.22% )
         17,27,858      faults                                                        ( +-  2.61% )
   34,77,06,21,298      cache-misses                                                  ( +-  0.61% )
               560      sched:sched_move_numa                                         ( +-  2.05% )
                16      sched:sched_stick_numa                                        ( +- 33.33% )
               310      sched:sched_swap_numa                                         ( +- 15.16% )
          1,25,062      migrate:mm_migrate_pages                                      ( +-  0.91% )
                 0      migrate:mm_numa_migrate_ratelimit

     916.777315465 seconds time elapsed

(numbers are almost same with just that check commented/modified)

So we are seeing an improvement, but the improvement is because of
bypassing the active cpu balancing. Do we really want to by-pass this
code?

> +
> +	/* Safe to try placing the task on the preferred node */
> +	p->numa_migrate_retry = numa_migrate_retry;
>
>  	/* Success if task is already running on preferred CPU */
>  	if (task_node(p) == p->numa_preferred_nid)
> @@ -5759,6 +5771,48 @@ wake_affine_weight(struct sched_domain *sd, struct task_struct *p,