Re: [RFC PATCH 1/3] Add sysfs based cpuinfo structure.

Re: [RFC PATCH 1/3] Add sysfs based cpuinfo structure.

[Greg KH]

On Thu, Jun 08, 2017 at 06:56:13PM +0200, Felix Schnizlein wrote:
> Create a new sysfs attribute group called 'cpuinfo' for each
> online cpu. The cleaned up cpuinfo shows up in a sysfs
> subdirectory here: /sys/devices/system/cpu/cpu*/cpuinfo.
> 
> Define preprocessor macros (CPUINFO_DEFINE_* and CPUINFO_ATTR) to make
> defining new sysfs attributes for cpuinfo more easy.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.com>

But not you?

> ---
>  drivers/base/Kconfig       |  7 ++++++
>  drivers/base/Makefile      |  1 +
>  drivers/base/cpuinfo.c     | 57 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/cpuhotplug.h |  1 +
>  include/linux/cpuinfo.h    | 47 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 113 insertions(+)
>  create mode 100644 drivers/base/cpuinfo.c
>  create mode 100644 include/linux/cpuinfo.h
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index d718ae4..7900e31 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -224,6 +224,13 @@ config DEBUG_TEST_DRIVER_REMOVE
>  	  unusable. You should say N here unless you are explicitly looking to
>  	  test this functionality.
>  
> +config CPUINFO_SYSFS
> +	bool "Enable sysfs based cpuinfo"
> +	depends on SYSFS && X86
> +	default y
> +	help
> +	  This option enables cpuinfo in sysfs.
> +
>  source "drivers/base/test/Kconfig"
>  
>  config SYS_HYPERVISOR
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index f2816f6..cbbba07 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_SOC_BUS) += soc.o
>  obj-$(CONFIG_PINCTRL) += pinctrl.o
>  obj-$(CONFIG_DEV_COREDUMP) += devcoredump.o
>  obj-$(CONFIG_GENERIC_MSI_IRQ_DOMAIN) += platform-msi.o
> +obj-$(CONFIG_CPUINFO_SYSFS) += cpuinfo.o
>  
>  obj-y			+= test/
>  
> diff --git a/drivers/base/cpuinfo.c b/drivers/base/cpuinfo.c
> new file mode 100644
> index 0000000..52c21f2
> --- /dev/null
> +++ b/drivers/base/cpuinfo.c
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright (C) 2017 SUSE Linux GmbH
> + * Written by: Felix Schnizlein <fschnizlein@suse.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; see the file COPYING.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301 USA.

Did you run this through checkpatch.pl first?  Please do so :)

thanks,

greg k-h

Re: [RFC PATCH 2/3] Implement sysfs based cpuinfo for x86 cpus.

[Greg KH]

On Thu, Jun 08, 2017 at 06:56:14PM +0200, Felix Schnizlein wrote:
> A few entries have been omitted, due to already existing information in
> sysfs:
> 
> - microcode -> /sys/devices/system/cpu/cpu*/microcode/version
> - cache -> /sys/devices/system/cpu/cpu*/cache/index*/size
> - core_id -> /sys/devices/system/cpu/cpu*/topology/core_id
> - siblings -> /sys/devices/system/cpu/cpu*/topology/core_siblings_list
> - pyshical_id -> /sys/devices/system/cpu/cpu*/topology/physical_package_id
> 
> Following attributes have been removed due to obsolete information or
> legacy processors: wp, power management, f00f bug, coma bug, fdiv bug
> 
> Signed-off-by: Thomas Renninger <trenn@suse.com>

Again, no signoff from you?

> ---
>  arch/x86/kernel/Makefile        |   1 +
>  arch/x86/kernel/cpuinfo_sysfs.c | 166 ++++++++++++++++++++++++++++++++++++++++
>  drivers/base/cpuinfo.c          |   4 -
>  3 files changed, 167 insertions(+), 4 deletions(-)
>  create mode 100644 arch/x86/kernel/cpuinfo_sysfs.c
> 

When you add new sysfs entries, you have to also add new
Documentation/ABI/ entries.

thanks,

greg k-h

Re: [RFC PATCH 3/3] Add deprecation warning to cpuinfo proc

[Greg KH]

On Thu, Jun 08, 2017 at 06:56:15PM +0200, Felix Schnizlein wrote:
> Enable deprecation warning if the kernel was compiled with sysfs cpuinfo.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.com>
> ---
>  fs/proc/cpuinfo.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/proc/cpuinfo.c b/fs/proc/cpuinfo.c
> index 06f4d31..044f56e 100644
> --- a/fs/proc/cpuinfo.c
> +++ b/fs/proc/cpuinfo.c
> @@ -6,6 +6,10 @@
>  extern const struct seq_operations cpuinfo_op;
>  static int cpuinfo_open(struct inode *inode, struct file *file)
>  {
> +#ifdef CONFIG_CPUINFO_SYSFS
> +	pr_info("Do not access deprecated file /proc/cpuinfo, "
> +		"opened by: (%s,%d)\n", current->comm, current->pid);

Hah, that's not going to happen.  Do you know just how many different
programs open this file for no good reason?  Well, they think it's a
good reason...

You are just going to spam syslog so hard, it's going to be annoying.
This patch is not ok at all, sorry.

It's not like we are ever going to get rid of /proc/cpuinfo so it's not
a big deal.

thanks,

greg k-h

Re: [RFC PATCH 1/3] Add sysfs based cpuinfo structure.

[Greg KH]

On Thu, Jun 08, 2017 at 06:56:13PM +0200, Felix Schnizlein wrote:
> Create a new sysfs attribute group called 'cpuinfo' for each
> online cpu. The cleaned up cpuinfo shows up in a sysfs
> subdirectory here: /sys/devices/system/cpu/cpu*/cpuinfo.
> 
> Define preprocessor macros (CPUINFO_DEFINE_* and CPUINFO_ATTR) to make
> defining new sysfs attributes for cpuinfo more easy.
> 
> Signed-off-by: Thomas Renninger <trenn@suse.com>
> ---
>  drivers/base/Kconfig       |  7 ++++++
>  drivers/base/Makefile      |  1 +
>  drivers/base/cpuinfo.c     | 57 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/cpuhotplug.h |  1 +
>  include/linux/cpuinfo.h    | 47 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 113 insertions(+)
>  create mode 100644 drivers/base/cpuinfo.c
>  create mode 100644 include/linux/cpuinfo.h
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index d718ae4..7900e31 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -224,6 +224,13 @@ config DEBUG_TEST_DRIVER_REMOVE
>  	  unusable. You should say N here unless you are explicitly looking to
>  	  test this functionality.
>  
> +config CPUINFO_SYSFS
> +	bool "Enable sysfs based cpuinfo"
> +	depends on SYSFS && X86
> +	default y
> +	help
> +	  This option enables cpuinfo in sysfs.

Why would you not want this?  Why make it an option at all?

Re: [RFC PATCH 2/3] Implement sysfs based cpuinfo for x86 cpus.

[Greg KH]

On Thu, Jun 08, 2017 at 06:56:14PM +0200, Felix Schnizlein wrote:
> A few entries have been omitted, due to already existing information in
> sysfs:
> 
> - microcode -> /sys/devices/system/cpu/cpu*/microcode/version
> - cache -> /sys/devices/system/cpu/cpu*/cache/index*/size
> - core_id -> /sys/devices/system/cpu/cpu*/topology/core_id
> - siblings -> /sys/devices/system/cpu/cpu*/topology/core_siblings_list
> - pyshical_id -> /sys/devices/system/cpu/cpu*/topology/physical_package_id
> 
> Following attributes have been removed due to obsolete information or
> legacy processors: wp, power management, f00f bug, coma bug, fdiv bug
> 
> Signed-off-by: Thomas Renninger <trenn@suse.com>

???

> ---
>  arch/x86/kernel/Makefile        |   1 +
>  arch/x86/kernel/cpuinfo_sysfs.c | 166 ++++++++++++++++++++++++++++++++++++++++
>  drivers/base/cpuinfo.c          |   4 -
>  3 files changed, 167 insertions(+), 4 deletions(-)
>  create mode 100644 arch/x86/kernel/cpuinfo_sysfs.c
> 
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 4b99423..5c689d9 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -65,6 +65,7 @@ obj-y				+= step.o
>  obj-$(CONFIG_INTEL_TXT)		+= tboot.o
>  obj-$(CONFIG_ISA_DMA_API)	+= i8237.o
>  obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
> +obj-$(CONFIG_CPUINFO_SYSFS)     += cpuinfo_sysfs.o

Use tabs properly please.

Re: [RFC PATCH 2/3] Implement sysfs based cpuinfo for x86 cpus.

[Thomas Renninger]

On Thursday, June 08, 2017 08:24:01 PM Greg KH wrote:
> On Thu, Jun 08, 2017 at 06:56:14PM +0200, Felix Schnizlein wrote:
> > ---
> >  arch/x86/kernel/Makefile        |   1 +
> >  arch/x86/kernel/cpuinfo_sysfs.c | 166 
++++++++++++++++++++++++++++++++++++++++
> >  drivers/base/cpuinfo.c          |   4 -
> >  3 files changed, 167 insertions(+), 4 deletions(-)
> >  create mode 100644 arch/x86/kernel/cpuinfo_sysfs.c
> > 
> 
> When you add new sysfs entries, you have to also add new
> Documentation/ABI/ entries.

This one seem to be rather unmaintained?
There even is:
Documentation/ABI/stable/sysfs-devices-system-cpu
this patchset would have to add:
Documentation/ABI/stable/sysfs-devices-system-cpu-cpuinfo
then.

But:
Documentation/ABI/stable/sysfs-devices-system-cpu
describes
/sys/devices/system/cpu/dscr_default
and
/sys/devices/system/cpu/cpu[0-9]+/dscr

which I have never seen.

Much more important values in there like:
/sys/devices/system/cpu/topology
/sys/devices/system/cpu/microcode
/sys/devices/system/cpu/cpuidle
/sys/devices/system/cpu/cpufreq
/sys/devices/system/cpu/{online,offline}

are not described there at all.

I wonder whether .../cpu/dscr_default still exists in the
kernel, a quick grep did not reveal anything.
A
Source:
tag would be a nice non-optional addition in the README.

We can also send definitions for topology/microcode/cache/..
to catch up a bit there again, not sure that makes much sense.

Be aware that the output of /proc/cpuinfo is rather arbitrary
depending on architecure and some build options.

      Thomas

Re: [RFC PATCH 2/3] Implement sysfs based cpuinfo for x86 cpus.

[Brice Goglin]

Le 09/06/2017 15:28, Thomas Renninger a écrit :
> On Thursday, June 08, 2017 08:24:01 PM Greg KH wrote:
>> On Thu, Jun 08, 2017 at 06:56:14PM +0200, Felix Schnizlein wrote:
>>> ---
>>>  arch/x86/kernel/Makefile        |   1 +
>>>  arch/x86/kernel/cpuinfo_sysfs.c | 166 
> ++++++++++++++++++++++++++++++++++++++++
>>>  drivers/base/cpuinfo.c          |   4 -
>>>  3 files changed, 167 insertions(+), 4 deletions(-)
>>>  create mode 100644 arch/x86/kernel/cpuinfo_sysfs.c
>>>
>> When you add new sysfs entries, you have to also add new
>> Documentation/ABI/ entries.
> This one seem to be rather unmaintained?
> There even is:
> Documentation/ABI/stable/sysfs-devices-system-cpu
> this patchset would have to add:
> Documentation/ABI/stable/sysfs-devices-system-cpu-cpuinfo
> then.
>
> But:
> Documentation/ABI/stable/sysfs-devices-system-cpu
> describes
> /sys/devices/system/cpu/dscr_default
> and
> /sys/devices/system/cpu/cpu[0-9]+/dscr
>
> which I have never seen.
>
> Much more important values in there like:
> /sys/devices/system/cpu/topology
> /sys/devices/system/cpu/microcode
> /sys/devices/system/cpu/cpuidle
> /sys/devices/system/cpu/cpufreq
> /sys/devices/system/cpu/{online,offline}

They are documented in
Documentation/ABI/testing/sysfs-devices-system-cpu (which doesn't
contain dscr)

Brice


>
> are not described there at all.
>
> I wonder whether .../cpu/dscr_default still exists in the
> kernel, a quick grep did not reveal anything.
> A
> Source:
> tag would be a nice non-optional addition in the README.
>
> We can also send definitions for topology/microcode/cache/..
> to catch up a bit there again, not sure that makes much sense.
>
> Be aware that the output of /proc/cpuinfo is rather arbitrary
> depending on architecure and some build options.
>
>       Thomas