lkml.org 
[lkml]   [2018]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] sched/numa: Stagger NUMA balancing scan periods for new threads

* 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

\
 
 \ /
  Last update: 2018-04-27 08:51    [W:0.038 / U:0.696 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site