Re: [PATCH] kernel: Refactor task_struct to use numa_faults instead of numa_* pointers

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Oct 30, 2014 at 09:38:17PM +0200, Iulia Manda wrote:

> +++ b/include/linux/sched.h
> @@ -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.

> +++ 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)
> +/*
> + * 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;

>  }

> @@ -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)

>  		unsigned 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.


>  			if (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.

> +				 *
> +				 * 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...