Re: [PATCH v13 1/3] /proc/pid/status: Add support for architecture specific output

Re: [PATCH v13 1/3] /proc/pid/status: Add support for architecture specific output

[Thomas Gleixner]

On Sun, 24 Feb 2019, Aubrey Li wrote:

> The architecture specific information of the running processes could
> be useful to the userland. Add support to examine process architecture
> specific information externally.
> 
> Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Arjan van de Ven <arjan@linux.intel.com>

This really lacks

Cc: Linux API <linux-api@vger.kernel.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>

Cc'ed now.

> ---
>  fs/proc/array.c         | 5 +++++
>  include/linux/proc_fs.h | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 9d428d5a0ac8..ea7a981f289c 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -401,6 +401,10 @@ static inline void task_thp_status(struct seq_file *m, struct mm_struct *mm)
>  	seq_printf(m, "THP_enabled:\t%d\n", thp_enabled);
>  }
>  
> +void __weak arch_proc_pid_status(struct seq_file *m, struct task_struct *task)
> +{
> +}
> +
>  int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
>  			struct pid *pid, struct task_struct *task)
>  {
> @@ -424,6 +428,7 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
>  	task_cpus_allowed(m, task);
>  	cpuset_task_status_allowed(m, task);
>  	task_context_switch_counts(m, task);
> +	arch_proc_pid_status(m, task);
>  	return 0;
>  }
>  
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index d0e1f1522a78..1de9ba1b064f 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -73,6 +73,8 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
>  						    int (*show)(struct seq_file *, void *),
>  						    proc_write_t write,
>  						    void *data);
> +/* Add support for architecture specific output in /proc/pid/status */
> +extern void arch_proc_pid_status(struct seq_file *m, struct task_struct *task);
>  
>  #else /* CONFIG_PROC_FS */
>  
> -- 
> 2.17.1
> 
> 

Re: [PATCH v13 1/3] /proc/pid/status: Add support for architecture specific output

[Alexey Dobriyan]

On Fri, Apr 05, 2019 at 09:32:35PM +0200, Thomas Gleixner wrote:
> > +/* Add support for architecture specific output in /proc/pid/status */
> > +extern void arch_proc_pid_status(struct seq_file *m, struct task_struct *task);
     ^^^^^^

Unnecessary extern.

Re: [PATCH v13 1/3] /proc/pid/status: Add support for architecture specific output

[Li, Aubrey]

On 2019/4/7 5:41, Alexey Dobriyan wrote:
> On Fri, Apr 05, 2019 at 09:32:35PM +0200, Thomas Gleixner wrote:
>>> +/* Add support for architecture specific output in /proc/pid/status */
>>> +extern void arch_proc_pid_status(struct seq_file *m, struct task_struct *task);
>      ^^^^^^
> 
> Unnecessary extern.
> 
The linkage is default extern, but with this functions and variables
can be treated the same way.

Is it mandatory not to use it explicitly? ./script/checkpatch.pl did
not report this.

Thanks,
-Aubrey

Re: [PATCH v13 1/3] /proc/pid/status: Add support for architecture specific output

[Alexey Dobriyan]

On Sun, Apr 07, 2019 at 09:02:38PM +0800, Li, Aubrey wrote:
> On 2019/4/7 5:41, Alexey Dobriyan wrote:
> > On Fri, Apr 05, 2019 at 09:32:35PM +0200, Thomas Gleixner wrote:
> >>> +/* Add support for architecture specific output in /proc/pid/status */
> >>> +extern void arch_proc_pid_status(struct seq_file *m, struct task_struct *task);
> >      ^^^^^^
> > 
> > Unnecessary extern.
> > 
> The linkage is default extern, but with this functions and variables
> can be treated the same way.
> 
> Is it mandatory not to use it explicitly? ./script/checkpatch.pl did
> not report this.

"extern" is only necessary for variables. For prototypes, it is more typing
and more characters on the screen.

checkpatch.pl doesn't report it because opening floodgates is not the plan.

Yours,
	Extern Police.

Re: [PATCH v13 1/3] /proc/pid/status: Add support for architecture specific output

[Li, Aubrey]

On 2019/4/7 23:46, Alexey Dobriyan wrote:
> On Sun, Apr 07, 2019 at 09:02:38PM +0800, Li, Aubrey wrote:
>> On 2019/4/7 5:41, Alexey Dobriyan wrote:
>>> On Fri, Apr 05, 2019 at 09:32:35PM +0200, Thomas Gleixner wrote:
>>>>> +/* Add support for architecture specific output in /proc/pid/status */
>>>>> +extern void arch_proc_pid_status(struct seq_file *m, struct task_struct *task);
>>>      ^^^^^^
>>>
>>> Unnecessary extern.
>>>
>> The linkage is default extern, but with this functions and variables
>> can be treated the same way.
>>
>> Is it mandatory not to use it explicitly? ./script/checkpatch.pl did
>> not report this.
> 
> "extern" is only necessary for variables. For prototypes, it is more typing
> and more characters on the screen.
> 
> checkpatch.pl doesn't report it because opening floodgates is not the plan.
> 
> Yours,
> 	Extern Police.
>
 
No problem, Mr. Police, I can remove it in the next version.

Thanks,
-Aubrey

Re: [PATCH v13 1/3] /proc/pid/status: Add support for architecture specific output

[Andy Lutomirski]

On Fri, Apr 5, 2019 at 12:32 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sun, 24 Feb 2019, Aubrey Li wrote:
>
> > The architecture specific information of the running processes could
> > be useful to the userland. Add support to examine process architecture
> > specific information externally.
> >
> > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Cc: Tim Chen <tim.c.chen@linux.intel.com>
> > Cc: Dave Hansen <dave.hansen@intel.com>
> > Cc: Arjan van de Ven <arjan@linux.intel.com>
>
> This really lacks
>
> Cc: Linux API <linux-api@vger.kernel.org>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
>
> Cc'ed now.
>

I certainly understand why you want to expose this info, but would it
make more sense to instead add an arch_status file in /proc with
architecture-specific info?  Or maybe an x86_status field for x86
status, etc.

Re: [PATCH v13 1/3] /proc/pid/status: Add support for architecture specific output

[Li, Aubrey]

On 2019/4/8 1:34, Andy Lutomirski wrote:
> On Fri, Apr 5, 2019 at 12:32 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> On Sun, 24 Feb 2019, Aubrey Li wrote:
>>
>>> The architecture specific information of the running processes could
>>> be useful to the userland. Add support to examine process architecture
>>> specific information externally.
>>>
>>> Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Andi Kleen <ak@linux.intel.com>
>>> Cc: Tim Chen <tim.c.chen@linux.intel.com>
>>> Cc: Dave Hansen <dave.hansen@intel.com>
>>> Cc: Arjan van de Ven <arjan@linux.intel.com>
>>
>> This really lacks
>>
>> Cc: Linux API <linux-api@vger.kernel.org>
>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>
>> Cc'ed now.
>>
> 
> I certainly understand why you want to expose this info, but would it
> make more sense to instead add an arch_status file in /proc with
> architecture-specific info?  Or maybe an x86_status field for x86
> status, etc.
> 

I tried this, but no other architecture showed interest in arch_status
under /proc.

Thanks,
-Aubrey

Re: [PATCH v13 1/3] /proc/pid/status: Add support for architecture specific output

[Andy Lutomirski]

On Sun, Apr 7, 2019 at 5:38 PM Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>
> On 2019/4/8 1:34, Andy Lutomirski wrote:
> > On Fri, Apr 5, 2019 at 12:32 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >> On Sun, 24 Feb 2019, Aubrey Li wrote:
> >>
> >>> The architecture specific information of the running processes could
> >>> be useful to the userland. Add support to examine process architecture
> >>> specific information externally.
> >>>
> >>> Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
> >>> Cc: Peter Zijlstra <peterz@infradead.org>
> >>> Cc: Andi Kleen <ak@linux.intel.com>
> >>> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> >>> Cc: Dave Hansen <dave.hansen@intel.com>
> >>> Cc: Arjan van de Ven <arjan@linux.intel.com>
> >>
> >> This really lacks
> >>
> >> Cc: Linux API <linux-api@vger.kernel.org>
> >> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >>
> >> Cc'ed now.
> >>
> >
> > I certainly understand why you want to expose this info, but would it
> > make more sense to instead add an arch_status file in /proc with
> > architecture-specific info?  Or maybe an x86_status field for x86
> > status, etc.
> >
>
> I tried this, but no other architecture showed interest in arch_status
> under /proc.
>

Why is that a problem?  It could exist on x86 and not exist on other
arches until needed.

Re: [PATCH v13 1/3] /proc/pid/status: Add support for architecture specific output

[Li, Aubrey]

On 2019/4/8 9:52, Andy Lutomirski wrote:
> On Sun, Apr 7, 2019 at 5:38 PM Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>>
>> On 2019/4/8 1:34, Andy Lutomirski wrote:
>>> On Fri, Apr 5, 2019 at 12:32 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>>>>
>>>> On Sun, 24 Feb 2019, Aubrey Li wrote:
>>>>
>>>>> The architecture specific information of the running processes could
>>>>> be useful to the userland. Add support to examine process architecture
>>>>> specific information externally.
>>>>>
>>>>> Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
>>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>>> Cc: Andi Kleen <ak@linux.intel.com>
>>>>> Cc: Tim Chen <tim.c.chen@linux.intel.com>
>>>>> Cc: Dave Hansen <dave.hansen@intel.com>
>>>>> Cc: Arjan van de Ven <arjan@linux.intel.com>
>>>>
>>>> This really lacks
>>>>
>>>> Cc: Linux API <linux-api@vger.kernel.org>
>>>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>
>>>> Cc'ed now.
>>>>
>>>
>>> I certainly understand why you want to expose this info, but would it
>>> make more sense to instead add an arch_status file in /proc with
>>> architecture-specific info?  Or maybe an x86_status field for x86
>>> status, etc.
>>>
>>
>> I tried this, but no other architecture showed interest in arch_status
>> under /proc.
>>
> 
> Why is that a problem?  It could exist on x86 and not exist on other
> arches until needed.
> 

I placed it in tid_base_stuff, under live_patch entry, so it exists for
all arches, is there a better way to do this?

Thanks,
-Aubrey

Re: [PATCH v13 2/3] x86,/proc/pid/status: Add AVX-512 usage elapsed time

[Jann Horn]

On Fri, Apr 5, 2019 at 10:02 PM Aubrey Li <aubrey.li@linux.intel.com> wrote:
> AVX-512 components use could cause core turbo frequency drop. So
> it's useful to expose AVX-512 usage elapsed time as a heuristic hint
> for the user space job scheduler to cluster the AVX-512 using tasks
> together.
>
> Tensorflow example:
> $ while [ 1 ]; do cat /proc/pid/status | grep AVX; sleep 1; done
> AVX512_elapsed_ms:      4
> AVX512_elapsed_ms:      8
> AVX512_elapsed_ms:      4
>
> This means that 4 milliseconds have elapsed since the AVX512 usage
> of tensorflow task was detected when the task was scheduled out.
>
> Or:
> $ cat /proc/pid/status | grep AVX512_elapsed_ms
> AVX512_elapsed_ms:      -1

(Very nitpicky, feel free to ignore: If you change the /proc/pid to
/proc/tid in the commit message, it becomes clearer that this status
is really per-task/thread, not per-process/threadgroup.)

[...]
> +
> +/*
> + * Report the amount of time elapsed in millisecond since last AVX512
> + * use in the task.
> + */
> +static void avx512_status(struct seq_file *m, struct task_struct *task)
> +{
> +       unsigned long timestamp = task->thread.fpu.avx512_timestamp;

This is theoretically a data race, right? Should this have a READ_ONCE() on it?

Is there something that zeroes out the avx512_timestamp on
fork()/clone(), or will every child inherit the avx512 timestamp? As
far as I can tell, the timestamp is inherited; I think it would be
nicer to zero it out at that point. Either way, It might be worth
documenting this decision.

Re: [PATCH v13 2/3] x86,/proc/pid/status: Add AVX-512 usage elapsed time

[Li, Aubrey]

On 2019/4/6 4:27, Jann Horn wrote:
> On Fri, Apr 5, 2019 at 10:02 PM Aubrey Li <aubrey.li@linux.intel.com> wrote:
>> AVX-512 components use could cause core turbo frequency drop. So
>> it's useful to expose AVX-512 usage elapsed time as a heuristic hint
>> for the user space job scheduler to cluster the AVX-512 using tasks
>> together.
>>
>> Tensorflow example:
>> $ while [ 1 ]; do cat /proc/pid/status | grep AVX; sleep 1; done
>> AVX512_elapsed_ms:      4
>> AVX512_elapsed_ms:      8
>> AVX512_elapsed_ms:      4
>>
>> This means that 4 milliseconds have elapsed since the AVX512 usage
>> of tensorflow task was detected when the task was scheduled out.
>>
>> Or:
>> $ cat /proc/pid/status | grep AVX512_elapsed_ms
>> AVX512_elapsed_ms:      -1
> 
> (Very nitpicky, feel free to ignore: If you change the /proc/pid to
> /proc/tid in the commit message, it becomes clearer that this status
> is really per-task/thread, not per-process/threadgroup.)

Thanks, I'll refine.

> 
> [...]
>> +
>> +/*
>> + * Report the amount of time elapsed in millisecond since last AVX512
>> + * use in the task.
>> + */
>> +static void avx512_status(struct seq_file *m, struct task_struct *task)
>> +{
>> +       unsigned long timestamp = task->thread.fpu.avx512_timestamp;
> 
> This is theoretically a data race, right? Should this have a READ_ONCE() on it?

Thanks, I'll refine.

> 
> Is there something that zeroes out the avx512_timestamp on
> fork()/clone(), or will every child inherit the avx512 timestamp? As
> far as I can tell, the timestamp is inherited; I think it would be
> nicer to zero it out at that point. Either way, It might be worth
> documenting this decision.
> 

This timestamp is not inherited, see below:

_do_fork()
->copy_process()
-->dup_task_struct()
--->arch_dup_task_struct()
---->fpu__copy()

Thanks,
-Aubrey