[PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Aubrey Li]

The architecture specific information of the running processes
could be useful to the userland. Add /proc/<pid>/arch_status
interface support to examine process architecture specific
information externally.

Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
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: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Linux API <linux-api@vger.kernel.org>
---
 arch/x86/Kconfig |  1 +
 fs/proc/Kconfig  | 10 ++++++++++
 fs/proc/base.c   | 23 +++++++++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 5ad92419be19..d5a9c5ddd453 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -208,6 +208,7 @@ config X86
 	select USER_STACKTRACE_SUPPORT
 	select VIRT_TO_BUS
 	select X86_FEATURE_NAMES		if PROC_FS
+	select PROC_PID_ARCH_STATUS		if PROC_FS
 
 config INSTRUCTION_DECODER
 	def_bool y
diff --git a/fs/proc/Kconfig b/fs/proc/Kconfig
index 817c02b13b1d..101bf5054e81 100644
--- a/fs/proc/Kconfig
+++ b/fs/proc/Kconfig
@@ -97,3 +97,13 @@ config PROC_CHILDREN
 
 	  Say Y if you are running any user-space software which takes benefit from
 	  this interface. For example, rkt is such a piece of software.
+
+config PROC_PID_ARCH_STATUS
+	bool "Enable /proc/<pid>/arch_status file"
+	default n
+	help
+	  Provides a way to examine process architecture specific information.
+	  See <file:Documentation/filesystems/proc.txt> for more information.
+
+	  Say Y if you are running any user-space software which wants to obtain
+	  process architecture specific information from this interface.
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6a803a0b75df..a890d9f12851 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -94,6 +94,7 @@
 #include <linux/sched/debug.h>
 #include <linux/sched/stat.h>
 #include <linux/posix-timers.h>
+#include <linux/processor.h>
 #include <trace/events/oom.h>
 #include "internal.h"
 #include "fd.h"
@@ -2957,6 +2958,22 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
 }
 #endif /* CONFIG_STACKLEAK_METRICS */
 
+/*
+ * Add support for task architecture specific output in /proc/pid/arch_status.
+ * task_arch_status() must be defined in asm/processor.h
+ */
+#ifdef CONFIG_PROC_PID_ARCH_STATUS
+# ifndef task_arch_status
+# define task_arch_status(m, task)
+# endif
+static int proc_pid_arch_status(struct seq_file *m, struct pid_namespace *ns,
+				struct pid *pid, struct task_struct *task)
+{
+	task_arch_status(m, task);
+	return 0;
+}
+#endif /* CONFIG_PROC_PID_ARCH_STATUS */
+
 /*
  * Thread groups
  */
@@ -3061,6 +3078,9 @@ static const struct pid_entry tgid_base_stuff[] = {
 #ifdef CONFIG_STACKLEAK_METRICS
 	ONE("stack_depth", S_IRUGO, proc_stack_depth),
 #endif
+#ifdef CONFIG_PROC_PID_ARCH_STATUS
+	ONE("arch_status", S_IRUGO, proc_pid_arch_status),
+#endif
 };
 
 static int proc_tgid_base_readdir(struct file *file, struct dir_context *ctx)
@@ -3449,6 +3469,9 @@ static const struct pid_entry tid_base_stuff[] = {
 #ifdef CONFIG_LIVEPATCH
 	ONE("patch_state",  S_IRUSR, proc_pid_patch_state),
 #endif
+#ifdef CONFIG_PROC_PID_ARCH_STATUS
+	ONE("arch_status", S_IRUGO, proc_pid_arch_status),
+#endif
 };
 
 static int proc_tid_base_readdir(struct file *file, struct dir_context *ctx)
-- 
2.17.1

[PATCH v17 2/3] /proc/pid/arch_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/arch_status | grep AVX512; 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/arch_status | grep AVX512
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: Thomas Gleixner <tglx@linutronix.de>
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: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Linux API <linux-api@vger.kernel.org>
---
 arch/x86/include/asm/processor.h |  6 +++++
 arch/x86/kernel/fpu/xstate.c     | 43 ++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 2bb3a648fc12..0728848473a2 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -991,4 +991,10 @@ enum l1tf_mitigations {
 
 extern enum l1tf_mitigations l1tf_mitigation;
 
+#ifdef CONFIG_PROC_PID_ARCH_STATUS
+/* Add support for task architecture specific output in /proc/pid/arch_status */
+void task_arch_status(struct seq_file *m, struct task_struct *task);
+#define task_arch_status task_arch_status
+#endif /* CONFIG_PROC_PID_ARCH_STATUS */
+
 #endif /* _ASM_X86_PROCESSOR_H */
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index d7432c2b1051..a0dda11ab72e 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -7,6 +7,7 @@
 #include <linux/cpu.h>
 #include <linux/mman.h>
 #include <linux/pkeys.h>
+#include <linux/seq_file.h>
 
 #include <asm/fpu/api.h>
 #include <asm/fpu/internal.h>
@@ -1243,3 +1244,45 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf)
 
 	return 0;
 }
+
+#ifdef CONFIG_PROC_PID_ARCH_STATUS
+/*
+ * 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 task_arch_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);
+}
+#endif /* CONFIG_PROC_PID_ARCH_STATUS */
-- 
2.17.1

Re: [PATCH v17 2/3] /proc/pid/arch_status: Add AVX-512 usage elapsed time

[Thomas Gleixner]

On Mon, 22 Apr 2019, Aubrey Li wrote:

$Subject: /proc/pid/arch_status: Add AVX-512 usage elapsed time

x86 patches surely have a different prefix.

>  
> +#ifdef CONFIG_PROC_PID_ARCH_STATUS
> +/* Add support for task architecture specific output in /proc/pid/arch_status */
> +void task_arch_status(struct seq_file *m, struct task_struct *task);
> +#define task_arch_status task_arch_status
> +#endif /* CONFIG_PROC_PID_ARCH_STATUS */

See previous reply.

Thanks,

	tglx

[PATCH v17 3/3] Documentation/filesystems/proc.txt: add arch_status file

[Aubrey Li]

Added /proc/<pid>/arch_status file, and added AVX512_elapsed_ms in
/proc/<pid>/arch_status. Report it in Documentation/filesystems/proc.txt

Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
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: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Linux API <linux-api@vger.kernel.org>
---
 Documentation/filesystems/proc.txt | 37 ++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 66cad5c86171..cf5114a8fb13 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -45,6 +45,7 @@ Table of Contents
   3.9   /proc/<pid>/map_files - Information about memory mapped files
   3.10  /proc/<pid>/timerslack_ns - Task timerslack value
   3.11	/proc/<pid>/patch_state - Livepatch patch operation state
+  3.12	/proc/<pid>/arch_status - Task architecture specific information
 
   4	Configuring procfs
   4.1	Mount options
@@ -1948,6 +1949,42 @@ patched.  If the patch is being enabled, then the task has already been
 patched.  If the patch is being disabled, then the task hasn't been
 unpatched yet.
 
+3.12 /proc/<pid>/arch_status - task architecture specific status
+-------------------------------------------------------------------
+When CONFIG_PROC_PID_ARCH_STATUS is enabled, this file displays the
+architecture specific status of the task.
+
+Example
+-------
+ $ cat /proc/6753/arch_status
+ AVX512_elapsed_ms:      8
+
+Description
+-----------
+
+ 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.
 
 ------------------------------------------------------------------------------
 Configuring procfs
-- 
2.17.1

Re: [PATCH v17 3/3] Documentation/filesystems/proc.txt: add arch_status file

[Thomas Gleixner]

On Mon, 22 Apr 2019, Aubrey Li wrote:
> +3.12 /proc/<pid>/arch_status - task architecture specific status
> +-------------------------------------------------------------------
> +When CONFIG_PROC_PID_ARCH_STATUS is enabled, this file displays the
> +architecture specific status of the task.
> +
> +Example
> +-------
> + $ cat /proc/6753/arch_status
> + AVX512_elapsed_ms:      8

Examples are nice, but as this is architecture specific this section needs
to list the entries which are exposed by a particular architecture.

x86 specific entries:
---------------------

  AVX512_elapsed_ms:
  ------------------
     If AVX512 is supported ....

Thanks,

	tglx

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Thomas Gleixner]

On Mon, 22 Apr 2019, Aubrey Li wrote:
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 5ad92419be19..d5a9c5ddd453 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -208,6 +208,7 @@ config X86
>  	select USER_STACKTRACE_SUPPORT
>  	select VIRT_TO_BUS
>  	select X86_FEATURE_NAMES		if PROC_FS
> +	select PROC_PID_ARCH_STATUS		if PROC_FS

Can you please stop mixing arch and proc code? There is no point in
enabling this on x86 right away.

> +
> +config PROC_PID_ARCH_STATUS
> +	bool "Enable /proc/<pid>/arch_status file"

Why is this switchable? x86 selects it if PROC_FS is enabled and all other
architectures are absolutely not interested in this.

> +	default n
> +	help
> +	  Provides a way to examine process architecture specific information.
> +	  See <file:Documentation/filesystems/proc.txt> for more information.

Which contains zero information about this file when only this patch is
applied. 

> @@ -94,6 +94,7 @@
>  #include <linux/sched/debug.h>
>  #include <linux/sched/stat.h>
>  #include <linux/posix-timers.h>
> +#include <linux/processor.h>

That include is required because it does NOT contain anything useful for
this, right?

> +/*
> + * Add support for task architecture specific output in /proc/pid/arch_status.
> + * task_arch_status() must be defined in asm/processor.h
> + */
> +#ifdef CONFIG_PROC_PID_ARCH_STATUS
> +# ifndef task_arch_status
> +# define task_arch_status(m, task)
> +# endif

What exactly is the point of this macro mess? If an architecture selects
CONFIG_PROC_PID_ARCH_STATUS then it has to provide proc_task_arch_status()
and the prototype should be in include/linux/proc_fs.h.

> +static int proc_pid_arch_status(struct seq_file *m, struct pid_namespace *ns,
> +				struct pid *pid, struct task_struct *task)
> +{
> +	task_arch_status(m, task);
> +	return 0;
> +}
> +#endif /* CONFIG_PROC_PID_ARCH_STATUS */

Thanks,

	tglx

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Li, Aubrey]

On 2019/4/25 5:18, Thomas Gleixner wrote:
> On Mon, 22 Apr 2019, Aubrey Li wrote:
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 5ad92419be19..d5a9c5ddd453 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -208,6 +208,7 @@ config X86
>>  	select USER_STACKTRACE_SUPPORT
>>  	select VIRT_TO_BUS
>>  	select X86_FEATURE_NAMES		if PROC_FS
>> +	select PROC_PID_ARCH_STATUS		if PROC_FS
> 
> Can you please stop mixing arch and proc code? There is no point in
> enabling this on x86 right away.
> 
>> +
>> +config PROC_PID_ARCH_STATUS
>> +	bool "Enable /proc/<pid>/arch_status file"
> 
> Why is this switchable? x86 selects it if PROC_FS is enabled and all other
> architectures are absolutely not interested in this.

Above and this, I was trying to avoid an empty arch_file on other architectures.
In previous proposal the entry only exists on the platform with AVX512.

> 
>> +	default n
>> +	help
>> +	  Provides a way to examine process architecture specific information.
>> +	  See <file:Documentation/filesystems/proc.txt> for more information.
> 
> Which contains zero information about this file when only this patch is
> applied. 
> 
>> @@ -94,6 +94,7 @@
>>  #include <linux/sched/debug.h>
>>  #include <linux/sched/stat.h>
>>  #include <linux/posix-timers.h>
>> +#include <linux/processor.h>
> 
> That include is required because it does NOT contain anything useful for
> this, right?
> 
>> +/*
>> + * Add support for task architecture specific output in /proc/pid/arch_status.
>> + * task_arch_status() must be defined in asm/processor.h
>> + */
>> +#ifdef CONFIG_PROC_PID_ARCH_STATUS
>> +# ifndef task_arch_status
>> +# define task_arch_status(m, task)
>> +# endif
> 
> What exactly is the point of this macro mess? If an architecture selects
> CONFIG_PROC_PID_ARCH_STATUS then it has to provide proc_task_arch_status()
> and the prototype should be in include/linux/proc_fs.h.

I was trying to address Andy's last comments. If we have the prototype in 
include/linux/proc_fs.h, we'll have a weak function definition in fs/proc/array.c,
which bloats other architectures.

In that way proc_task_arch_status() should be defined in asm/processor.h,
but proc_task_arch_status() has four parameters, I don't want unnecessary
"struct pid_namespace *ns" and "struct pid *pid" leaked into arch headers,
so I defined task_arch_status(m, task) to avoid that.

> 
>> +static int proc_pid_arch_status(struct seq_file *m, struct pid_namespace *ns,
>> +				struct pid *pid, struct task_struct *task)
>> +{
>> +	task_arch_status(m, task);
>> +	return 0;
>> +}
>> +#endif /* CONFIG_PROC_PID_ARCH_STATUS */
> 
> Thanks,
> 
> 	tglx
> 

Thanks,
-Aubrey

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Thomas Gleixner]

On Thu, 25 Apr 2019, Li, Aubrey wrote:

> On 2019/4/25 5:18, Thomas Gleixner wrote:
> > On Mon, 22 Apr 2019, Aubrey Li wrote:
> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >> index 5ad92419be19..d5a9c5ddd453 100644
> >> --- a/arch/x86/Kconfig
> >> +++ b/arch/x86/Kconfig
> >> @@ -208,6 +208,7 @@ config X86
> >>  	select USER_STACKTRACE_SUPPORT
> >>  	select VIRT_TO_BUS
> >>  	select X86_FEATURE_NAMES		if PROC_FS
> >> +	select PROC_PID_ARCH_STATUS		if PROC_FS
> > 
> > Can you please stop mixing arch and proc code? There is no point in
> > enabling this on x86 right away.
> > 
> >> +
> >> +config PROC_PID_ARCH_STATUS
> >> +	bool "Enable /proc/<pid>/arch_status file"
> > 
> > Why is this switchable? x86 selects it if PROC_FS is enabled and all other
> > architectures are absolutely not interested in this.
> 
> Above and this, I was trying to avoid an empty arch_file on other architectures.
> In previous proposal the entry only exists on the platform with AVX512.

What's the benefit of having a conditional enabled empty file for all other
architectures? Nothing AFAICT.
 
> >> +/*
> >> + * Add support for task architecture specific output in /proc/pid/arch_status.
> >> + * task_arch_status() must be defined in asm/processor.h
> >> + */
> >> +#ifdef CONFIG_PROC_PID_ARCH_STATUS
> >> +# ifndef task_arch_status
> >> +# define task_arch_status(m, task)
> >> +# endif
> > 
> > What exactly is the point of this macro mess? If an architecture selects
> > CONFIG_PROC_PID_ARCH_STATUS then it has to provide proc_task_arch_status()
> > and the prototype should be in include/linux/proc_fs.h.
> 
> I was trying to address Andy's last comments. If we have the prototype in 
> include/linux/proc_fs.h, we'll have a weak function definition in fs/proc/array.c,
> which bloats other architectures.
> 
> In that way proc_task_arch_status() should be defined in asm/processor.h,
> but proc_task_arch_status() has four parameters, I don't want unnecessary
> "struct pid_namespace *ns" and "struct pid *pid" leaked into arch headers,
> so I defined task_arch_status(m, task) to avoid that.

This #define mess is ugly and pointless. Let the arch select
CONFIG_PROC_PID_ARCH_STATUS and if it selects it it has to provide the
function. No waek function required at all.

Thanks,

	tglx

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Li, Aubrey]

On 2019/4/25 15:20, Thomas Gleixner wrote:
> On Thu, 25 Apr 2019, Li, Aubrey wrote:
> 
>> On 2019/4/25 5:18, Thomas Gleixner wrote:
>>> On Mon, 22 Apr 2019, Aubrey Li wrote:
>>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>>> index 5ad92419be19..d5a9c5ddd453 100644
>>>> --- a/arch/x86/Kconfig
>>>> +++ b/arch/x86/Kconfig
>>>> @@ -208,6 +208,7 @@ config X86
>>>>  	select USER_STACKTRACE_SUPPORT
>>>>  	select VIRT_TO_BUS
>>>>  	select X86_FEATURE_NAMES		if PROC_FS
>>>> +	select PROC_PID_ARCH_STATUS		if PROC_FS
>>>
>>> Can you please stop mixing arch and proc code? There is no point in
>>> enabling this on x86 right away.
>>>
>>>> +
>>>> +config PROC_PID_ARCH_STATUS
>>>> +	bool "Enable /proc/<pid>/arch_status file"
>>>
>>> Why is this switchable? x86 selects it if PROC_FS is enabled and all other
>>> architectures are absolutely not interested in this.
>>
>> Above and this, I was trying to avoid an empty arch_file on other architectures.
>> In previous proposal the entry only exists on the platform with AVX512.
> 
> What's the benefit of having a conditional enabled empty file for all other
> architectures? Nothing AFAICT.
>  
>>>> +/*
>>>> + * Add support for task architecture specific output in /proc/pid/arch_status.
>>>> + * task_arch_status() must be defined in asm/processor.h
>>>> + */
>>>> +#ifdef CONFIG_PROC_PID_ARCH_STATUS
>>>> +# ifndef task_arch_status
>>>> +# define task_arch_status(m, task)
>>>> +# endif
>>>
>>> What exactly is the point of this macro mess? If an architecture selects
>>> CONFIG_PROC_PID_ARCH_STATUS then it has to provide proc_task_arch_status()
>>> and the prototype should be in include/linux/proc_fs.h.
>>
>> I was trying to address Andy's last comments. If we have the prototype in 
>> include/linux/proc_fs.h, we'll have a weak function definition in fs/proc/array.c,
>> which bloats other architectures.
>>
>> In that way proc_task_arch_status() should be defined in asm/processor.h,
>> but proc_task_arch_status() has four parameters, I don't want unnecessary
>> "struct pid_namespace *ns" and "struct pid *pid" leaked into arch headers,
>> so I defined task_arch_status(m, task) to avoid that.
> 
> This #define mess is ugly and pointless.

> Let the arch select CONFIG_PROC_PID_ARCH_STATUS 

Sorry, I didn't get the point here, above you mentioned not mixing arch and proc code
and not enabling this on x86 right away, then how to let x86 select it?

Thanks,
-Aubrey

>and if it selects it it has to provide the function. No waek function required at all.
> 
> Thanks,
> 
> 	tglx
> 

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Thomas Gleixner]

On Thu, 25 Apr 2019, Li, Aubrey wrote:
> On 2019/4/25 15:20, Thomas Gleixner wrote:
> > Let the arch select CONFIG_PROC_PID_ARCH_STATUS 
> 
> Sorry, I didn't get the point here, above you mentioned not mixing arch and proc code
> and not enabling this on x86 right away, then how to let x86 select it?
> 

By using 'select PROC_PID_ARCH_STATUS' in the arch/xxxx/Kconfig file in the
patch which implements the required arch function perhaps?

Thanks,

	tglx

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Li, Aubrey]

On 2019/4/25 16:20, Thomas Gleixner wrote:
> On Thu, 25 Apr 2019, Li, Aubrey wrote:
>> On 2019/4/25 15:20, Thomas Gleixner wrote:
>>> Let the arch select CONFIG_PROC_PID_ARCH_STATUS 
>>
>> Sorry, I didn't get the point here, above you mentioned not mixing arch and proc code
>> and not enabling this on x86 right away, then how to let x86 select it?
>>
> 
> By using 'select PROC_PID_ARCH_STATUS' in the arch/xxxx/Kconfig file in the
> patch which implements the required arch function perhaps?

Oh,oh, I misunderstood, thanks for the clarification!

Thanks,
-Aubrey

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Enrico Weigelt, metux IT consult]

On 25.04.19 03:50, Li, Aubrey wrote:

>>> +>>> +config PROC_PID_ARCH_STATUS>>> +	bool "Enable
/proc/<pid>/arch_status file">>>> Why is this switchable? x86 selects it
if PROC_FS is enabled and all other>> architectures are absolutely not
interested in this.> > Above and this, I was trying to avoid an empty
arch_file on other architectures.> In previous proposal the entry only
exists on the platform with AVX512.
Why not making it depend on those archs that actually support it ?

> In that way proc_task_arch_status() should be defined in asm/processor.h,> but proc_task_arch_status() has four parameters, I don't want
unnecessary> "struct pid_namespace *ns" and "struct pid *pid" leaked
into arch headers,> so I defined task_arch_status(m, task) to avoid that.

--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Li, Aubrey]

On 2019/4/25 18:11, Enrico Weigelt, metux IT consult wrote:
> On 25.04.19 03:50, Li, Aubrey wrote:
> 
>>>> +>>> +config PROC_PID_ARCH_STATUS>>> +	bool "Enable
> /proc/<pid>/arch_status file">>>> Why is this switchable? x86 selects it
> if PROC_FS is enabled and all other>> architectures are absolutely not
> interested in this.> > Above and this, I was trying to avoid an empty
> arch_file on other architectures.> In previous proposal the entry only
> exists on the platform with AVX512.
> Why not making it depend on those archs that actually support it ?

Yep, I'll make it disabled by default and not switchable and let arch select it.

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Enrico Weigelt, metux IT consult]

On 25.04.19 12:42, Li, Aubrey wrote:
> 
> Yep, I'll make it disabled by default and not switchable and let arch select it.
> 

That's not quite what I've suggested. Instead:

#1: make the switch depend on the arch's that support it
#2: still leave it selectable to the user, so somebody who doesn't need
    it, can just disable it.

(I, personally, tend to disable everything I don't need)


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Thomas Gleixner]

On Thu, 25 Apr 2019, Enrico Weigelt, metux IT consult wrote:

> On 25.04.19 12:42, Li, Aubrey wrote:
> > 
> > Yep, I'll make it disabled by default and not switchable and let arch select it.
> > 
> 
> That's not quite what I've suggested. Instead:
> 
> #1: make the switch depend on the arch's that support it

No. That's what select is for.

> #2: still leave it selectable to the user, so somebody who doesn't need
>     it, can just disable it.

Well, the number of knobs is exploding over time and the number of people
actually tweaking them is close to 0. So no, we don't want to have the
extra tunable for everything and the world.

Thanks,

	tglx

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Enrico Weigelt, metux IT consult]

On 25.04.19 12:50, Thomas Gleixner wrote:
> On Thu, 25 Apr 2019, Enrico Weigelt, metux IT consult wrote:
> 
>> On 25.04.19 12:42, Li, Aubrey wrote:
>>>
>>> Yep, I'll make it disabled by default and not switchable and let arch select it.
>>>
>>
>> That's not quite what I've suggested. Instead:
>>
>> #1: make the switch depend on the arch's that support it
> 
> No. That's what select is for.

Just for clarification: I've proposed the depend, because not only some
archs will support it - and avoid masses of #ifdef's in the code.
Therefore, it can only be enabled, when the archi supports it.

But if you insist in not having it configurable, letting the arch just
select this feature, your approach makes sense.

>> #2: still leave it selectable to the user, so somebody who doesn't need
>>     it, can just disable it.
> 
> Well, the number of knobs is exploding over time and the number of people
> actually tweaking them is close to 0. So no, we don't want to have the
> extra tunable for everything and the world.

The great configurability often is one of the major arguments for using
Linux in the first place.

Would you propose killing all the CONFIG_EMBEDDED/CONFIG_EXPERT
related knobs ?


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Thomas Gleixner]

On Fri, 26 Apr 2019, Enrico Weigelt, metux IT consult wrote:
> On 25.04.19 12:50, Thomas Gleixner wrote:
> > On Thu, 25 Apr 2019, Enrico Weigelt, metux IT consult wrote:
> > 
> >> On 25.04.19 12:42, Li, Aubrey wrote:
> >>>
> >>> Yep, I'll make it disabled by default and not switchable and let arch select it.
> >>>
> >>
> >> That's not quite what I've suggested. Instead:
> >>
> >> #1: make the switch depend on the arch's that support it
> > 
> > No. That's what select is for.
> 
> Just for clarification: I've proposed the depend, because not only some
> archs will support it - and avoid masses of #ifdef's in the code.
> Therefore, it can only be enabled, when the archi supports it.

What has the way how you enable support to do with masses of #ifdefs?
Absolutely nothing.

> But if you insist in not having it configurable, letting the arch just
> select this feature, your approach makes sense.

Even if you make it configurable, then having:

     depends on ARCH1 ... ARCHN

is just wrong. That's what dependency config symbols are for which can be
selected by the arch.
 
> >> #2: still leave it selectable to the user, so somebody who doesn't need
> >>     it, can just disable it.
> > 
> > Well, the number of knobs is exploding over time and the number of people
> > actually tweaking them is close to 0. So no, we don't want to have the
> > extra tunable for everything and the world.
> 
> The great configurability often is one of the major arguments for using
> Linux in the first place.
> 
> Would you propose killing all the CONFIG_EMBEDDED/CONFIG_EXPERT
> related knobs ?

No, but adding knobs for every tiny piece of code does not make the whole
thing any better in terms of usability and maintainability.

Thanks,

	tglx

Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status

[Enrico Weigelt, metux IT consult]

On 24.04.19 23:18, Thomas Gleixner wrote:

Hi,

>> +config PROC_PID_ARCH_STATUS
>> +	bool "Enable /proc/<pid>/arch_status file"
> 
> Why is this switchable? x86 selects it if PROC_FS is enabled and all other
> architectures are absolutely not interested in this.

IMHO, it's good to have a switch, but that way doesn't make much sense.
Instead, I'd do it the other way round: make that switch depending on
those archs that actually support it. Something like this:

config PROC_PID_ARCH_STATUS
	bool "Enable /proc/<pid>/arch_status file"
	depends on PROC_FS
	depends on BROKEN

When x86 comes in, it would change to:

config PROC_PID_ARCH_STATUS
	bool "Enable /proc/<pid>/arch_status file"
	depends on PROC_FS
	depends on X86

And later arm coming in:

config PROC_PID_ARCH_STATUS
	bool "Enable /proc/<pid>/arch_status file"
	depends on PROC_FS
	depends on X86 || ARM

>> +	default n
>> +	help
>> +	  Provides a way to examine process architecture specific information.
>> +	  See <file:Documentation/filesystems/proc.txt> for more information.
> 
> Which contains zero information about this file when only this patch is
> applied. 

hmm, the patch alone doesn't do anything useful anyway. it only becomes
useful with subsequent patches that add some arch. I wonder if there's
anything more useful to document at that point.

>> +/*
>> + * Add support for task architecture specific output in /proc/pid/arch_status.
>> + * task_arch_status() must be defined in asm/processor.h
>> + */
>> +#ifdef CONFIG_PROC_PID_ARCH_STATUS
>> +# ifndef task_arch_status
>> +# define task_arch_status(m, task)
>> +# endif
> 
> What exactly is the point of this macro mess? If an architecture selects
> CONFIG_PROC_PID_ARCH_STATUS then it has to provide proc_task_arch_status()
> and the prototype should be in include/linux/proc_fs.h.

ACK.

>> +static int proc_pid_arch_status(struct seq_file *m, struct pid_namespace *ns,
>> +				struct pid *pid, struct task_struct *task)
>> +{
>> +	task_arch_status(m, task);
>> +	return 0;
>> +}

Is that wrapper really neeeded ?


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287