kernel: Refactor task_struct to use numa_faults instead of numa_* pointers
PATCH 1/1 kernel: Refactor task_struct to use numa_faults instead of numa_* pointers
This patch simplifies task_struct by removing the four numa_* pointers in the same array and replacing them with the array pointer. By doing this, the size of task_struct is reduced.
A new parameter is added to the task_faults_idx function so that it can return an index to the correct offset, corresponding with the old precalculated pointers.
All of the code in sched/ that depended on task_faults_idx and numa_* was changed in order to match the new logic.
--- include/linux/sched.h | 40 ++++++++++-------- kernel/sched/core.c | 3 +- kernel/sched/debug.c | 4 +- kernel/sched/fair.c | 110 +++++++++++++++++++++++++------------------------ 4 files changed, 82 insertions(+), 75 deletions(-)
include/linux/sched.h +22 -18
@@ -796,6 +796,15 @@ struct sched_info { };#endif /* defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) */ +#ifdef CONFIG_NUMA_BALANCING +enum numa_faults_stats { +NUMA_MEM =0 , +NUMA_CPU , +NUMA_MEMBUF , +NUMA_CPUBUF +}; +#endif +#ifdef CONFIG_TASK_DELAY_ACCT struct task_delay_info { spinlock_t lock;
@@ -1558,28 +1567,23 @@ struct task_struct {struct numa_group *numa_group;/* -* Exponential decaying average of faults on a per-node basis. -* Scheduling placement decisions are made based on the these counts. -* The values remain static for the duration of a PTE scan +* numa_faults is an array split into four regions: +* faults_memory, faults_cpu, faults_memory_buffer, faults_cpu_buffer +* in this precise order. +* +* faults_memory: Exponential decaying average of faults on a per-node +* basis. Scheduling placement decisions are made based on these +* counts. The values remain static for the duration of a PTE scan. +* faults_cpu: Track the nodes the process was running on when a NUMA +* hinting fault was incurred. +* faults_memory_buffer and faults_cpu_buffer: Record faults per node +* during the current scan window. When the scan completes, the counts +* in faults_memory and faults_cpu decay and these values are copied. */ -unsigned long *numa_faults_memory; +unsigned long *numa_faults;unsigned long total_numa_faults;/* -* numa_faults_buffer records faults per node during the current -* scan window. When the scan completes, the counts in -* numa_faults_memory decay and these values are copied. -*/ -unsigned long *numa_faults_buffer_memory; - -/* -* Track the nodes the process was running on when a NUMA hinting -* fault was incurred. -*/ -unsigned long *numa_faults_cpu; -unsigned long *numa_faults_buffer_cpu; - -/* * numa_faults_locality tracks if faults recorded during the last * scan window were remote/local. The task scan period is adapted * based on the locality of the faults with different weights
kernel/sched/core.c +1 -2
@@ -1853,8 +1853,7 @@ static void __sched_fork(unsigned long clone_flags, struct task_struct *p) 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_memory =NULL ; - p->numa_faults_buffer_memory =NULL ; + p->numa_faults =NULL ; p->last_task_numa_placement =0 ; p->last_sum_exec_runtime =0 ;
kernel/sched/debug.c +2 -2
@@ -528,8 +528,8 @@ static void sched_show_numa(struct task_struct *p, struct seq_file *m)unsigned long nr_faults =-1 ;int cpu_current, home_node; -if (p->numa_faults_memory) - nr_faults = p->numa_faults_memory[2 *node + i]; +if (p->numa_faults) + nr_faults = p->numa_faults[2 *node + i]; cpu_current = !i ? (task_node (p) == node) : (pol &&node_isset (node, pol->v.nodes));
kernel/sched/fair.c +57 -53
226 lines not on this page · raw message
Signature · 1 line
1.7.10.4
Quoted 4 lines · Iulia Manda
This patch simplifies
task_structby removing the four numa_* pointers in the same array and replacing them with the array pointer. By doing this, the size oftask_structis reduced.
Quoted 8 lines
A new parameter is added to the
task_faults_idxfunction so that it can return an index to the correct offset, corresponding with the old precalculated pointers.All of the code in sched/ that depended on
task_faults_idxand numa_* was changed in order to match the new logic.Signed-off-by Iulia Manda <[email protected]>
Peter Zijlstra
maintainerinclude/linux/sched.h struct sched_info { · line 806
Quoted 13 lines · Iulia Manda
+++
b/include/linux/sched.hinclude/linux/sched.h +8
-0@@ -796,6 +796,15 @@ struct sched_info { };#endif /* defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) */ +#ifdef CONFIG_NUMA_BALANCING +enum numa_faults_stats { +NUMA_MEM =0 , +NUMA_CPU , +NUMA_MEMBUF , +NUMA_CPUBUF +}; +#endif
Maybe stick this in kernel/sched/sched.h, I don't think there is need of this outside of the scheduler.
And please use an existing ifdef wherever possible.
Quoted 7 lines
kernel/sched/fair.c +2
-1+++ b/kernel/sched/fair.c@@ -895,18 +895,25 @@ pid_t task_numa_group_id(struct task_struct *p)return p->numa_group ? p->numa_group->gid :0 ; } -static inline int task_faults_idx (int nid,int priv) +/*
kernel/sched/fair.c pid_t task_numa_group_id(struct task_struct *p) · line 907
Quoted 10 lines
kernel/sched/fair.c +8
-1+* The averaged statistics, shared & private, memory & cpu, +* occupy the first half of the array. The second half of the +* array is for current counters, which are averaged into the +* first set by task_numa_placement. +*/ +static inline int task_faults_idx (enum numa_faults_stats s,int nid,int priv) { -return NR_NUMA_HINT_FAULT_TYPES * nid + priv; +return s *NR_NUMA_HINT_FAULT_TYPES * nr_node_ids + +NR_NUMA_HINT_FAULT_TYPES * nid + priv;
return NR_NUMA_HINT_FAULTS_TYPES * (s * nr_node_ids + nid) + priv;
kernel/sched/fair.c static void task_numa_placement(struct task_struct *p) · line 1593
Quoted 5 lines
kernel/sched/fair.c +0
-0}@@ -1585,17 +1592,22 @@ static void task_numa_placement(struct task_struct *p)/* Find the node with the highest number of faults */ for_each_online_node (nid) {
(a)
kernel/sched/fair.c static void task_numa_placement(struct task_struct *p) · line 1597
Quoted 5 lines
kernel/sched/fair.c +3
-1unsigned long faults =0 , group_faults =0 ; -int priv, i; +int priv; +/* Keep track of the offsets in numa_faults array */ +int mem_idx, membuf_idx, cpu_idx, cpubuf_idx;
Maybe move the new line on top (a), such that its an inverse xmas tree.
kernel/sched/fair.c static void task_numa_placement(struct task_struct *p) · line 1630
Quoted 5 lines
kernel/sched/fair.c +1
-3if (p->numa_group) { -/* safe because we can only change our own group */ - p->numa_group->faults[i] += diff; - p->numa_group->faults_cpu[i] += f_diff; +/* safe because we can only change our own group
Incorrect multi-line comment style.
kernel/sched/fair.c static void task_numa_placement(struct task_struct *p) · line 1642
Quoted 13 lines
kernel/sched/fair.c +8
-1+* +* mem_idx represents the offset for a given +* nid and priv in a specific region because it +* is at the beginning of the numa_faults array. +*/ + p->numa_group->faults[mem_idx] += diff; + p->numa_group->faults_cpu[mem_idx] += f_diff; p->numa_group->total_faults += diff; - group_faults += p->numa_group->faults[i]; + group_faults += p->numa_group->faults[mem_idx]; } }
Other than that the patch is surprisingly painless...
Quoted 3 lines · Iulia Manda
This patch simplifies
task_structby removing the four numa_* pointers in the same array and replacing them with the array pointer. By doing this, the size oftask_structis reduced.
This is always welcome, but for completeness you should specify by how much on <insert your favorite arch>. afaict you're removing 3 ulong pointers.
Quoted 8 lines
A new parameter is added to the
task_faults_idxfunction so that it can return an index to the correct offset, corresponding with the old precalculated pointers.All of the code in sched/ that depended on
task_faults_idxand numa_* was changed in order to match the new logic.Signed-off-by Iulia Manda <[email protected]>
With some suggestions below.
Quoted 36 lines
--- include/linux/sched.h | 40 ++++++++++-------- kernel/sched/core.c | 3 +- kernel/sched/debug.c | 4 +- kernel/sched/fair.c | 110 +++++++++++++++++++++++++------------------------ 4 files changed, 82 insertions(+), 75 deletions(-)include/linux/sched.h +11
-3@@ -796,6 +796,15 @@ struct sched_info { };#endif /* defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) */ +#ifdef CONFIG_NUMA_BALANCING +enum numa_faults_stats { +NUMA_MEM =0 , +NUMA_CPU , +NUMA_MEMBUF , +NUMA_CPUBUF +}; +#endif +#ifdef CONFIG_TASK_DELAY_ACCT struct task_delay_info { spinlock_t lock;@@ -1558,28 +1567,23 @@ struct task_struct {struct numa_group *numa_group;/* -* Exponential decaying average of faults on a per-node basis. -* Scheduling placement decisions are made based on the these counts. -* The values remain static for the duration of a PTE scan +* numa_faults is an array split into four regions: +* faults_memory, faults_cpu, faults_memory_buffer, faults_cpu_buffer
include/linux/sched.h struct task_struct { · line 1581
Quoted 10 lines
include/linux/sched.h +10
-0+* in this precise order. +* +* faults_memory: Exponential decaying average of faults on a per-node +* basis. Scheduling placement decisions are made based on these +* counts. The values remain static for the duration of a PTE scan. +* faults_cpu: Track the nodes the process was running on when a NUMA +* hinting fault was incurred. +* faults_memory_buffer and faults_cpu_buffer: Record faults per node +* during the current scan window. When the scan completes, the counts +* in faults_memory and faults_cpu decay and these values are copied.
How about moving these comments to where you have the enum numa_faults_stats? And just point to that here.
[...]
include/linux/sched.h struct task_struct {
Quoted 12 lines
include/linux/sched.h +9
-2-static inline int task_faults_idx (int nid,int priv) +/* +* The averaged statistics, shared & private, memory & cpu, +* occupy the first half of the array. The second half of the +* array is for current counters, which are averaged into the +* first set by task_numa_placement. +*/ +static inline int task_faults_idx (enum numa_faults_stats s,int nid,int priv) { -return NR_NUMA_HINT_FAULT_TYPES * nid + priv; +return s *NR_NUMA_HINT_FAULT_TYPES * nr_node_ids + +NR_NUMA_HINT_FAULT_TYPES * nid + priv;
parenthesis here?
Thanks,
Davidlohr