Re: [PATCH] sched/numa: Stagger NUMA balancing scan periods for new threads

[Ingo Molnar <mingo@kernel.org>]

* Mel Gorman <mgorman@techsingularity.net> wrote:

> The different in headline performance across a range of machines and
> workloads is marginal but the system CPU usage is reduced due to less scan
> activity.  The following is the time reported by NAS Parallel Benchmark
> using unbound openmp threads and a D size class.
> 
>                       4.17.0-rc1             4.17.0-rc1
>                          vanilla           stagger-v1r1
> Time bt.D      442.77 (   0.00%)      419.70 (   5.21%)
> Time cg.D      171.90 (   0.00%)      180.85 (  -5.21%)
> Time ep.D       33.10 (   0.00%)       32.90 (   0.60%)
> Time is.D        9.59 (   0.00%)        9.42 (   1.77%)
> Time lu.D      306.75 (   0.00%)      304.65 (   0.68%)
> Time mg.D       54.56 (   0.00%)       52.38 (   4.00%)
> Time sp.D     1020.03 (   0.00%)      903.77 (  11.40%)
> Time ua.D      400.58 (   0.00%)      386.49 (   3.52%)
> 
> Note it's not a universal win but we have no prior knowledge of which
> thread matters but the number of threads created often exceeds the size of
> the node when the threads are not bound. On balance, the set of workloads
> complete faster and there is a a reducation of overall system CPU usage
> 
>                             4.17.0-rc1             4.17.0-rc1
>                                vanilla           stagger-v1r1
> sys-time-bt.D         48.78 (   0.00%)       48.22 (   1.15%)
> sys-time-cg.D         25.31 (   0.00%)       26.63 (  -5.22%)
> sys-time-ep.D          1.65 (   0.00%)        0.62 (  62.42%)
> sys-time-is.D         40.05 (   0.00%)       24.45 (  38.95%)
> sys-time-lu.D         37.55 (   0.00%)       29.02 (  22.72%)
> sys-time-mg.D         47.52 (   0.00%)       34.92 (  26.52%)
> sys-time-sp.D        119.01 (   0.00%)      109.05 (   8.37%)
> sys-time-ua.D         51.52 (   0.00%)       45.13 (  12.40%)
> 
> NUMA scan activity is reduced as well as other balancing activity.
> 
> NUMA alloc local               1042828     1342670
> NUMA base PTE updates        140481138    93577468
> NUMA huge PMD updates           272171      180766
> NUMA page range updates      279832690   186129660
> NUMA hint faults               1395972     1193897
> NUMA hint local faults          877925      855053
> NUMA hint local percent             62          71
> NUMA pages migrated           12057909     9158023

Looks like a nice reduction in scanning overhead - which was always the main worry 
with the fault based active NUMA balancing technique.

I have a couple of minor code cleanliness nit:

> +void init_numa_balancing(unsigned long clone_flags, struct task_struct *p)
> +{
> +	int mm_users = 0;
> +
> +	if (p->mm) {
> +		mm_users = atomic_read(&p->mm->mm_users);
> +		if (mm_users == 1) {
> +			p->mm->numa_next_scan = jiffies + msecs_to_jiffies(sysctl_numa_balancing_scan_delay);
> +			p->mm->numa_scan_seq = 0;
> +		}
> +	}
> +	p->node_stamp = 0ULL;
> +	p->numa_scan_seq = p->mm ? p->mm->numa_scan_seq : 0;
> +	p->numa_scan_period = sysctl_numa_balancing_scan_delay;
> +	p->numa_work.next = &p->numa_work;
> +	p->numa_faults = NULL;
> +	p->last_task_numa_placement = 0;
> +	p->last_sum_exec_runtime = 0;
> +	p->numa_group = NULL;

While this is pre-existing code that you moved, could we please use a bit more 
organization to make this more readable:

	p->node_stamp			= 0ULL;
	p->numa_scan_seq		= p->mm ? p->mm->numa_scan_seq : 0;
	p->numa_scan_period		= sysctl_numa_balancing_scan_delay;
	p->numa_work.next		= &p->numa_work;
	p->numa_faults			= NULL;
	p->last_task_numa_placement	= 0;
	p->last_sum_exec_runtime	= 0;
	p->numa_group			= NULL;

?

This form made me notice a detail: the 0ULL asymmetry looks weird, this integer 
literal type specification is entirely superfluous here, we can just write '0'.

> +	/* New address space */
> +	if (!(clone_flags & CLONE_VM)) {
> +		p->numa_preferred_nid = -1;
> +		return;
> +	}
> +
> +	/* New thread, use existing preferred nid but stagger scans */
> +	if (p->mm) {
> +		unsigned int delay;
> +
> +		delay = min_t(unsigned int, task_scan_max(current),
> +			current->numa_scan_period * mm_users * NSEC_PER_MSEC);
> +		delay += 2 * TICK_NSEC;
> +		p->numa_preferred_nid = current->numa_preferred_nid;
> +		p->node_stamp = delay;
> +	}

So this is a fork time function, shouldn't p->numa_preferred_nid be equal to 
current->numa_preferred_nid already?

This is what happens in the !p->mm && CLONE_VM case anyway, right?

So we could leave out the superfluous assignment and make it clear in a comment 
that we inherit the parent's ->numa_preferred_nid intentionally.

Also, there's a lot of p->mm use, could we add this helper local variable to 
simplify the code some more:

	struct mm_struct *mm = p->mm;

?

Thanks,

	Ingo