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

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

[Aubrey Li]

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>
Cc: Linux API <linux-api@vger.kernel.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 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 2edbb657f859..331592a61718 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 52a283ba0465..bf4328cb58ed 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -74,6 +74,8 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
 						    proc_write_t write,
 						    void *data);
 extern struct pid *tgid_pidfd_to_pid(const struct file *file);
+/* Add support for architecture specific output in /proc/pid/status */
+void arch_proc_pid_status(struct seq_file *m, struct task_struct *task);
 
 #else /* CONFIG_PROC_FS */
 
-- 
2.21.0

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

[Aubrey Li]

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/tid/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/tid/status | grep AVX512_elapsed_ms
AVX512_elapsed_ms:      -1

The number '-1' indicates that no AVX512 usage recorded before
thus the task unlikely has frequency drop issue.

User space tools may want to further check by:

$ perf stat --pid <pid> -e core_power.lvl2_turbo_license -- sleep 1

 Performance counter stats for process id '3558':

     3,251,565,961      core_power.lvl2_turbo_license

       1.004031387 seconds time elapsed

Non-zero counter value confirms that the task causes frequency drop.

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>
Cc: Linux API <linux-api@vger.kernel.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 arch/x86/kernel/fpu/xstate.c | 42 ++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index d7432c2b1051..5e55ed9584ab 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -7,6 +7,8 @@
 #include <linux/cpu.h>
 #include <linux/mman.h>
 #include <linux/pkeys.h>
+#include <linux/seq_file.h>
+#include <linux/proc_fs.h>
 
 #include <asm/fpu/api.h>
 #include <asm/fpu/internal.h>
@@ -1243,3 +1245,43 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 
 	return 0;
 }
+
+/*
+ * 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 = READ_ONCE(task->thread.fpu.avx512_timestamp);
+	long delta;
+
+	if (!timestamp) {
+		/*
+		 * Report -1 if no AVX512 usage
+		 */
+		delta = -1;
+	} else {
+		delta = (long)(jiffies - timestamp);
+		/*
+		 * Cap to LONG_MAX if time difference > LONG_MAX
+		 */
+		if (delta < 0)
+			delta = LONG_MAX;
+		delta = jiffies_to_msecs(delta);
+	}
+
+	seq_put_decimal_ll(m, "AVX512_elapsed_ms:\t", delta);
+	seq_putc(m, '\n');
+}
+
+/*
+ * Report architecture specific information
+ */
+void arch_proc_pid_status(struct seq_file *m, struct task_struct *task)
+{
+	/*
+	 * Report AVX512 state if the processor and build option supported.
+	 */
+	if (cpu_feature_enabled(X86_FEATURE_AVX512F))
+		avx512_status(m, task);
+}
-- 
2.21.0

[PATCH v14 3/3] Documentation/filesystems/proc.txt: add AVX512_elapsed_ms

[Aubrey Li]

Added AVX512_elapsed_ms in /proc/<pid>/status. Report it
in Documentation/filesystems/proc.txt

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>
Cc: Linux API <linux-api@vger.kernel.org>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 Documentation/filesystems/proc.txt | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 66cad5c86171..c4a9e48681ad 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -207,6 +207,7 @@ read the file /proc/PID/status:
   Speculation_Store_Bypass:       thread vulnerable
   voluntary_ctxt_switches:        0
   nonvoluntary_ctxt_switches:     1
+  AVX512_elapsed_ms:	8
 
 This shows you nearly the same information you would get if you viewed it with
 the ps  command.  In  fact,  ps  uses  the  proc  file  system  to  obtain its
@@ -224,7 +225,7 @@ asynchronous manner and the value may not be very precise. To see a precise
 snapshot of a moment, you can see /proc/<pid>/smaps file and scan page table.
 It's slow but very precise.
 
-Table 1-2: Contents of the status files (as of 4.19)
+Table 1-2: Contents of the status files (as of 5.1)
 ..............................................................................
  Field                       Content
  Name                        filename of the executable
@@ -289,6 +290,32 @@ Table 1-2: Contents of the status files (as of 4.19)
  Mems_allowed_list           Same as previous, but in "list format"
  voluntary_ctxt_switches     number of voluntary context switches
  nonvoluntary_ctxt_switches  number of non voluntary context switches
+ AVX512_elapsed_ms           time elapsed since last AVX512 usage recorded
+
+ AVX512_elapsed_ms:
+ ------------------
+  If AVX512 is supported on the machine, this entry shows the milliseconds
+  elapsed since the last time AVX512 usage was recorded. The recording
+  happens on a best effort basis when a task is scheduled out. This means
+  that the value depends on two factors:
+
+    1) The time which the task spent on the CPU without being scheduled
+       out. With CPU isolation and a single runnable task this can take
+       several seconds.
+
+    2) The time since the task was scheduled out last. Depending on the
+       reason for being scheduled out (time slice exhausted, syscall ...)
+       this can be arbitrary long time.
+
+  As a consequence the value cannot be considered precise and authoritative
+  information. The application which uses this information has to be aware
+  of the overall scenario on the system in order to determine whether a
+  task is a real AVX512 user or not.
+
+  A special value of '-1' indicates that no AVX512 usage was recorded, thus
+  the task is unlikely an AVX512 user, but depends on the workload and the
+  scheduling scenario, it also could be a false negative mentioned above.
+
 ..............................................................................
 
 Table 1-3: Contents of the statm files (as of 2.6.8-rc3)
-- 
2.21.0

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

[Andy Lutomirski]

On Tue, Apr 9, 2019 at 6:55 PM Aubrey Li <aubrey.li@linux.intel.com> 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>
> Cc: Linux API <linux-api@vger.kernel.org>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  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 2edbb657f859..331592a61718 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)
> +{
> +}

This pointlessly bloats other architectures.  Do this instead in an
appropriate header:

#ifndef arch_proc_pid_status
static inline void arch_proc_pid_status(...)
{
}
#endif

Or add /proc/PID/x86_status, which sounds better in most respects to me.

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

[Li, Aubrey]

On 2019/4/10 9:58, Andy Lutomirski wrote:
> On Tue, Apr 9, 2019 at 6:55 PM Aubrey Li <aubrey.li@linux.intel.com> 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>
>> Cc: Linux API <linux-api@vger.kernel.org>
>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> ---
>>  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 2edbb657f859..331592a61718 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)
>> +{
>> +}
> 
> This pointlessly bloats other architectures.  Do this instead in an
> appropriate header:
> 
> #ifndef arch_proc_pid_status
> static inline void arch_proc_pid_status(...)
> {
> }
> #endif
> 

I saw a bunch of similar weak functions, is it not acceptable?

fs/proc$ grep weak *.c
cpuinfo.c:__weak void arch_freq_prepare_all(void)
meminfo.c:void __attribute__((weak)) arch_report_meminfo(struct seq_file *m)
vmcore.c:int __weak elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size)
vmcore.c:void __weak elfcorehdr_free(unsigned long long addr)
vmcore.c:ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
vmcore.c:ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
vmcore.c:int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
vmcore.c:ssize_t __weak

> Or add /proc/PID/x86_status, which sounds better in most respects to me.
> 

I didn't figure out how to make /proc/PID/x86_status invisible to other
architectures in an appropriate way, do you have any suggestions?

Thanks,
-Aubrey

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

[Andy Lutomirski]

On Tue, Apr 9, 2019 at 7:20 PM Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>
> On 2019/4/10 9:58, Andy Lutomirski wrote:
> > On Tue, Apr 9, 2019 at 6:55 PM Aubrey Li <aubrey.li@linux.intel.com> 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>
> >> Cc: Linux API <linux-api@vger.kernel.org>
> >> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> ---
> >>  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 2edbb657f859..331592a61718 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)
> >> +{
> >> +}
> >
> > This pointlessly bloats other architectures.  Do this instead in an
> > appropriate header:
> >
> > #ifndef arch_proc_pid_status
> > static inline void arch_proc_pid_status(...)
> > {
> > }
> > #endif
> >
>
> I saw a bunch of similar weak functions, is it not acceptable?
>
> fs/proc$ grep weak *.c
> cpuinfo.c:__weak void arch_freq_prepare_all(void)
> meminfo.c:void __attribute__((weak)) arch_report_meminfo(struct seq_file *m)
> vmcore.c:int __weak elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size)
> vmcore.c:void __weak elfcorehdr_free(unsigned long long addr)
> vmcore.c:ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
> vmcore.c:ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
> vmcore.c:int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
> vmcore.c:ssize_t __weak

I think they're acceptable, but I don't personally like them.

>
> > Or add /proc/PID/x86_status, which sounds better in most respects to me.
> >
>
> I didn't figure out how to make /proc/PID/x86_status invisible to other
> architectures in an appropriate way, do you have any suggestions?

#ifdef CONFIG_X86?

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

[Li, Aubrey]

On 2019/4/10 10:25, Andy Lutomirski wrote:
> On Tue, Apr 9, 2019 at 7:20 PM Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>>
>> On 2019/4/10 9:58, Andy Lutomirski wrote:
>>> On Tue, Apr 9, 2019 at 6:55 PM Aubrey Li <aubrey.li@linux.intel.com> 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>
>>>> Cc: Linux API <linux-api@vger.kernel.org>
>>>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>> ---
>>>>  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 2edbb657f859..331592a61718 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)
>>>> +{
>>>> +}
>>>
>>> This pointlessly bloats other architectures.  Do this instead in an
>>> appropriate header:
>>>
>>> #ifndef arch_proc_pid_status
>>> static inline void arch_proc_pid_status(...)
>>> {
>>> }
>>> #endif
>>>
>>
>> I saw a bunch of similar weak functions, is it not acceptable?
>>
>> fs/proc$ grep weak *.c
>> cpuinfo.c:__weak void arch_freq_prepare_all(void)
>> meminfo.c:void __attribute__((weak)) arch_report_meminfo(struct seq_file *m)
>> vmcore.c:int __weak elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size)
>> vmcore.c:void __weak elfcorehdr_free(unsigned long long addr)
>> vmcore.c:ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
>> vmcore.c:ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
>> vmcore.c:int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
>> vmcore.c:ssize_t __weak
> 
> I think they're acceptable, but I don't personally like them.
> 

okay, let me try to see if I can refine it in an appropriate way.

>>
>>> Or add /proc/PID/x86_status, which sounds better in most respects to me.
>>>
>>
>> I didn't figure out how to make /proc/PID/x86_status invisible to other
>> architectures in an appropriate way, do you have any suggestions?
> 
> #ifdef CONFIG_X86?
> 

I'm not sure if everyone like adding an arch #ifdef in a common file,
please allow me to wait a while to see if there are other comments.

Thanks,
-Aubrey

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

[Li, Aubrey]

On 2019/4/10 10:36, Li, Aubrey wrote:
> On 2019/4/10 10:25, Andy Lutomirski wrote:
>> On Tue, Apr 9, 2019 at 7:20 PM Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>>>
>>> On 2019/4/10 9:58, Andy Lutomirski wrote:
>>>> On Tue, Apr 9, 2019 at 6:55 PM Aubrey Li <aubrey.li@linux.intel.com> 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>
>>>>> Cc: Linux API <linux-api@vger.kernel.org>
>>>>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>> ---
>>>>>  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 2edbb657f859..331592a61718 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)
>>>>> +{
>>>>> +}
>>>>
>>>> This pointlessly bloats other architectures.  Do this instead in an
>>>> appropriate header:
>>>>
>>>> #ifndef arch_proc_pid_status
>>>> static inline void arch_proc_pid_status(...)
>>>> {
>>>> }
>>>> #endif
>>>>
>>>
>>> I saw a bunch of similar weak functions, is it not acceptable?
>>>
>>> fs/proc$ grep weak *.c
>>> cpuinfo.c:__weak void arch_freq_prepare_all(void)
>>> meminfo.c:void __attribute__((weak)) arch_report_meminfo(struct seq_file *m)
>>> vmcore.c:int __weak elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size)
>>> vmcore.c:void __weak elfcorehdr_free(unsigned long long addr)
>>> vmcore.c:ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
>>> vmcore.c:ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
>>> vmcore.c:int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
>>> vmcore.c:ssize_t __weak
>>
>> I think they're acceptable, but I don't personally like them.
>>
> 
> okay, let me try to see if I can refine it in an appropriate way.

Hi Andy,

Is this what you want? 

Thanks,
-Aubrey

====================================================================
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 2bb3a648fc12..82d77d3aefff 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -990,5 +990,8 @@ enum l1tf_mitigations {
 };
 
 extern enum l1tf_mitigations l1tf_mitigation;
+/* Add support for architecture specific output in /proc/pid/status */
+void arch_proc_pid_status(struct seq_file *m, struct task_struct *task);
+#define arch_proc_pid_status arch_proc_pid_status
 
 #endif /* _ASM_X86_PROCESSOR_H */
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 2edbb657f859..fd65a6ba2864 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -401,6 +401,11 @@ static inline void task_thp_status(struct seq_file *m, struct mm_struct *mm)
 	seq_printf(m, "THP_enabled:\t%d\n", thp_enabled);
 }
 
+/* Add support for architecture specific output in /proc/pid/status */
+#ifndef arch_proc_pid_status
+#define arch_proc_pid_status(m, task)
+#endif
+
 int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
 			struct pid *pid, struct task_struct *task)
 {
@@ -424,6 +429,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;
 }
 

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

[Andy Lutomirski]

On Tue, Apr 9, 2019 at 8:40 PM Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>
> On 2019/4/10 10:36, Li, Aubrey wrote:
> > On 2019/4/10 10:25, Andy Lutomirski wrote:
> >> On Tue, Apr 9, 2019 at 7:20 PM Li, Aubrey <aubrey.li@linux.intel.com> wrote:
> >>>
> >>> On 2019/4/10 9:58, Andy Lutomirski wrote:
> >>>> On Tue, Apr 9, 2019 at 6:55 PM Aubrey Li <aubrey.li@linux.intel.com> 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>
> >>>>> Cc: Linux API <linux-api@vger.kernel.org>
> >>>>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> >>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
> >>>>> ---
> >>>>>  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 2edbb657f859..331592a61718 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)
> >>>>> +{
> >>>>> +}
> >>>>
> >>>> This pointlessly bloats other architectures.  Do this instead in an
> >>>> appropriate header:
> >>>>
> >>>> #ifndef arch_proc_pid_status
> >>>> static inline void arch_proc_pid_status(...)
> >>>> {
> >>>> }
> >>>> #endif
> >>>>
> >>>
> >>> I saw a bunch of similar weak functions, is it not acceptable?
> >>>
> >>> fs/proc$ grep weak *.c
> >>> cpuinfo.c:__weak void arch_freq_prepare_all(void)
> >>> meminfo.c:void __attribute__((weak)) arch_report_meminfo(struct seq_file *m)
> >>> vmcore.c:int __weak elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size)
> >>> vmcore.c:void __weak elfcorehdr_free(unsigned long long addr)
> >>> vmcore.c:ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
> >>> vmcore.c:ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
> >>> vmcore.c:int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
> >>> vmcore.c:ssize_t __weak
> >>
> >> I think they're acceptable, but I don't personally like them.
> >>
> >
> > okay, let me try to see if I can refine it in an appropriate way.
>
> Hi Andy,
>
> Is this what you want?
>
> Thanks,
> -Aubrey
>
> ====================================================================
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 2bb3a648fc12..82d77d3aefff 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -990,5 +990,8 @@ enum l1tf_mitigations {
>  };
>
>  extern enum l1tf_mitigations l1tf_mitigation;
> +/* Add support for architecture specific output in /proc/pid/status */
> +void arch_proc_pid_status(struct seq_file *m, struct task_struct *task);
> +#define arch_proc_pid_status arch_proc_pid_status
>
>  #endif /* _ASM_X86_PROCESSOR_H */
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 2edbb657f859..fd65a6ba2864 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -401,6 +401,11 @@ static inline void task_thp_status(struct seq_file *m, struct mm_struct *mm)
>         seq_printf(m, "THP_enabled:\t%d\n", thp_enabled);
>  }
>
> +/* Add support for architecture specific output in /proc/pid/status */
> +#ifndef arch_proc_pid_status
> +#define arch_proc_pid_status(m, task)
> +#endif
> +
>  int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
>                         struct pid *pid, struct task_struct *task)
>  {
> @@ -424,6 +429,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;
>  }
>

Yes.  But I still think it would be nicer to separate the arch stuff
into its own file.  Others might reasonably disagree with me.

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

[Li, Aubrey]

On 2019/4/10 22:54, Andy Lutomirski wrote:
> On Tue, Apr 9, 2019 at 8:40 PM Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>>
>> On 2019/4/10 10:36, Li, Aubrey wrote:
>>> On 2019/4/10 10:25, Andy Lutomirski wrote:
>>>> On Tue, Apr 9, 2019 at 7:20 PM Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>>>>>
>>>>> On 2019/4/10 9:58, Andy Lutomirski wrote:
>>>>>> On Tue, Apr 9, 2019 at 6:55 PM Aubrey Li <aubrey.li@linux.intel.com> 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>
>>>>>>> Cc: Linux API <linux-api@vger.kernel.org>
>>>>>>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>>>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>>>> ---
>>>>>>>  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 2edbb657f859..331592a61718 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)
>>>>>>> +{
>>>>>>> +}
>>>>>>
>>>>>> This pointlessly bloats other architectures.  Do this instead in an
>>>>>> appropriate header:
>>>>>>
>>>>>> #ifndef arch_proc_pid_status
>>>>>> static inline void arch_proc_pid_status(...)
>>>>>> {
>>>>>> }
>>>>>> #endif
>>>>>>
>>>>>
>>>>> I saw a bunch of similar weak functions, is it not acceptable?
>>>>>
>>>>> fs/proc$ grep weak *.c
>>>>> cpuinfo.c:__weak void arch_freq_prepare_all(void)
>>>>> meminfo.c:void __attribute__((weak)) arch_report_meminfo(struct seq_file *m)
>>>>> vmcore.c:int __weak elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size)
>>>>> vmcore.c:void __weak elfcorehdr_free(unsigned long long addr)
>>>>> vmcore.c:ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
>>>>> vmcore.c:ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
>>>>> vmcore.c:int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
>>>>> vmcore.c:ssize_t __weak
>>>>
>>>> I think they're acceptable, but I don't personally like them.
>>>>
>>>
>>> okay, let me try to see if I can refine it in an appropriate way.
>>
>> Hi Andy,
>>
>> Is this what you want?
>>
>> Thanks,
>> -Aubrey
>>
>> ====================================================================
>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>> index 2bb3a648fc12..82d77d3aefff 100644
>> --- a/arch/x86/include/asm/processor.h
>> +++ b/arch/x86/include/asm/processor.h
>> @@ -990,5 +990,8 @@ enum l1tf_mitigations {
>>  };
>>
>>  extern enum l1tf_mitigations l1tf_mitigation;
>> +/* Add support for architecture specific output in /proc/pid/status */
>> +void arch_proc_pid_status(struct seq_file *m, struct task_struct *task);
>> +#define arch_proc_pid_status arch_proc_pid_status
>>
>>  #endif /* _ASM_X86_PROCESSOR_H */
>> diff --git a/fs/proc/array.c b/fs/proc/array.c
>> index 2edbb657f859..fd65a6ba2864 100644
>> --- a/fs/proc/array.c
>> +++ b/fs/proc/array.c
>> @@ -401,6 +401,11 @@ static inline void task_thp_status(struct seq_file *m, struct mm_struct *mm)
>>         seq_printf(m, "THP_enabled:\t%d\n", thp_enabled);
>>  }
>>
>> +/* Add support for architecture specific output in /proc/pid/status */
>> +#ifndef arch_proc_pid_status
>> +#define arch_proc_pid_status(m, task)
>> +#endif
>> +
>>  int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
>>                         struct pid *pid, struct task_struct *task)
>>  {
>> @@ -424,6 +429,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;
>>  }
>>
> 
> Yes.  But I still think it would be nicer to separate the arch stuff
> into its own file.  Others might reasonably disagree with me.
> 
I like arch_status, I proposed but no other arch shows interesting in it.

I think the problem is similar for x86_status, it does not make sense for
those x86 platform without AVX512 to have an empty arch file. I personally
don't like [arch]_status because the code may become unclean if more arches
added in future.

Maybe it's too early to have a separated arch staff file for now.

Thanks,
-Aubrey

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

[Li, Aubrey]

On 2019/4/11 9:02, Li, Aubrey wrote:
> On 2019/4/10 22:54, Andy Lutomirski wrote:
>> On Tue, Apr 9, 2019 at 8:40 PM Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>>>
>>> On 2019/4/10 10:36, Li, Aubrey wrote:
>>>> On 2019/4/10 10:25, Andy Lutomirski wrote:
>>>>> On Tue, Apr 9, 2019 at 7:20 PM Li, Aubrey <aubrey.li@linux.intel.com> wrote:
>>>>>>
>>>>>> On 2019/4/10 9:58, Andy Lutomirski wrote:
>>>>>>> On Tue, Apr 9, 2019 at 6:55 PM Aubrey Li <aubrey.li@linux.intel.com> 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>
>>>>>>>> Cc: Linux API <linux-api@vger.kernel.org>
>>>>>>>> Cc: Alexey Dobriyan <adobriyan@gmail.com>
>>>>>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>>>>>> ---
>>>>>>>>  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 2edbb657f859..331592a61718 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)
>>>>>>>> +{
>>>>>>>> +}
>>>>>>>
>>>>>>> This pointlessly bloats other architectures.  Do this instead in an
>>>>>>> appropriate header:
>>>>>>>
>>>>>>> #ifndef arch_proc_pid_status
>>>>>>> static inline void arch_proc_pid_status(...)
>>>>>>> {
>>>>>>> }
>>>>>>> #endif
>>>>>>>
>>>>>>
>>>>>> I saw a bunch of similar weak functions, is it not acceptable?
>>>>>>
>>>>>> fs/proc$ grep weak *.c
>>>>>> cpuinfo.c:__weak void arch_freq_prepare_all(void)
>>>>>> meminfo.c:void __attribute__((weak)) arch_report_meminfo(struct seq_file *m)
>>>>>> vmcore.c:int __weak elfcorehdr_alloc(unsigned long long *addr, unsigned long long *size)
>>>>>> vmcore.c:void __weak elfcorehdr_free(unsigned long long addr)
>>>>>> vmcore.c:ssize_t __weak elfcorehdr_read(char *buf, size_t count, u64 *ppos)
>>>>>> vmcore.c:ssize_t __weak elfcorehdr_read_notes(char *buf, size_t count, u64 *ppos)
>>>>>> vmcore.c:int __weak remap_oldmem_pfn_range(struct vm_area_struct *vma,
>>>>>> vmcore.c:ssize_t __weak
>>>>>
>>>>> I think they're acceptable, but I don't personally like them.
>>>>>
>>>>
>>>> okay, let me try to see if I can refine it in an appropriate way.
>>>
>>> Hi Andy,
>>>
>>> Is this what you want?
>>>
>>> Thanks,
>>> -Aubrey
>>>
>>> ====================================================================
>>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
>>> index 2bb3a648fc12..82d77d3aefff 100644
>>> --- a/arch/x86/include/asm/processor.h
>>> +++ b/arch/x86/include/asm/processor.h
>>> @@ -990,5 +990,8 @@ enum l1tf_mitigations {
>>>  };
>>>
>>>  extern enum l1tf_mitigations l1tf_mitigation;
>>> +/* Add support for architecture specific output in /proc/pid/status */
>>> +void arch_proc_pid_status(struct seq_file *m, struct task_struct *task);
>>> +#define arch_proc_pid_status arch_proc_pid_status
>>>
>>>  #endif /* _ASM_X86_PROCESSOR_H */
>>> diff --git a/fs/proc/array.c b/fs/proc/array.c
>>> index 2edbb657f859..fd65a6ba2864 100644
>>> --- a/fs/proc/array.c
>>> +++ b/fs/proc/array.c
>>> @@ -401,6 +401,11 @@ static inline void task_thp_status(struct seq_file *m, struct mm_struct *mm)
>>>         seq_printf(m, "THP_enabled:\t%d\n", thp_enabled);
>>>  }
>>>
>>> +/* Add support for architecture specific output in /proc/pid/status */
>>> +#ifndef arch_proc_pid_status
>>> +#define arch_proc_pid_status(m, task)
>>> +#endif
>>> +
>>>  int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
>>>                         struct pid *pid, struct task_struct *task)
>>>  {
>>> @@ -424,6 +429,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;
>>>  }
>>>
>>
>> Yes.  But I still think it would be nicer to separate the arch stuff
>> into its own file.  Others might reasonably disagree with me.
>>
> I like arch_status, I proposed but no other arch shows interesting in it.
> 
> I think the problem is similar for x86_status, it does not make sense for
> those x86 platform without AVX512 to have an empty arch file. I personally
> don't like [arch]_status because the code may become unclean if more arches
> added in future.
> 
> Maybe it's too early to have a separated arch staff file for now.

Hi Andy,

Is it acceptable to you if I make the above change and post v15?

Thanks,
-Aubrey