Re: [PATCH] fs/proc: Expose mm_cpumask in /proc/[pid]/status

[Oleg Nesterov <oleg@redhat.com>]

Can't really comment this patch... I mean the intent.
Just a couple of nits:

	- I think this patch should also update Documentation/filesystems/proc.rst

	- I won't object, but do we really need/want another "if (mm)" block ?

	- I guess this is just my poor English, but the usage of "affinity"
	  in the changelog/comment looks a bit confusing to me ;) As if this
	  refers to task_struct.cpus_mask.

	  Fortunately "Cpus_active_mm..." in task_cpus_active_mm() makes it
	  more clear, so feel free to ignore.

Oleg.

On 12/16, Aaron Tomlin wrote:
>
> This patch introduces two new fields to /proc/[pid]/status to display the
> set of CPUs, representing the CPU affinity of the process's active
> memory context, in both mask and list format: "Cpus_active_mm" and
> "Cpus_active_mm_list". The mm_cpumask is primarily used for TLB and
> cache synchronisation.
>
> Exposing this information allows userspace to easily identify
> memory-task affinity, insight to NUMA alignment, CPU isolation and
> real-time workload placement.
>
> Frequent mm_cpumask changes may indicate instability in placement
> policies or excessive task migration overhead.
>
> Signed-off-by: Aaron Tomlin <atomlin@atomlin.com>
> ---
>  fs/proc/array.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 42932f88141a..8887c5e38e51 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -409,6 +409,23 @@ static void task_cpus_allowed(struct seq_file *m, struct task_struct *task)
>  		   cpumask_pr_args(&task->cpus_mask));
>  }
>
> +/**
> + * task_cpus_active_mm - Show the mm_cpumask for a process
> + * @m: The seq_file structure for the /proc/PID/status output
> + * @mm: The memory descriptor of the process
> + *
> + * Prints the set of CPUs, representing the CPU affinity of the process's
> + * active memory context, in both mask and list format. This mask is
> + * primarily used for TLB and cache synchronisation.
> + */
> +static void task_cpus_active_mm(struct seq_file *m, struct mm_struct *mm)
> +{
> +	seq_printf(m, "Cpus_active_mm:\t%*pb\n",
> +		   cpumask_pr_args(mm_cpumask(mm)));
> +	seq_printf(m, "Cpus_active_mm_list:\t%*pbl\n",
> +		   cpumask_pr_args(mm_cpumask(mm)));
> +}
> +
>  static inline void task_core_dumping(struct seq_file *m, struct task_struct *task)
>  {
>  	seq_put_decimal_ull(m, "CoreDumping:\t", !!task->signal->core_state);
> @@ -450,12 +467,15 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
>  		task_core_dumping(m, task);
>  		task_thp_status(m, mm);
>  		task_untag_mask(m, mm);
> -		mmput(mm);
>  	}
>  	task_sig(m, task);
>  	task_cap(m, task);
>  	task_seccomp(m, task);
>  	task_cpus_allowed(m, task);
> +	if (mm) {
> +		task_cpus_active_mm(m, mm);
> +		mmput(mm);
> +	}
>  	cpuset_task_status_allowed(m, task);
>  	task_context_switch_counts(m, task);
>  	arch_proc_pid_thread_features(m, task);
> --
> 2.51.0
>