v2:
As requested by Greg KH: avoid negative logic and use uppercase defines.
v1:
Create die and cluster cpu topology sysfs attributes only if an
architecture makes uses of it, instead of creating them always for all
architectures with bogus default values.
Also change the book and drawer cpu topology macros so they match all
all other topology macros.
v1: https://lore.kernel.org/lkml/[email protected]/
Heiko Carstens (3):
topology/sysfs: export die attributes only if an architectures has support
topology/sysfs: export cluster attributes only if an architectures has support
topology/sysfs: rework book and drawer topology ifdefery
Documentation/admin-guide/cputopology.rst | 33 +++++++++++------------
drivers/base/topology.c | 28 ++++++++++++++-----
include/linux/topology.h | 25 +++++++++++++++++
3 files changed, 62 insertions(+), 24 deletions(-)
--
2.32.0
The die_id and die_cpus topology sysfs attributes have been added with
commit 0e344d8c709f ("cpu/topology: Export die_id") and commit
2e4c54dac7b3 ("topology: Create core_cpus and die_cpus sysfs attributes").
While they are currently only used and useful for x86 they are still
present with bogus default values for all architectures. Instead of
enforcing such new sysfs attributes to all architectures, make them
only optional visible if an architecture opts in by defining both the
topology_die_id and topology_die_cpumask attributes.
This is similar to what was done when the book and drawer topology
levels were introduced: avoid useless and therefore confusing sysfs
attributes for architectures which cannot make use of them.
This should not break any existing applications, since this is a
rather new interface and applications should be able to handle also
older kernel versions without such attributes - besides that they
contain only useful information for x86.
Signed-off-by: Heiko Carstens <[email protected]>
---
Documentation/admin-guide/cputopology.rst | 3 +++
drivers/base/topology.c | 8 ++++++++
include/linux/topology.h | 4 ++++
3 files changed, 15 insertions(+)
diff --git a/Documentation/admin-guide/cputopology.rst b/Documentation/admin-guide/cputopology.rst
index 6b62e182baf4..c68d07533c45 100644
--- a/Documentation/admin-guide/cputopology.rst
+++ b/Documentation/admin-guide/cputopology.rst
@@ -11,6 +11,9 @@ Architecture-neutral, drivers/base/topology.c, exports these attributes.
However, the book and drawer related sysfs files will only be created if
CONFIG_SCHED_BOOK and CONFIG_SCHED_DRAWER are selected, respectively.
+The die hierarchy related sysfs files will only be created if an architecture
+provides the related macros as described below.
+
CONFIG_SCHED_BOOK and CONFIG_SCHED_DRAWER are currently only used on s390,
where they reflect the cpu and cache hierarchy.
diff --git a/drivers/base/topology.c b/drivers/base/topology.c
index 8f2b641d0b8c..f079a55793ec 100644
--- a/drivers/base/topology.c
+++ b/drivers/base/topology.c
@@ -45,8 +45,10 @@ static ssize_t name##_list_read(struct file *file, struct kobject *kobj, \
define_id_show_func(physical_package_id);
static DEVICE_ATTR_RO(physical_package_id);
+#ifdef TOPOLOGY_DIE_SYSFS
define_id_show_func(die_id);
static DEVICE_ATTR_RO(die_id);
+#endif
define_id_show_func(cluster_id);
static DEVICE_ATTR_RO(cluster_id);
@@ -70,9 +72,11 @@ define_siblings_read_func(cluster_cpus, cluster_cpumask);
static BIN_ATTR_RO(cluster_cpus, 0);
static BIN_ATTR_RO(cluster_cpus_list, 0);
+#ifdef TOPOLOGY_DIE_SYSFS
define_siblings_read_func(die_cpus, die_cpumask);
static BIN_ATTR_RO(die_cpus, 0);
static BIN_ATTR_RO(die_cpus_list, 0);
+#endif
define_siblings_read_func(package_cpus, core_cpumask);
static BIN_ATTR_RO(package_cpus, 0);
@@ -103,8 +107,10 @@ static struct bin_attribute *bin_attrs[] = {
&bin_attr_core_siblings_list,
&bin_attr_cluster_cpus,
&bin_attr_cluster_cpus_list,
+#ifdef TOPOLOGY_DIE_SYSFS
&bin_attr_die_cpus,
&bin_attr_die_cpus_list,
+#endif
&bin_attr_package_cpus,
&bin_attr_package_cpus_list,
#ifdef CONFIG_SCHED_BOOK
@@ -120,7 +126,9 @@ static struct bin_attribute *bin_attrs[] = {
static struct attribute *default_attrs[] = {
&dev_attr_physical_package_id.attr,
+#ifdef TOPOLOGY_DIE_SYSFS
&dev_attr_die_id.attr,
+#endif
&dev_attr_cluster_id.attr,
&dev_attr_core_id.attr,
#ifdef CONFIG_SCHED_BOOK
diff --git a/include/linux/topology.h b/include/linux/topology.h
index 0b3704ad13c8..8d1bdae76230 100644
--- a/include/linux/topology.h
+++ b/include/linux/topology.h
@@ -180,6 +180,10 @@ static inline int cpu_to_mem(int cpu)
#endif /* [!]CONFIG_HAVE_MEMORYLESS_NODES */
+#if defined(topology_die_id) && defined(topology_die_cpumask)
+#define TOPOLOGY_DIE_SYSFS
+#endif
+
#ifndef topology_physical_package_id
#define topology_physical_package_id(cpu) ((void)(cpu), -1)
#endif
--
2.32.0
On Mon, Nov 29, 2021 at 02:03:06PM +0100, Heiko Carstens wrote:
> v2:
> As requested by Greg KH: avoid negative logic and use uppercase defines.
>
> v1:
> Create die and cluster cpu topology sysfs attributes only if an
> architecture makes uses of it, instead of creating them always for all
> architectures with bogus default values.
> Also change the book and drawer cpu topology macros so they match all
> all other topology macros.
>
> v1: https://lore.kernel.org/lkml/[email protected]/
>
> Heiko Carstens (3):
> topology/sysfs: export die attributes only if an architectures has support
> topology/sysfs: export cluster attributes only if an architectures has support
> topology/sysfs: rework book and drawer topology ifdefery
>
> Documentation/admin-guide/cputopology.rst | 33 +++++++++++------------
> drivers/base/topology.c | 28 ++++++++++++++-----
> include/linux/topology.h | 25 +++++++++++++++++
> 3 files changed, 62 insertions(+), 24 deletions(-)
Seems entirely reasonable to me,
Acked-by: Peter Zijlstra (Intel) <[email protected]>
On Mon, Nov 29, 2021 at 03:03:18PM +0100, Peter Zijlstra wrote:
> On Mon, Nov 29, 2021 at 02:03:06PM +0100, Heiko Carstens wrote:
> > v2:
> > As requested by Greg KH: avoid negative logic and use uppercase defines.
> >
> > v1:
> > Create die and cluster cpu topology sysfs attributes only if an
> > architecture makes uses of it, instead of creating them always for all
> > architectures with bogus default values.
> > Also change the book and drawer cpu topology macros so they match all
> > all other topology macros.
> >
> > v1: https://lore.kernel.org/lkml/[email protected]/
> >
> > Heiko Carstens (3):
> > topology/sysfs: export die attributes only if an architectures has support
> > topology/sysfs: export cluster attributes only if an architectures has support
> > topology/sysfs: rework book and drawer topology ifdefery
> >
> > Documentation/admin-guide/cputopology.rst | 33 +++++++++++------------
> > drivers/base/topology.c | 28 ++++++++++++++-----
> > include/linux/topology.h | 25 +++++++++++++++++
> > 3 files changed, 62 insertions(+), 24 deletions(-)
>
> Seems entirely reasonable to me,
>
> Acked-by: Peter Zijlstra (Intel) <[email protected]>
Since there seem to be no objections, who should pick this up?
I'd assume the s390 tree would not be appropriate.
Andrew, Greg, Peter?
On Thu, Dec 02, 2021 at 03:22:05PM +0100, Heiko Carstens wrote:
> On Mon, Nov 29, 2021 at 03:03:18PM +0100, Peter Zijlstra wrote:
> > On Mon, Nov 29, 2021 at 02:03:06PM +0100, Heiko Carstens wrote:
> > > v2:
> > > As requested by Greg KH: avoid negative logic and use uppercase defines.
> > >
> > > v1:
> > > Create die and cluster cpu topology sysfs attributes only if an
> > > architecture makes uses of it, instead of creating them always for all
> > > architectures with bogus default values.
> > > Also change the book and drawer cpu topology macros so they match all
> > > all other topology macros.
> > >
> > > v1: https://lore.kernel.org/lkml/[email protected]/
> > >
> > > Heiko Carstens (3):
> > > topology/sysfs: export die attributes only if an architectures has support
> > > topology/sysfs: export cluster attributes only if an architectures has support
> > > topology/sysfs: rework book and drawer topology ifdefery
> > >
> > > Documentation/admin-guide/cputopology.rst | 33 +++++++++++------------
> > > drivers/base/topology.c | 28 ++++++++++++++-----
> > > include/linux/topology.h | 25 +++++++++++++++++
> > > 3 files changed, 62 insertions(+), 24 deletions(-)
> >
> > Seems entirely reasonable to me,
> >
> > Acked-by: Peter Zijlstra (Intel) <[email protected]>
>
> Since there seem to be no objections, who should pick this up?
> I'd assume the s390 tree would not be appropriate.
>
> Andrew, Greg, Peter?
I will take it, thanks.
greg k-h
> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:[email protected]]
> Sent: Friday, December 3, 2021 3:27 AM
> To: Heiko Carstens <[email protected]>
> Cc: Peter Zijlstra <[email protected]>; Rafael J . Wysocki
> <[email protected]>; Andrew Morton <[email protected]>;
> [email protected]; Jonathan Cameron <[email protected]>;
> Len Brown <[email protected]>; Thomas Richter <[email protected]>; Ian
> Rogers <[email protected]>
> Subject: Re: [PATCH v2 0/3] topology/sysfs: only export used sysfs attributes
>
> On Thu, Dec 02, 2021 at 03:22:05PM +0100, Heiko Carstens wrote:
> > On Mon, Nov 29, 2021 at 03:03:18PM +0100, Peter Zijlstra wrote:
> > > On Mon, Nov 29, 2021 at 02:03:06PM +0100, Heiko Carstens wrote:
> > > > v2:
> > > > As requested by Greg KH: avoid negative logic and use uppercase defines.
> > > >
> > > > v1:
> > > > Create die and cluster cpu topology sysfs attributes only if an
> > > > architecture makes uses of it, instead of creating them always for all
> > > > architectures with bogus default values.
> > > > Also change the book and drawer cpu topology macros so they match all
> > > > all other topology macros.
> > > >
> > > > v1:
> https://lore.kernel.org/lkml/[email protected]/
> > > >
> > > > Heiko Carstens (3):
> > > > topology/sysfs: export die attributes only if an architectures has
> support
> > > > topology/sysfs: export cluster attributes only if an architectures has
> support
> > > > topology/sysfs: rework book and drawer topology ifdefery
> > > >
> > > > Documentation/admin-guide/cputopology.rst | 33 +++++++++++------------
> > > > drivers/base/topology.c | 28 ++++++++++++++-----
> > > > include/linux/topology.h | 25 +++++++++++++++++
> > > > 3 files changed, 62 insertions(+), 24 deletions(-)
> > >
> > > Seems entirely reasonable to me,
> > >
> > > Acked-by: Peter Zijlstra (Intel) <[email protected]>
> >
> > Since there seem to be no objections, who should pick this up?
> > I'd assume the s390 tree would not be appropriate.
> >
> > Andrew, Greg, Peter?
>
> I will take it, thanks.
Could you give me one minute?
+Brice
I'd like to hear some feedbacks from hwloc if there
is a chance to break userspace and if userspace depends
on the existence of sysfs even though the topology
doesn't exist.
If no, I feel it is safe to take.
>
> greg k-h
Thanks
Barry
Le 04/12/2021 à 10:07, Song Bao Hua (Barry Song) a écrit :
>
> Could you give me one minute?
>
> +Brice
>
> I'd like to hear some feedbacks from hwloc if there
> is a chance to break userspace and if userspace depends
> on the existence of sysfs even though the topology
> doesn't exist.
>
> If no, I feel it is safe to take.
>
Hello
If the question is whether hwloc *requires* cluster/die/book/drawer
sysfs files to exist, then the answer is no. We have to support old
kernels without those files anyway.
Brice
> -----Original Message-----
> From: Brice Goglin [mailto:[email protected]]
> Sent: Saturday, December 4, 2021 10:48 PM
> To: Song Bao Hua (Barry Song) <[email protected]>; Greg Kroah-Hartman
> <[email protected]>; Heiko Carstens <[email protected]>
> Cc: Peter Zijlstra <[email protected]>; Rafael J . Wysocki
> <[email protected]>; Andrew Morton <[email protected]>;
> [email protected]; Jonathan Cameron <[email protected]>;
> Len Brown <[email protected]>; Thomas Richter <[email protected]>; Ian
> Rogers <[email protected]>
> Subject: Re: [PATCH v2 0/3] topology/sysfs: only export used sysfs attributes
>
> Le 04/12/2021 à 10:07, Song Bao Hua (Barry Song) a écrit :
> >
> > Could you give me one minute?
> >
> > +Brice
> >
> > I'd like to hear some feedbacks from hwloc if there
> > is a chance to break userspace and if userspace depends
> > on the existence of sysfs even though the topology
> > doesn't exist.
> >
> > If no, I feel it is safe to take.
> >
>
> Hello
>
> If the question is whether hwloc *requires* cluster/die/book/drawer
> sysfs files to exist, then the answer is no. We have to support old
> kernels without those files anyway.
Thanks for clarification, Brice. I saw hwloc is reading sysfs
drawer, book and cluster. But since it doesn't assume their
existence in sysfs, I have more confidence on this patchset
now.
Greg, please ignore my comment then :-)
>
> Brice
>
>
Thanks
Barry