Re: [PATCH RFC 1/3] drivers: base: support cpu cache information interface to userspace via sysfs

On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:

Please don't use "raw" kobjects for this, use the device attribute
groups, that's what they are there for.  Bonus is that your code should
get a lot simpler when you do that.

thanks,

greg k-h

On 08/01/14 20:27, Greg Kroah-Hartman wrote:

On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:

quoted

From: Sudeep Holla <redacted>

This patch adds initial support for providing processor cache informatio=

n

quoted

to userspace through sysfs interface. This is based on x86 implementatio=

n

quoted

and hence the interface is intended to be fully compatible.

A per-cpu array of cache information maintained is used mainly for
sysfs-related book keeping.

Signed-off-by: Sudeep Holla <redacted>
---
 drivers/base/Makefile     |   2 +-
 drivers/base/cacheinfo.c  | 296 +++++++++++++++++++++++++++++++++++++++=

+++++++

quoted

 include/linux/cacheinfo.h |  43 +++++++
 3 files changed, 340 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/cacheinfo.c
 create mode 100644 include/linux/cacheinfo.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 94e8a80..76f07c8 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile

@@ -4,7 +4,7 @@ obj-y=09=09=09:=3D core.o bus.o dd.o syscore.o \
 =09=09=09   driver.o class.o platform.o \
 =09=09=09   cpu.o firmware.o init.o map.o devres.o \
 =09=09=09   attribute_container.o transport_class.o \
-=09=09=09   topology.o
+=09=09=09   topology.o cacheinfo.o
 obj-$(CONFIG_DEVTMPFS)=09+=3D devtmpfs.o
 obj-$(CONFIG_DMA_CMA) +=3D dma-contiguous.o
 obj-y=09=09=09+=3D power/

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
new file mode 100644
index 0000000..f436c31
--- /dev/null
+++ b/drivers/base/cacheinfo.c

@@ -0,0 +1,296 @@
+/*
+ * cacheinfo support - processor cache information via sysfs
+ *
+ * Copyright (C) 2013 ARM Ltd.
+ * All Rights Reserved
+ *
+ * Author: Sudeep Holla <sudeep.holla@arm.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 "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/bitops.h>
+#include <linux/cacheinfo.h>
+#include <linux/compiler.h>
+#include <linux/cpu.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/kobject.h>
+#include <linux/of.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/smp.h>
+#include <linux/sysfs.h>
+
+struct cache_attr {
+=09struct attribute attr;
+=09 ssize_t(*show) (unsigned int, unsigned short, char *);
+=09 ssize_t(*store) (unsigned int, unsigned short, const char *, size_t=

);

quoted

+};
+
+/* pointer to kobject for cpuX/cache */
+static DEFINE_PER_CPU(struct kobject *, ci_cache_kobject);
+#define per_cpu_cache_kobject(cpu)     (per_cpu(ci_cache_kobject, cpu))
+
+struct index_kobject {
+=09struct kobject kobj;
+=09unsigned int cpu;
+=09unsigned short index;
+};
+
+static cpumask_t cache_dev_map;
+
+/* pointer to array of kobjects for cpuX/cache/indexY */

=20
Please don't use "raw" kobjects for this, use the device attribute
groups, that's what they are there for.  Bonus is that your code should
get a lot simpler when you do that.
=20

Yes I now understand device attribute group simplifies the code, but I thin=
k
kobjects are still needed as we need to track both cpu and cache index.
By reusing only cpu device kobject, we can track cpu only.

Please correct me if I am missing to understand something here.

One thought I have is to make cache_info structure common to all architectu=
re
(for now its ARM specific) and introduce kobject in that similar to ia64
implementation. That even eliminates lot of weak functions defined.

Regards,
Sudeep

On Thu, Jan 09, 2014 at 07:19:00PM +0000, Sudeep Holla wrote:

On 08/01/14 20:27, Greg Kroah-Hartman wrote:

quoted

On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:

quoted

From: Sudeep Holla <redacted>

This patch adds initial support for providing processor cache information
to userspace through sysfs interface. This is based on x86 implementation
and hence the interface is intended to be fully compatible.

A per-cpu array of cache information maintained is used mainly for
sysfs-related book keeping.

Signed-off-by: Sudeep Holla <redacted>
---
 drivers/base/Makefile     |   2 +-
 drivers/base/cacheinfo.c  | 296 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cacheinfo.h |  43 +++++++
 3 files changed, 340 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/cacheinfo.c
 create mode 100644 include/linux/cacheinfo.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 94e8a80..76f07c8 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile

@@ -4,7 +4,7 @@ obj-y			:= core.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
-			   topology.o
+			   topology.o cacheinfo.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
new file mode 100644
index 0000000..f436c31
--- /dev/null
+++ b/drivers/base/cacheinfo.c

@@ -0,0 +1,296 @@
+/*
+ * cacheinfo support - processor cache information via sysfs
+ *
+ * Copyright (C) 2013 ARM Ltd.
+ * All Rights Reserved
+ *
+ * Author: Sudeep Holla <sudeep.holla@arm.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 "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/bitops.h>
+#include <linux/cacheinfo.h>
+#include <linux/compiler.h>
+#include <linux/cpu.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/kobject.h>
+#include <linux/of.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/smp.h>
+#include <linux/sysfs.h>
+
+struct cache_attr {
+	struct attribute attr;
+	 ssize_t(*show) (unsigned int, unsigned short, char *);
+	 ssize_t(*store) (unsigned int, unsigned short, const char *, size_t);
+};
+
+/* pointer to kobject for cpuX/cache */
+static DEFINE_PER_CPU(struct kobject *, ci_cache_kobject);
+#define per_cpu_cache_kobject(cpu)     (per_cpu(ci_cache_kobject, cpu))
+
+struct index_kobject {
+	struct kobject kobj;
+	unsigned int cpu;
+	unsigned short index;
+};
+
+static cpumask_t cache_dev_map;
+
+/* pointer to array of kobjects for cpuX/cache/indexY */

Please don't use "raw" kobjects for this, use the device attribute
groups, that's what they are there for.  Bonus is that your code should
get a lot simpler when you do that.

Yes I now understand device attribute group simplifies the code, but I think
kobjects are still needed as we need to track both cpu and cache index.
By reusing only cpu device kobject, we can track cpu only.

I don't understand, you are putting things under the cpu device object,
why do you care about a "cache" kobject?

One thought I have is to make cache_info structure common to all architecture
(for now its ARM specific) and introduce kobject in that similar to ia64
implementation. That even eliminates lot of weak functions defined.

Please don't use raw kobjects if at all possible, it's not good for a
variety of reasons (no userspace events, have to roll your own code,
etc.)

thanks,

greg k-h

On 09/01/14 19:31, Greg Kroah-Hartman wrote:

On Thu, Jan 09, 2014 at 07:19:00PM +0000, Sudeep Holla wrote:

quoted

On 08/01/14 20:27, Greg Kroah-Hartman wrote:

quoted

On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:

quoted

From: Sudeep Holla <redacted>

This patch adds initial support for providing processor cache informat=

ion

quoted

quoted

quoted

to userspace through sysfs interface. This is based on x86 implementat=

ion

quoted

quoted

quoted

and hence the interface is intended to be fully compatible.

A per-cpu array of cache information maintained is used mainly for
sysfs-related book keeping.

Signed-off-by: Sudeep Holla <redacted>
---
 drivers/base/Makefile     |   2 +-
 drivers/base/cacheinfo.c  | 296 +++++++++++++++++++++++++++++++++++++=

+++++++++

quoted

quoted

quoted

 include/linux/cacheinfo.h |  43 +++++++
 3 files changed, 340 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/cacheinfo.c
 create mode 100644 include/linux/cacheinfo.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 94e8a80..76f07c8 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile

@@ -4,7 +4,7 @@ obj-y=09=09=09:=3D core.o bus.o dd.o syscore.o \
 =09=09=09   driver.o class.o platform.o \
 =09=09=09   cpu.o firmware.o init.o map.o devres.o \
 =09=09=09   attribute_container.o transport_class.o \
-=09=09=09   topology.o
+=09=09=09   topology.o cacheinfo.o
 obj-$(CONFIG_DEVTMPFS)=09+=3D devtmpfs.o
 obj-$(CONFIG_DMA_CMA) +=3D dma-contiguous.o
 obj-y=09=09=09+=3D power/

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
new file mode 100644
index 0000000..f436c31
--- /dev/null
+++ b/drivers/base/cacheinfo.c

@@ -0,0 +1,296 @@
+/*
+ * cacheinfo support - processor cache information via sysfs
+ *
+ * Copyright (C) 2013 ARM Ltd.
+ * All Rights Reserved
+ *
+ * Author: Sudeep Holla <sudeep.holla@arm.com>
+ *
+ * This program is free software; you can redistribute it and/or modi=

fy

quoted

quoted

quoted

+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warrant=

y

quoted

quoted

quoted

+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/bitops.h>
+#include <linux/cacheinfo.h>
+#include <linux/compiler.h>
+#include <linux/cpu.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/kobject.h>
+#include <linux/of.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/smp.h>
+#include <linux/sysfs.h>
+
+struct cache_attr {
+=09struct attribute attr;
+=09 ssize_t(*show) (unsigned int, unsigned short, char *);
+=09 ssize_t(*store) (unsigned int, unsigned short, const char *, size=

_t);

quoted

quoted

quoted

+};
+
+/* pointer to kobject for cpuX/cache */
+static DEFINE_PER_CPU(struct kobject *, ci_cache_kobject);
+#define per_cpu_cache_kobject(cpu)     (per_cpu(ci_cache_kobject, cpu=

))

quoted

quoted

quoted

+
+struct index_kobject {
+=09struct kobject kobj;
+=09unsigned int cpu;
+=09unsigned short index;
+};
+
+static cpumask_t cache_dev_map;
+
+/* pointer to array of kobjects for cpuX/cache/indexY */

Please don't use "raw" kobjects for this, use the device attribute
groups, that's what they are there for.  Bonus is that your code should
get a lot simpler when you do that.

Yes I now understand device attribute group simplifies the code, but I t=

hink

quoted

kobjects are still needed as we need to track both cpu and cache index.
By reusing only cpu device kobject, we can track cpu only.

=20
I don't understand, you are putting things under the cpu device object,
why do you care about a "cache" kobject?
=20

Yes though the cache attributes are under cpu objects, it's hierarchical
something like:
/sys/devices/system/cpu/cpu<n>/cache/index<m>/<attribute_x>
<attribute_x> is unique for each pair of (cpu<n>, index<m>
index is more like cache level, but with 2 indices if they are separate(I$,=
D$)

quoted

One thought I have is to make cache_info structure common to all archite=

cture

quoted

(for now its ARM specific) and introduce kobject in that similar to ia64
implementation. That even eliminates lot of weak functions defined.

=20
Please don't use raw kobjects if at all possible, it's not good for a
variety of reasons (no userspace events, have to roll your own code,
etc.)
=20

Yes I understand, will try to explore other feasible solutions.

Regards,
Sudeep

On Thu, Jan 09, 2014 at 07:47:47PM +0000, Sudeep Holla wrote:

On 09/01/14 19:31, Greg Kroah-Hartman wrote:

quoted

On Thu, Jan 09, 2014 at 07:19:00PM +0000, Sudeep Holla wrote:

quoted

On 08/01/14 20:27, Greg Kroah-Hartman wrote:

quoted

On Wed, Jan 08, 2014 at 07:26:06PM +0000, Sudeep Holla wrote:

quoted

From: Sudeep Holla <redacted>

This patch adds initial support for providing processor cache information
to userspace through sysfs interface. This is based on x86 implementation
and hence the interface is intended to be fully compatible.

A per-cpu array of cache information maintained is used mainly for
sysfs-related book keeping.

Signed-off-by: Sudeep Holla <redacted>
---
 drivers/base/Makefile     |   2 +-
 drivers/base/cacheinfo.c  | 296 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/cacheinfo.h |  43 +++++++
 3 files changed, 340 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/cacheinfo.c
 create mode 100644 include/linux/cacheinfo.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 94e8a80..76f07c8 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile

@@ -4,7 +4,7 @@ obj-y			:= core.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
-			   topology.o
+			   topology.o cacheinfo.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
new file mode 100644
index 0000000..f436c31
--- /dev/null
+++ b/drivers/base/cacheinfo.c

@@ -0,0 +1,296 @@
+/*
+ * cacheinfo support - processor cache information via sysfs
+ *
+ * Copyright (C) 2013 ARM Ltd.
+ * All Rights Reserved
+ *
+ * Author: Sudeep Holla <sudeep.holla@arm.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 "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/bitops.h>
+#include <linux/cacheinfo.h>
+#include <linux/compiler.h>
+#include <linux/cpu.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/kobject.h>
+#include <linux/of.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/smp.h>
+#include <linux/sysfs.h>
+
+struct cache_attr {
+	struct attribute attr;
+	 ssize_t(*show) (unsigned int, unsigned short, char *);
+	 ssize_t(*store) (unsigned int, unsigned short, const char *, size_t);
+};
+
+/* pointer to kobject for cpuX/cache */
+static DEFINE_PER_CPU(struct kobject *, ci_cache_kobject);
+#define per_cpu_cache_kobject(cpu)     (per_cpu(ci_cache_kobject, cpu))
+
+struct index_kobject {
+	struct kobject kobj;
+	unsigned int cpu;
+	unsigned short index;
+};
+
+static cpumask_t cache_dev_map;
+
+/* pointer to array of kobjects for cpuX/cache/indexY */

Please don't use "raw" kobjects for this, use the device attribute
groups, that's what they are there for.  Bonus is that your code should
get a lot simpler when you do that.

Yes I now understand device attribute group simplifies the code, but I think
kobjects are still needed as we need to track both cpu and cache index.
By reusing only cpu device kobject, we can track cpu only.

I don't understand, you are putting things under the cpu device object,
why do you care about a "cache" kobject?

Yes though the cache attributes are under cpu objects, it's hierarchical
something like:
/sys/devices/system/cpu/cpu<n>/cache/index<m>/<attribute_x>
<attribute_x> is unique for each pair of (cpu<n>, index<m>
index is more like cache level, but with 2 indices if they are separate(I$,D$)

Then make the "cache" a struct device, and then create the attribute
group under that?  You'll want that anyway as you don't have any
visibility to userspace tools by using raw kobjects, they don't see them
at all (i.e. libudev and the like.)

thanks,

greg k-h