Raise maximum number of memory controllers

PATCH Applied Justin Ernst 7 participants 28 messages Sep 25, 2018 – Nov 13, 2018 EDAC-core

lore

Share & tools

lore

Justin Ernst

author

2018-09-25 14:34 UTC #0 lore raw

PATCH 1/1 Raise maximum number of memory controllers

We observe an oops in the skx_edac module during boot. Examining /var/log/messages:

[ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
[ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
...
[ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
[ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
[ 3401.987579] Too many memory controllers: 16
[ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0

We observe there are two memory controllers per socket, with a limit of 16. Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).

Cc Borislav Petkov <[email protected]>

Cc Mauro Carvalho Chehab <[email protected]>

Cc [email protected]

Acked-by Russ Anderson <[email protected]>

Signed-off-by Justin Ernst <[email protected]>

---
 include/linux/edac.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

include/linux/edac.h +2 -1

index bffb97828ed6..958d69332c1d 100644
@@ -17,6 +17,7 @@
 #include <linux/completion.h>
 #include <linux/workqueue.h>
 #include <linux/debugfs.h>
+#include <linux/numa.h>
 
 #define EDAC_DEVICE_NAME_LEN	31

@@ -670,6 +671,6 @@ struct mem_ctl_info {
 /*
  * Maximum number of memory controllers in the coherent fabric.
  */
-#define EDAC_MAX_MCS	16
+#define EDAC_MAX_MCS	2 * MAX_NUMNODES
 
 #endif

Borislav Petkov

maintainer

2018-09-25 15:26 UTC #1 lore raw

Quoted 12 lines · Justin Ernst

We observe an oops in the skx_edac module during boot. Examining /var/log/messages:

[ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
[ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
...
[ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
[ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
[ 3401.987579] Too many memory controllers: 16
[ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0

Quoted 2 lines

We observe there are two memory controllers per socket, with a limit of 16. Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).

Tony,

can we read that out from the hardware instead of having this silly static number?

Leaving in the rest.

Quoted 33 lines

Cc Borislav Petkov <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
Cc [email protected]
Cc [email protected]
Acked-by Russ Anderson <[email protected]>
Signed-off-by Justin Ernst <[email protected]>
---
 include/linux/edac.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
include/linux/edac.h +2 -1
index bffb97828ed6..958d69332c1d 100644
@@ -17,6 +17,7 @@
 #include <linux/completion.h>
 #include <linux/workqueue.h>
 #include <linux/debugfs.h>
+#include <linux/numa.h>
 
 #define EDAC_DEVICE_NAME_LEN	31
 
@@ -670,6 +671,6 @@ struct mem_ctl_info {
 /*
  * Maximum number of memory controllers in the coherent fabric.
  */
-#define EDAC_MAX_MCS	16
+#define EDAC_MAX_MCS	2 * MAX_NUMNODES
 
 #endif
Signature · 1 line
2.12.3

Luck, Tony

maintainer

2018-09-25 17:50 UTC #2 lore raw

Quoted 16 lines · Borislav Petkov

Quoted 12 lines · Justin Ernst

We observe an oops in the skx_edac module during boot. Examining /var/log/messages:

[ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
[ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
...
[ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
[ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
[ 3401.987579] Too many memory controllers: 16
[ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0

Quoted 2 lines

We observe there are two memory controllers per socket, with a limit of 16. Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).

Quoted 6 lines

Tony,
can we read that out from the hardware instead of having this silly static number?
Leaving in the rest.

There are way too many places where we use the identifier "bus" in the edac core and drivers. But I'm not sure that we need a static array mc_bus[EDAC_MAX_MCS].

Why can't we:

- mci->bus = &mc_bus[mci->mc_idx];
+ mci->bus = kmalloc(sizeof *(mci->bus), GFP_KERNEL);

and then figure out where to kfree(mci->bus) on driver removal?

Do we every do arithmetic on different mci->bus pointers that assume they are all part of a single array?

-Tony

Borislav Petkov

maintainer

2018-09-25 18:07 UTC #3 lore raw

Quoted 3 lines · Tony

There are way too many places where we use the identifier "bus" in the edac core and drivers. But I'm not sure that we need a static array mc_bus[EDAC_MAX_MCS].

That, of course, is another way of looking at it which I didn't think of.

Quoted 7 lines

Why can't we:
(quoted diff) +1 -1
-	mci->bus = &mc_bus[mci->mc_idx];
+	mci->bus = kmalloc(sizeof *(mci->bus), GFP_KERNEL);
and then figure out where to kfree(mci->bus) on driver removal?

AFAICT, in _edac_mc_free(). We free there mci itself so kfree(mci->bus) can happen directly before it.

Quoted 2 lines

Do we every do arithmetic on different mci->bus pointers that assume they are all part of a single array?

AFAICT, we use that thing for the bus_reg/unreg functions and we hand it back'n'forth in edac_mc_sysfs.c, see

$ git grep -E "mci.*bus" drivers/edac/
drivers/edac/edac_mc.c:763:     mci->bus = &mc_bus[mci->mc_idx];
drivers/edac/edac_mc_sysfs.c:408:       csrow->dev.bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:639:       dimm->dev.bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:928:       mci->bus->name = name;
drivers/edac/edac_mc_sysfs.c:930:       edac_dbg(0, "creating bus %s\n", mci->bus->name);
drivers/edac/edac_mc_sysfs.c:932:       err = bus_register(mci->bus);
drivers/edac/edac_mc_sysfs.c:943:       mci->dev.bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:1002:      bus_unregister(mci->bus);
drivers/edac/edac_mc_sysfs.c:1035:      struct bus_type *bus = mci->bus;
drivers/edac/edac_mc_sysfs.c:1036:      const char *name = mci->bus->name;
drivers/edac/edac_mc_sysfs.c:1071:      mci_pdev->bus = edac_get_sysfs_subsys();
drivers/edac/i5100_edac.c:967:  priv->debugfs = edac_debugfs_create_dir_at(mci->bus->name, i5100_debugfs);
drivers/edac/i7core_edac.c:1170:        pvt->addrmatch_dev->bus = mci->dev.bus;
drivers/edac/i7core_edac.c:1191:                pvt->chancounts_dev->bus = mci->dev.bus;

HOWEVER, look at

88d84ac97378 ("EDAC: Fix lockdep splat")

Now I remember. I did that for lockdep because it wants statically allocated memory. I'll try to think of something tomorrow.

Thx.

Borislav Petkov

maintainer

2018-09-26 09:35 UTC #4 lore raw

Quoted 2 lines · Borislav Petkov

Now I remember. I did that for lockdep because it wants statically allocated memory. I'll try to think of something tomorrow.

Some more info after some staring:

We could've made the lock_class_key only static storage so that lockdep is happy but then struct bus_type embeds the *whole* lock_class_key and not a pointer to it.

Which leaves us with either:

* this fix postponing the problem

* or Greg coming and saying, you're using bus_type all wrong and you shouldn't and you should remove it completely! :-)

Which would be much better.

Thx.

Borislav Petkov

maintainer

2018-09-26 15:27 UTC #5 lore raw

Quoted 2 lines · Borislav Petkov

* or Greg coming and saying, you're using bus_type all wrong and you shouldn't and you should remove it completely! :-)

Yap, and so he did! :-)

It looks like we can remove the whole per-MC bus thing, see below. Patch seems to work here and grepping sysfs hierarchy before and after:

find /sys/ | grep -i edac > ...

doesn't show any differences.

Tony, I'd appreciate running this on one of your big boxes to check nothing is missing in sysfs...

Thx.

---
 drivers/edac/edac_mc.c       |  9 +--------
 drivers/edac/edac_mc_sysfs.c | 30 ++----------------------------
 include/linux/edac.h         |  6 ------
 3 files changed, 3 insertions(+), 42 deletions(-)

drivers/edac/edac_mc.c +1 -8

index 7d3edd713932..13594ffadcb3 100644
@@ -55,8 +55,6 @@ static LIST_HEAD(mc_devices);
  */
 static const char *edac_mc_owner;
 
-static struct bus_type mc_bus[EDAC_MAX_MCS];
-
 int edac_get_report_status(void)
 {
 	return edac_report;

@@ -716,11 +714,6 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
 	int ret = -EINVAL;
 	edac_dbg(0, "\n");
 
-	if (mci->mc_idx >= EDAC_MAX_MCS) {
-		pr_warn_once("Too many memory controllers: %d\n", mci->mc_idx);
-		return -ENODEV;
-	}
-
 #ifdef CONFIG_EDAC_DEBUG
 	if (edac_debug_level >= 3)
 		edac_mc_dump_mci(mci);

@@ -760,7 +753,7 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
 	/* set load time so that error rate can be tracked */
 	mci->start_time = jiffies;
 
-	mci->bus = &mc_bus[mci->mc_idx];
+	mci->bus = edac_get_sysfs_subsys();
 
 	if (edac_create_sysfs_mci_device(mci, groups)) {
 		edac_mc_printk(mci, KERN_WARNING,

drivers/edac/edac_mc_sysfs.c +2 ~~-28~~

index 20374b8248f0..2ca2012f2857 100644
@@ -914,27 +914,8 @@ static const struct device_type mci_attr_type = {
 int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 				 const struct attribute_group **groups)
 {
-	char *name;
 	int i, err;
 
-	/*
-	 * The memory controller needs its own bus, in order to avoid
-	 * namespace conflicts at /sys/bus/edac.
-	 */
-	name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
-	if (!name)
-		return -ENOMEM;
-
-	mci->bus->name = name;
-
-	edac_dbg(0, "creating bus %s\n", mci->bus->name);
-
-	err = bus_register(mci->bus);
-	if (err < 0) {
-		kfree(name);
-		return err;
-	}
-
 	/* get the /sys/devices/system/edac subsys reference */
 	mci->dev.type = &mci_attr_type;
 	device_initialize(&mci->dev);

@@ -950,7 +931,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 	err = device_add(&mci->dev);
 	if (err < 0) {
 		edac_dbg(1, "failure: create device %s\n", dev_name(&mci->dev));
-		goto fail_unregister_bus;
+		goto out;
 	}
 
 	/*

@@ -998,10 +979,8 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 		device_unregister(&dimm->dev);
 	}
 	device_unregister(&mci->dev);
-fail_unregister_bus:
-	bus_unregister(mci->bus);
-	kfree(name);
 
+out:
 	return err;
 }

@@ -1032,13 +1011,8 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
 
 void edac_unregister_sysfs(struct mem_ctl_info *mci)
 {
-	struct bus_type *bus = mci->bus;
-	const char *name = mci->bus->name;
-
 	edac_dbg(1, "Unregistering device %s\n", dev_name(&mci->dev));
 	device_unregister(&mci->dev);
-	bus_unregister(bus);
-	kfree(name);
 }
 
 static void mc_attr_release(struct device *dev)

include/linux/edac.h +0 -6

index a45ce1f84bfc..dd8d006f1837 100644
@@ -668,10 +668,4 @@ struct mem_ctl_info {
 	bool fake_inject_ue;
 	u16 fake_inject_count;
 };
-
-/*
- * Maximum number of memory controllers in the coherent fabric.
- */
-#define EDAC_MAX_MCS	16
-
 #endif

Mauro Carvalho Chehab

2018-09-26 16:03 UTC #6 lore raw

Em Wed, 26 Sep 2018 17:27:52 +0200
Borislav Petkov <[email protected]> escreveu:

Quoted 7 lines · Borislav Petkov

Quoted 2 lines
* or Greg coming and saying, you're using bus_type all wrong and you shouldn't and you should remove it completely! :-)
Yap, and so he did! :-)
It looks like we can remove the whole per-MC bus thing, see below.

I guess this is/was needed to create things like this:

	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc

(I don't test this logic for a while).

Quoted 9 lines

Patch
seems to work here and grepping sysfs hierarchy before and after:
find /sys/ | grep -i edac > ...
doesn't show any differences.
Tony, I'd appreciate running this on one of your big boxes to check nothing is missing in sysfs...

There are a few EDAC drivers for old server chipsets that create error report mechanisms for PCI bus controllers. I don't remember the specifics anymore. I *suspect* those are the drivers that do such usage:

	$ git grep -l edac_pci_add_device
	drivers/edac/amd8111_edac.c
	drivers/edac/amd8131_edac.c
	drivers/edac/edac_pci.c
	drivers/edac/edac_pci.h
	drivers/edac/mpc85xx_edac.c
	drivers/edac/mv64x60_edac.c
	drivers/edac/octeon_edac-pci.c

None of them use Intel chipsets.

Last time I tried to test it (more than 5 years ago), it was hard to find such systems at the labs I had access on that time.

Anyway, as this is a more unusual usage of EDAC API, it could be worth to double-check if this patch won't break it, maybe doing some test on such machines before/after this patch in order to verify that this won't cause regressions there.

In thesis, it shouldn't affect, as the changes are happening at edac_mc*.c, but I won't doubt that it might have a hidden dependency somewhere.

Quoted 115 lines

Thx.

---
 drivers/edac/edac_mc.c       |  9 +--------
 drivers/edac/edac_mc_sysfs.c | 30 ++----------------------------
 include/linux/edac.h         |  6 ------
 3 files changed, 3 insertions(+), 42 deletions(-)

drivers/edac/edac_mc.c +1 -8

index 7d3edd713932..13594ffadcb3 100644
@@ -55,8 +55,6 @@ static LIST_HEAD(mc_devices);
  */
 static const char *edac_mc_owner;
 
-static struct bus_type mc_bus[EDAC_MAX_MCS];
-
 int edac_get_report_status(void)
 {
 	return edac_report;

@@ -716,11 +714,6 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
 	int ret = -EINVAL;
 	edac_dbg(0, "\n");
 
-	if (mci->mc_idx >= EDAC_MAX_MCS) {
-		pr_warn_once("Too many memory controllers: %d\n", mci->mc_idx);
-		return -ENODEV;
-	}
-
 #ifdef CONFIG_EDAC_DEBUG
 	if (edac_debug_level >= 3)
 		edac_mc_dump_mci(mci);

@@ -760,7 +753,7 @@ int edac_mc_add_mc_with_groups(struct mem_ctl_info *mci,
 	/* set load time so that error rate can be tracked */
 	mci->start_time = jiffies;
 
-	mci->bus = &mc_bus[mci->mc_idx];
+	mci->bus = edac_get_sysfs_subsys();
 
 	if (edac_create_sysfs_mci_device(mci, groups)) {
 		edac_mc_printk(mci, KERN_WARNING,

drivers/edac/edac_mc_sysfs.c +2 ~~-28~~

index 20374b8248f0..2ca2012f2857 100644
@@ -914,27 +914,8 @@ static const struct device_type mci_attr_type = {
 int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 				 const struct attribute_group **groups)
 {
-	char *name;
 	int i, err;
 
-	/*
-	 * The memory controller needs its own bus, in order to avoid
-	 * namespace conflicts at /sys/bus/edac.
-	 */
-	name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
-	if (!name)
-		return -ENOMEM;
-
-	mci->bus->name = name;
-
-	edac_dbg(0, "creating bus %s\n", mci->bus->name);
-
-	err = bus_register(mci->bus);
-	if (err < 0) {
-		kfree(name);
-		return err;
-	}
-
 	/* get the /sys/devices/system/edac subsys reference */
 	mci->dev.type = &mci_attr_type;
 	device_initialize(&mci->dev);

@@ -950,7 +931,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 	err = device_add(&mci->dev);
 	if (err < 0) {
 		edac_dbg(1, "failure: create device %s\n", dev_name(&mci->dev));
-		goto fail_unregister_bus;
+		goto out;
 	}
 
 	/*

@@ -998,10 +979,8 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 		device_unregister(&dimm->dev);
 	}
 	device_unregister(&mci->dev);
-fail_unregister_bus:
-	bus_unregister(mci->bus);
-	kfree(name);
 
+out:
 	return err;
 }

@@ -1032,13 +1011,8 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
 
 void edac_unregister_sysfs(struct mem_ctl_info *mci)
 {
-	struct bus_type *bus = mci->bus;
-	const char *name = mci->bus->name;
-
 	edac_dbg(1, "Unregistering device %s\n", dev_name(&mci->dev));
 	device_unregister(&mci->dev);
-	bus_unregister(bus);
-	kfree(name);
 }
 
 static void mc_attr_release(struct device *dev)

include/linux/edac.h +0 -0

index a45ce1f84bfc..dd8d006f1837 100644

include/linux/edac.h struct mem_ctl_info { · line 671

Quoted 10 lines

include/linux/edac.h +0 -6

@@ -668,10 +668,4 @@ struct mem_ctl_info {
 	bool fake_inject_ue;
 	u16 fake_inject_count;
 };
-
-/*
- * Maximum number of memory controllers in the coherent fabric.
- */
-#define EDAC_MAX_MCS	16
-
 #endif

Thanks,
Mauro

Borislav Petkov

maintainer

2018-09-26 16:17 UTC #7 lore raw

Quoted 3 lines · Mauro Carvalho Chehab

I guess this is/was needed to create things like this:
	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc

They're still there:

$ ls -l /sys/bus/edac/devices/
total 0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0

Mauro Carvalho Chehab

2018-09-26 17:39 UTC #8 lore raw

Em Wed, 26 Sep 2018 18:17:49 +0200
Borislav Petkov <[email protected]> escreveu:

Quoted 7 lines · Mauro Carvalho Chehab

Quoted 3 lines
I guess this is/was needed to create things like this:
	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc  
They're still there:

Quoted 10 lines

$ ls -l /sys/bus/edac/devices/
total 0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0

Ok. Yeah, probably some changes at the driver's core made that bus thing obsolete. Or maybe it is there to allow unbind/remove the edac driver. Had you test it with KASAN and tried to unbind/remove the EDAC driver?

Thanks,
Mauro

Luck, Tony

maintainer

2018-09-26 18:10 UTC #9 lore raw

In reply to Borislav Petkov

Quoted 7 lines · Borislav Petkov

Quoted 3 lines · Mauro Carvalho Chehab
I guess this is/was needed to create things like this:
	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc
They're still there:

Quoted 9 lines

$ ls -l /sys/bus/edac/devices/
total 0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0

I ran into trouble on my 4 socket broadwell server (so 8 memory controllers, a whole pile of DIMMs, running from sb_edac.c)

Things start going wrong with:

[   45.216657] sysfs: cannot create duplicate filename '/bus/edac/devices/dimm0'
[   45.216663] CPU: 37 PID: 2034 Comm: systemd-udevd Not tainted 4.19.0-rc5 #1
[   45.216665] Hardware name: Intel Corporation BRICKLAND/BRICKLAND, BIOS BRBDXSD1.86B.0338.V01.1603162127 03/16/2016
[   45.216667] Call Trace:
[   45.216688]  dump_stack+0x5c/0x7b
[   45.216697]  sysfs_warn_dup+0x56/0x70
[   45.216702]  sysfs_do_create_link_sd.isra.2+0x98/0xb0
[   45.216714]  bus_add_device+0x77/0x160
[   45.216720]  device_add+0x424/0x660
[   45.216731]  edac_create_sysfs_mci_device+0xb9/0x2f0
[   45.216738]  edac_mc_add_mc_with_groups+0x111/0x2b0
[   45.216747]  sbridge_init+0x13c9/0x2000 [sb_edac]
[   45.216757]  ? _raw_spin_lock+0x1d/0x20
[   45.216765]  ? free_pcppages_bulk+0x2ca/0x630
[   45.216769]  ? 0xffffffffc050f000
[   45.216779]  do_one_initcall+0x46/0x1c8
[   45.216784]  ? free_unref_page_commit+0x95/0x120
[   45.216791]  ? _cond_resched+0x15/0x40
[   45.216798]  ? kmem_cache_alloc_trace+0x153/0x1c0
[   45.216805]  do_init_module+0x5b/0x208
[   45.216826]  load_module+0x1a2d/0x1fb0
[   45.216835]  ? __do_sys_finit_module+0xe9/0x110
[   45.216840]  __do_sys_finit_module+0xe9/0x110
[   45.216847]  do_syscall_64+0x5b/0x180
[   45.216852]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   45.216856] RIP: 0033:0x7fcdec618bd9

and fell off a cliff after that.

Going back to the old code I have a "dimm0" on each of the eight controllers:

# find /sys -name dimm0
/sys/devices/system/edac/mc/mc6/dimm0
/sys/devices/system/edac/mc/mc4/dimm0
/sys/devices/system/edac/mc/mc2/dimm0
/sys/devices/system/edac/mc/mc0/dimm0
/sys/devices/system/edac/mc/mc7/dimm0
/sys/devices/system/edac/mc/mc5/dimm0
/sys/devices/system/edac/mc/mc3/dimm0
/sys/devices/system/edac/mc/mc1/dimm0
/sys/bus/mc6/devices/dimm0
/sys/bus/mc4/devices/dimm0
/sys/bus/mc2/devices/dimm0
/sys/bus/mc0/devices/dimm0
/sys/bus/mc7/devices/dimm0
/sys/bus/mc5/devices/dimm0
/sys/bus/mc3/devices/dimm0
/sys/bus/mc1/devices/dimm0
# ls -l /sys/bus/mc0/devices
total 0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 mc0 -> ../../../devices/system/edac/mc/mc0

It looks like the new code isn't trying to place the dimm symlinks in the proper subdirectories.

-Tony

Russ Anderson

2018-09-26 18:23 UTC #10 lore raw

Quoted 18 lines · Tony

Quoted 7 lines · Borislav Petkov

Quoted 3 lines · Mauro Carvalho Chehab
I guess this is/was needed to create things like this:
	lrwxrwxrwx 1 root root 0 set 26 05:24 /sys/bus/edac/devices/mc -> ../../../devices/system/edac/mc
They're still there:

Quoted 9 lines

$ ls -l /sys/bus/edac/devices/
total 0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
lrwxrwxrwx 1 root root 0 Sep 26 18:15 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc -> ../../../devices/system/edac/mc
lrwxrwxrwx 1 root root 0 Sep 26 18:15 mc0 -> ../../../devices/system/edac/mc/mc0

Quoted 2 lines

I ran into trouble on my 4 socket broadwell server (so 8 memory controllers, a whole pile of DIMMs, running from sb_edac.c)

We are also having trouble on a 32 socket system.

---------------------------------------------------------------------------------------------
[ OK ] Started Load kdump kernel early on startup.
[ ***] (2 of 2) A start job is running for...work interfaces (18s / no limit)[ 132.638611] BUG: unable to handle kernel paging request at ffff8c7efeebefff

[  132.640895] PGD 5fec3fdd067 P4D 5fec3fdd067 PUD 5fec3fda067 PMD 0 
[  132.640895] Oops: 0002 [#1] SMP PTI
[  132.640895] CPU: 650 PID: 9884 Comm: kworker/650:1 Kdump: loaded Tainted: G            E     4.19.0-rc4-ernstj+ #6
[  132.640895] Hardware name: HPE Superdome Flex/Superdome Flex, BIOS Bundle:3.0.196 SFW:IP147.007.000.071.000.1809242200 09/24/2018
[  132.640895] Workqueue: events cache_reap
[  132.640895] RIP: 0010:free_block+0x11c/0x1e0
[  132.640895] Code: ea 20 45 29 d7 41 d3 ef 0f b6 4f 1d 45 01 fa 41 d3 ea 8b 48 30 44 8d 79 ff 48 8b 48 20 44 89 78 30 48 85 c9 0f 84 a5 00 00 00 <46> 88 14 39 8b 48 30 85 c9 0f 84 32 ff ff ff 49 8b 49 10 4c 8d 50
[  132.640895] RSP: 0018:ffffc9004b0c7d90 EFLAGS: 00010086
[  132.640895] RAX: ffffea11f7fbafc0 RBX: 0000000000000002 RCX: ffff8c7dfeebf000
[  132.640895] RDX: 0000000000000005 RSI: ffff8c7e08da9328 RDI: ffff880147c02000
[  132.640895] RBP: 0000000080000000 R08: ffff8c5047c004a8 R09: ffff8c5047c00480
[  132.640895] R10: 0000000000000001 R11: ffff8c7dfeebf800 R12: ffffea0000000000
[  132.640895] R13: 000077ff80000000 R14: ffff8c5047c00488 R15: 00000000ffffffff
[  132.640895] FS:  0000000000000000(0000) GS:ffff8c7e08d80000(0000) knlGS:0000000000000000
[  132.640895] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  132.640895] CR2: ffff8c7efeebefff CR3: 000000000200a006 CR4: 00000000007606e0
[  132.640895] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  132.640895] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  132.640895] PKRU: 55555554
[  132.640895] Call Trace:
[  132.640895]  drain_array_locked+0x5b/0x80
[  132.640895]  drain_array+0x63/0x90
[  132.640895]  cache_reap+0x68/0x1f0
[  132.640895]  process_one_work+0x165/0x360
[  132.640895]  worker_thread+0x49/0x3e0
[  132.640895]  kthread+0xf8/0x130
[  132.640895]  ? max_active_store+0x60/0x60
[  132.640895]  ? kthread_bind+0x10/0x10
[  132.640895]  ret_from_fork+0x35/0x40
[  132.640895] Modules linked in: acpi_cpufreq(E-) skx_edac(E+) intel_rapl(E) nfit(E) libnvdimm(E) x86_pkg_temp_thermal(E) intel_powerclamp(E) coretemp(E) kvm_intel(E) kvm(E) irqbypass(E) crct10dif_pclmul(E) crc32_pclmul(E) ghash_clmulni_intel(E) pcbc(E) aesni_intel(E) aes_x86_64(E) iscsi_ibft(E) crypto_simd(E) iscsi_boot_sysfs(E) cryptd(E) glue_helper(E) pcspkr(E) nls_iso8859_1(E) nls_cp437(E) vfat(E) fat(E) joydev(E) i40e(E) ipmi_ssif(E) lpc_ich(E) i2c_i801(E) mfd_core(E) wmi(E) ipmi_si(E) ipmi_devintf(E) ipmi_msghandler(E) button(E) xfs(E) libcrc32c(E) hid_generic(E) usbhid(E) sd_mod(E) mgag200(E) i2c_algo_bit(E) drm_kms_helper(E) syscopyarea(E) crc32c_intel(E) sysfillrect(E) xhci_pci(E) sysimgblt(E) fb_sys_fops(E) xhci_hcd(E) ahci(E) libahci(E) ttm(E) drm(E) libata(E) usbcore(E) sg(E) dm_multipath(E)
[  132.916934]  dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E) msr(E) efivarfs(E) autofs4(E)
[  132.916934] CR2: ffff8c7efeebefff
[  132.916934] ---[ end trace 9ee1381bf4bae01f ]---
[  *** ] (2 of 2) A start job is running for.[  132.916934] RIP: 0010:free_block+0x11c/0x1e0
[  132.916934] Code: ea 20 45 29 d7 41 d3 ef 0f b6 4f 1d 45 01 fa 41 d3 ea 8b 48 30 44 8d 79 ff 48 8b 48 20 44 89 78 30 48 85 c9 0f 84 a5 00 00 00 <46> 88 14 39 8b 48 30 85 c9 0f 84 32 ff ff ff 49 8b 49 10 4c 8d 50
[  132.916934] RSP: 0018:ffffc9004b0c7d90 EFLAGS: 00010086
..work interface[  132.977236] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0: DEV 0000:80:0a.0
[  132.916934] RAX: ffffea11f7fbafc0 RBX: 0000000000000002 RCX: ffff8c7dfeebf000
[  132.916934] RDX: 0000000000000005 RSI: ffff8c7e08da9328 RDI: ffff880147c02000
s (19s / no limi[  133.004953] RBP: 0000000080000000 R08: ffff8c5047c004a8 R09: ffff8c5047c00480
[  133.004953] R10: 0000000000000001 R11: ffff8c7dfeebf800 R12: ffffea0000000000
[  133.004953] R13: 000077ff80000000 R14: ffff8c5047c00488 R15: 00000000ffffffff
[  133.004953] FS:  0000000000000000(0000) GS:ffff8c7e08d80000(0000) knlGS:0000000000000000
[  133.004953] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  133.004953] CR2: ffff8c7efeebefff CR3: 000000000200a006 CR4: 00000000007606e0
[  133.004953] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  133.004953] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  133.004953] PKRU: 55555554
[  133.004953] Kernel panic - not syncing: Fatal exception

---------------------------------------------------------------------------------------------

Quoted 63 lines

Things start going wrong with:

[   45.216657] sysfs: cannot create duplicate filename '/bus/edac/devices/dimm0'
[   45.216663] CPU: 37 PID: 2034 Comm: systemd-udevd Not tainted 4.19.0-rc5 #1
[   45.216665] Hardware name: Intel Corporation BRICKLAND/BRICKLAND, BIOS BRBDXSD1.86B.0338.V01.1603162127 03/16/2016
[   45.216667] Call Trace:
[   45.216688]  dump_stack+0x5c/0x7b
[   45.216697]  sysfs_warn_dup+0x56/0x70
[   45.216702]  sysfs_do_create_link_sd.isra.2+0x98/0xb0
[   45.216714]  bus_add_device+0x77/0x160
[   45.216720]  device_add+0x424/0x660
[   45.216731]  edac_create_sysfs_mci_device+0xb9/0x2f0
[   45.216738]  edac_mc_add_mc_with_groups+0x111/0x2b0
[   45.216747]  sbridge_init+0x13c9/0x2000 [sb_edac]
[   45.216757]  ? _raw_spin_lock+0x1d/0x20
[   45.216765]  ? free_pcppages_bulk+0x2ca/0x630
[   45.216769]  ? 0xffffffffc050f000
[   45.216779]  do_one_initcall+0x46/0x1c8
[   45.216784]  ? free_unref_page_commit+0x95/0x120
[   45.216791]  ? _cond_resched+0x15/0x40
[   45.216798]  ? kmem_cache_alloc_trace+0x153/0x1c0
[   45.216805]  do_init_module+0x5b/0x208
[   45.216826]  load_module+0x1a2d/0x1fb0
[   45.216835]  ? __do_sys_finit_module+0xe9/0x110
[   45.216840]  __do_sys_finit_module+0xe9/0x110
[   45.216847]  do_syscall_64+0x5b/0x180
[   45.216852]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   45.216856] RIP: 0033:0x7fcdec618bd9

and fell off a cliff after that.

Going back to the old code I have a "dimm0" on each of the eight controllers:

# find /sys -name dimm0
/sys/devices/system/edac/mc/mc6/dimm0
/sys/devices/system/edac/mc/mc4/dimm0
/sys/devices/system/edac/mc/mc2/dimm0
/sys/devices/system/edac/mc/mc0/dimm0
/sys/devices/system/edac/mc/mc7/dimm0
/sys/devices/system/edac/mc/mc5/dimm0
/sys/devices/system/edac/mc/mc3/dimm0
/sys/devices/system/edac/mc/mc1/dimm0
/sys/bus/mc6/devices/dimm0
/sys/bus/mc4/devices/dimm0
/sys/bus/mc2/devices/dimm0
/sys/bus/mc0/devices/dimm0
/sys/bus/mc7/devices/dimm0
/sys/bus/mc5/devices/dimm0
/sys/bus/mc3/devices/dimm0
/sys/bus/mc1/devices/dimm0
# ls -l /sys/bus/mc0/devices
total 0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 dimm9 -> ../../../devices/system/edac/mc/mc0/dimm9
lrwxrwxrwx. 1 root root 0 Sep 26 11:08 mc0 -> ../../../devices/system/edac/mc/mc0

It looks like the new code isn't trying to place the dimm symlinks in the proper subdirectories.

-Tony

Luck, Tony

maintainer

2018-09-26 23:02 UTC #11 lore raw

This issue has made me look a bit more at what EDAC puts in sysfs. It seems like the current code inherits some useless baggage from the device calls it makes.

E.g. all the "power" subdirectories:

$ find /sys/devices/system/edac -name power
/sys/devices/system/edac/power
/sys/devices/system/edac/mc/mc6/dimm3/power
/sys/devices/system/edac/mc/mc6/power
/sys/devices/system/edac/mc/mc6/csrow0/power
/sys/devices/system/edac/mc/mc6/dimm6/power
/sys/devices/system/edac/mc/mc6/dimm0/power
/sys/devices/system/edac/mc/mc6/dimm9/power
/sys/devices/system/edac/mc/mc4/dimm3/power
/sys/devices/system/edac/mc/mc4/power
... total of 50 of these ...

$ grep -r . /sys/devices/system/edac/mc/mc6/dimm0/power
/sys/devices/system/edac/mc/mc6/dimm0/power/runtime_active_time:0
/sys/devices/system/edac/mc/mc6/dimm0/power/runtime_status:unsupported
grep: /sys/devices/system/edac/mc/mc6/dimm0/power/autosuspend_delay_ms: Input/output error
/sys/devices/system/edac/mc/mc6/dimm0/power/runtime_suspended_time:0
/sys/devices/system/edac/mc/mc6/dimm0/power/control:auto

We don't have stats, nor control of power on a per memory controller or per dimm basis. So all these files are just noise.

But ... we are at -rc5. Not sure that we'll figure out, write, test & debug the proper solution in the next 3-4 weeks. So perhaps we should apply

-#define EDAC_MAX_MCS 16
+#define EDAC_MAX_MCS 64

as a temporary band-aid to get HPE's 32-socket machine running while we work on the proper fix?

-Tony

Borislav Petkov

maintainer

2018-09-27 04:52 UTC #12 lore raw

Quoted 2 lines · Tony

We don't have stats, nor control of power on a per memory controller or per dimm basis. So all these files are just noise.

Yeah, and also, looking at your previous mail, stuff like:

/sys/bus/mc6/devices/dimm0
/sys/bus/mc4/devices/dimm0

doesn't make any sense: why is mc* directly under bus? It should be under ...bus/edac/mc/...

We'll have to clean it up carefully, when there's time.

Quoted 8 lines

But ... we are at -rc5. Not sure that we'll figure out, write, test & debug the proper solution in the next 3-4 weeks. So perhaps we should apply
(quoted diff) +1 -1
-#define EDAC_MAX_MCS   16
+#define EDAC_MAX_MCS   64
as a temporary band-aid to get HPE's 32-socket machine running while we work on the proper fix?

Yeah, after sleeping on it I see it the same way - band-aid it now and clean it up properly later.

Thx.

Luck, Tony

maintainer

2018-09-27 21:44 UTC #13 lore raw

Quoted 4 lines · Borislav Petkov

Quoted 3 lines · Tony
But ... we are at -rc5. Not sure that we'll figure out, write, test & debug the proper solution in the next 3-4 weeks. So perhaps we should apply

Quoted 8 lines

Quoted 5 lines
(quoted diff) +1 -1
-#define EDAC_MAX_MCS   16
+#define EDAC_MAX_MCS   64
as a temporary band-aid to get HPE's 32-socket machine running while we work on the proper fix?
Yeah, after sleeping on it I see it the same way - band-aid it now and clean it up properly later.

The problem with your patch that gets rid of EDAC_MAX_MCS is making device links under /sys/bus/edac. Which is hinted at in some of the code your patch deleted:

- /*

-        * The memory controller needs its own bus, in order to avoid
-        * namespace conflicts at /sys/bus/edac.
-        */

- name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
- if (!name)
- return -ENOMEM;
-
- mci->bus->name = name;
-
- edac_dbg(0, "creating bus %s\n", mci->bus->name);
-
- err = bus_register(mci->bus);

Just to see if there was anything else wrong I added a patch to make the names unique:

which seemed to work. But then I began wondering what are ABI expectations from applications that read the EDAC /sys files?

Is this this current source repository? https://github.com/grondo/edac-utils

This code doesn't seem to know about the "dimm*" directories below the "mc*" level. It just looks for the csrow* entries.

-Tony

drivers/edac/edac_mc_sysfs.c +3 -3

index 2ca2012f2857..6ec6d8a2adb8 100644
@@ -410,7 +410,7 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
 	device_initialize(&csrow->dev);
 	csrow->dev.parent = &mci->dev;
 	csrow->mci = mci;
-	dev_set_name(&csrow->dev, "csrow%d", index);
+	dev_set_name(&csrow->dev, "mci%d_csrow%d", mci->mc_idx, index);
 	dev_set_drvdata(&csrow->dev, csrow);
 
 	edac_dbg(0, "creating (virtual) csrow node %s\n",

@@ -641,9 +641,9 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
 
 	dimm->dev.parent = &mci->dev;
 	if (mci->csbased)
-		dev_set_name(&dimm->dev, "rank%d", index);
+		dev_set_name(&dimm->dev, "mci%d_rank%d", mci->mc_idx, index);
 	else
-		dev_set_name(&dimm->dev, "dimm%d", index);
+		dev_set_name(&dimm->dev, "mci%d_dimm%d", mci->mc_idx, index);
 	dev_set_drvdata(&dimm->dev, dimm);
 	pm_runtime_forbid(&mci->dev);

Borislav Petkov

maintainer

2018-09-27 22:03 UTC #14 lore raw

Review

Quoted 13 lines · Tony

The problem with your patch that gets rid of EDAC_MAX_MCS is making device links under /sys/bus/edac. Which is hinted at in some of the code your patch deleted:
(quoted diff) +0 -9
-       /*
-        * The memory controller needs its own bus, in order to avoid
-        * namespace conflicts at /sys/bus/edac.
-        */
-       name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
-       if (!name)
-               return -ENOMEM;
-
-       mci->bus->name = name;

Yes, and that needed to go because I am using a single bus. Which kinda makes sense because you want to have a single bus and multiple devices on it. I mean, if we *have* to have a bus.

I think this whole /sys/bus/edac thing is crap and needs to go. We have a perfectly fine hierarchy under /sys/devices/system/edac and duplicating it under /sys/bus/edac is just bollocks. IMHO. Feel free to correct me with, but but, this is useful for...

Quoted 1 line

which seemed to work.

Right.

Quoted 7 lines

But then I began wondering what are ABI expectations from applications that read the EDAC /sys files?
Is this this current source repository? https://github.com/grondo/edac-utils
This code doesn't seem to know about the "dimm*" directories below the "mc*" level. It just looks for the csrow* entries.

I guess this is a question for Mauro. I never really needed any special edac tool to get info and if you ask me, we probably should try to keep it simple and grep sysfs. So that you can always get the info without having to install any special tools. Like ftrace works on every system with just a shell and basic tools. I think this is very powerful. But this is old spartan me only thinking out loud.

In any case, I'm more than fine with dropping the bus hierarchy if nothing uses it.

Thx.

Mauro Carvalho Chehab

2018-09-28 01:10 UTC #15 lore raw

Em Fri, 28 Sep 2018 00:03:55 +0200
Borislav Petkov <[email protected]> escreveu:

Quoted 43 lines · Tony

Review
Quoted 13 lines
The problem with your patch that gets rid of EDAC_MAX_MCS is making device links under /sys/bus/edac. Which is hinted at in some of the code your patch deleted:
(quoted diff) +0 -9
-       /*
-        * The memory controller needs its own bus, in order to avoid
-        * namespace conflicts at /sys/bus/edac.
-        */
-       name = kasprintf(GFP_KERNEL, "mc%d", mci->mc_idx);
-       if (!name)
-               return -ENOMEM;
-
-       mci->bus->name = name;  
Yes, and that needed to go because I am using a single bus. Which kinda makes sense because you want to have a single bus and multiple devices on it. I mean, if we *have* to have a bus.
I think this whole /sys/bus/edac thing is crap and needs to go. We have a perfectly fine hierarchy under /sys/devices/system/edac and duplicating it under /sys/bus/edac is just bollocks. IMHO. Feel free to correct me with, but but, this is useful for...
Quoted 1 line
which seemed to work.
Right.
Quoted 7 lines
But then I began wondering what are ABI expectations from applications that read the EDAC /sys files?
Is this this current source repository? https://github.com/grondo/edac-utils
This code doesn't seem to know about the "dimm*" directories below the "mc*" level. It just looks for the csrow* entries.
I guess this is a question for Mauro. I never really needed any special edac tool to get info and if you ask me, we probably should try to keep it simple and grep sysfs. So that you can always get the info without having to install any special tools. Like ftrace works on every system with just a shell and basic tools. I think this is very powerful. But this is old spartan me only thinking out loud.

Quoted 2 lines

In any case, I'm more than fine with dropping the bus hierarchy if nothing uses it.

I don't remember about any rationale behind /sys/bus/edac. It was there already before I start working on EDAC about 10 years ago. I guess it was used in the past by edac-utils (or maybe it is just a side effect of the need to create a bus on some past).

Btw, The documented EDAC ABI is /sys/devices/system/edac, as described at Documentation/ABI/testing/sysfs-devices-edac.

So, I suspect it should be safe to get rid of /sys/bus/edac, provided that it won't cause side effects at /sys/devices/system/edac.

Why I think it is safe to get rid of /sys/bus/edac?
---------------------------------------------------

As far as I can tell, there are only two toolsets: the legacy edac-utils and the rasdaemon. At least on Fedora 28, both applications are packaged (meaning that there are probably people using both).

The edac-utils uses the old sysfs entries (the ones whose entries are dated up to 2007). I don't see any changes upstream for it since 2008:

https://sourceforge.net/projects/edac-utils/

I did a grep on its source code (on its version 0.16, from 2018). It seems that it uses only /sys/devices/system/edac.

The rasdaemon uses also the new sysfs entries (the ones dated as 2012 and 2016). I'm maintaining it. Rastool not only receive traces, but it can also store them on a database and even generate ABRT events. It also uses only /sys/devices/system/edac.

On both toolsets, the sysfs entries there are important, in order to not only list the memory layout and error counts, but also to store the dimm labels.

The rasdaemon itself uses perf trace events, although Aris added support for it to work on non-daemon mode, where it just reads the counters via sysfs, at /sys/devices/system/edac.

Thanks,
Mauro

Borislav Petkov

maintainer

2018-10-01 12:47 UTC #16 lore raw

Quoted 5 lines · Mauro Carvalho Chehab

I don't remember about any rationale behind /sys/bus/edac. It was there already before I start working on EDAC about 10 years ago. I guess it was used in the past by edac-utils (or maybe it is just a side effect of the need to create a bus on some past).

Quoted 8 lines

Btw, The documented EDAC ABI is /sys/devices/system/edac, as described at Documentation/ABI/testing/sysfs-devices-edac.
So, I suspect it should be safe to get rid of /sys/bus/edac, provided that it won't cause side effects at /sys/devices/system/edac.
Why I think it is safe to get rid of /sys/bus/edac?
---------------------------------------------------

...

Thanks for the analysis. So yap, I think we should try to rip out the whole bus hierarchy then, when we have a quiet minute and whoever does this, should add your analysis to the commit message so that we know.

Thx.

Signature · 4 lines

Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Luck, Tony

maintainer

2018-10-01 22:43 UTC #17 lore raw

PATCH 1/1 EDAC: Don't add devices under /sys/bus/edac

Nobody(*) uses them. Dropping this will allow us to make the total number of memory controllers configurable (as we won't have to worry about duplicated device names under this directory).

(*) https://marc.info/?l=linux-edac&m=153809709903987&w=2

Signed-off-by Tony Luck <[email protected]>

---

Boris: Apply this, then your earlier patch to get rid of the hard coded limit on the number of memory controllers:
https://marc.info/?l=linux-edac&m=153797567628947&w=2
the combination works on my 4 socket machine. Perhaps HPE can test on their superdome.

 drivers/edac/edac_mc_sysfs.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/edac/edac_mc_sysfs.c +0 -3

index 20374b8248f0..4c1bee59c2e6 100644
@@ -405,7 +405,6 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
 				    struct csrow_info *csrow, int index)
 {
 	csrow->dev.type = &csrow_attr_type;
-	csrow->dev.bus = mci->bus;
 	csrow->dev.groups = csrow_dev_groups;
 	device_initialize(&csrow->dev);
 	csrow->dev.parent = &mci->dev;

@@ -636,7 +635,6 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
 	dimm->mci = mci;
 
 	dimm->dev.type = &dimm_attr_type;
-	dimm->dev.bus = mci->bus;
 	device_initialize(&dimm->dev);
 
 	dimm->dev.parent = &mci->dev;

@@ -940,7 +938,6 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 	device_initialize(&mci->dev);
 
 	mci->dev.parent = mci_pdev;
-	mci->dev.bus = mci->bus;
 	mci->dev.groups = groups;
 	dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
 	dev_set_drvdata(&mci->dev, mci);

Mauro Carvalho Chehab

Acked-by on v1 1/1

2018-10-02 01:22 UTC #18 lore raw

Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac

Em Mon, 1 Oct 2018 15:43:13 -0700
"Luck, Tony" <[email protected]> escreveu:

Quoted 6 lines

Nobody(*) uses them. Dropping this will allow us to make the total number of memory controllers configurable (as we won't have to worry about duplicated device names under this directory).
(*) https://marc.info/?l=linux-edac&m=153809709903987&w=2

Quoted 9 lines

Signed-off-by Tony Luck <[email protected]>
---
Boris: Apply this, then your earlier patch to get rid of the hard coded limit on the number of memory controllers:
https://marc.info/?l=linux-edac&m=153797567628947&w=2
the combination works on my 4 socket machine. Perhaps HPE can test on their superdome.

For both this and the referred patch:

Acked-by Mauro Carvalho Chehab <[email protected]>

Quoted 24 lines

 drivers/edac/edac_mc_sysfs.c | 3 ---
 1 file changed, 3 deletions(-)

drivers/edac/edac_mc_sysfs.c +0 -2

index 20374b8248f0..4c1bee59c2e6 100644
@@ -405,7 +405,6 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
 				    struct csrow_info *csrow, int index)
 {
 	csrow->dev.type = &csrow_attr_type;
-	csrow->dev.bus = mci->bus;
 	csrow->dev.groups = csrow_dev_groups;
 	device_initialize(&csrow->dev);
 	csrow->dev.parent = &mci->dev;

@@ -636,7 +635,6 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
 	dimm->mci = mci;
 
 	dimm->dev.type = &dimm_attr_type;
-	dimm->dev.bus = mci->bus;
 	device_initialize(&dimm->dev);
 
 	dimm->dev.parent = &mci->dev;

drivers/edac/edac_mc_sysfs.c int edac_create_sysfs_mci_device(struct mem_ctl_info *mci, · line 943

Quoted 7 lines

drivers/edac/edac_mc_sysfs.c +0 -1

@@ -940,7 +938,6 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci,
 	device_initialize(&mci->dev);
 
 	mci->dev.parent = mci_pdev;
-	mci->dev.bus = mci->bus;
 	mci->dev.groups = groups;
 	dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
 	dev_set_drvdata(&mci->dev, mci);

Thanks,
Mauro

Ernst, Justin

author

2018-10-02 15:51 UTC #19 lore raw

RE: [PATCH] EDAC: Don't add devices under /sys/bus/edac

The combined patches work on a 20 socket system.
Thanks!
-Justin

Quoted 74 lines

-----Original Message-----
From: Mauro Carvalho Chehab [mailto:[email protected]]
Sent: Monday, October 1, 2018 8:23 PM
To: Luck, Tony <[email protected]>
Cc: Borislav Petkov <[email protected]>; Anderson, Russ <[email protected]>; Greg KH <[email protected]>; Ernst, Justin <[email protected]>; Anderson, Russ <[email protected]>; Mauro Carvalho Chehab <[email protected]>; linux- [email protected]; [email protected]; Aristeu Rozanski Filho <[email protected]>
Subject: Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac
Em Mon, 1 Oct 2018 15:43:13 -0700
"Luck, Tony" <[email protected]> escreveu:
Quoted 6 lines
Nobody(*) uses them. Dropping this will allow us to make the total number of memory controllers configurable (as we won't have to worry about duplicated device names under this directory).
(*) https://marc.info/?l=linux-edac&m=153809709903987&w=2
Quoted 9 lines
Signed-off-by Tony Luck <[email protected]>
---
Boris: Apply this, then your earlier patch to get rid of the hard coded limit on the number of memory controllers:
https://marc.info/?l=linux-edac&m=153797567628947&w=2
the combination works on my 4 socket machine. Perhaps HPE can test on their superdome.
For both this and the referred patch:
Acked-by Mauro Carvalho Chehab <[email protected]>
drivers/edac/edac_mc_sysfs.c static int edac_create_csrow_object(struct
Quoted 8 lines
 drivers/edac/edac_mc_sysfs.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/edac/edac_mc_sysfs.c +0 -0
index 20374b8248f0..4c1bee59c2e6 100644
@@ -405,7 +405,6 @@ static int edac_create_csrow_object(struct
mem_ctl_info *mci,
drivers/edac/edac_mc_sysfs.c static int edac_create_dimm_object(struct
Quoted 8 lines
drivers/edac/edac_mc_sysfs.c +0 -1
 				    struct csrow_info *csrow, int index)
 {
 	csrow->dev.type = &csrow_attr_type;
-	csrow->dev.bus = mci->bus;
 	csrow->dev.groups = csrow_dev_groups;
 	device_initialize(&csrow->dev);
 	csrow->dev.parent = &mci->dev;
@@ -636,7 +635,6 @@ static int edac_create_dimm_object(struct
mem_ctl_info *mci,
drivers/edac/edac_mc_sysfs.c int edac_create_sysfs_mci_device(struct
Quoted 8 lines
drivers/edac/edac_mc_sysfs.c +0 -1
 	dimm->mci = mci;
 
 	dimm->dev.type = &dimm_attr_type;
-	dimm->dev.bus = mci->bus;
 	device_initialize(&dimm->dev);
 
 	dimm->dev.parent = &mci->dev;
@@ -940,7 +938,6 @@ int edac_create_sysfs_mci_device(struct
mem_ctl_info *mci,
drivers/edac/edac_mc_sysfs.c int edac_create_sysfs_mci_device(struct mem_ctl_info *mci, · line 943
Quoted 7 lines
drivers/edac/edac_mc_sysfs.c +0 -1
 	device_initialize(&mci->dev);
 
 	mci->dev.parent = mci_pdev;
-	mci->dev.bus = mci->bus;
 	mci->dev.groups = groups;
 	dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
 	dev_set_drvdata(&mci->dev, mci);
Thanks,
Mauro

Borislav Petkov

maintainer

2018-10-02 16:26 UTC #20 lore raw

Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac

Quoted 2 lines · Justin

The combined patches work on a 20 socket system.
Thanks!

Cool, thanks for testing.

Nevertheless, I'll queue them for 4.21 so that we have a full cycle of testing before we really kill the bus thing.

Borislav Petkov

maintainer

2018-11-06 14:45 UTC #21 lore raw

Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac

Quoted 8 lines · Borislav Petkov

Quoted 2 lines · Justin
The combined patches work on a 20 socket system.
Thanks!
Cool, thanks for testing.
Nevertheless, I'll queue them for 4.21 so that we have a full cycle of testing before we really kill the bus thing.

Ok, I've pushed the two patches ontop of EDAC's for-next branch, here:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=edac-for-4.21-bus

and I'd appreciate testing them one more time on the big boxes you guys have.

Diffing sysfs here shows this:

which is the bus symlink:

subsystem -> ../../../../../../bus/mc0

and I think that's ok as nothing should be using it.

Thx.

edac.after renamed from edac.before +0 -1

@@ -37,7 +37,6 @@
 /sys/devices/system/edac/mc/mc0/dimm3/power/control
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_dev_type
 /sys/devices/system/edac/mc/mc0/dimm3/size
-/sys/devices/system/edac/mc/mc0/dimm3/subsystem
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_ce_count
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_label
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_location

Ernst, Justin

author

2018-11-13 19:09 UTC #22 lore raw

RE: [PATCH] EDAC: Don't add devices under /sys/bus/edac

Looks good on a 32 socket system. All 64 memory controllers show up and I'm able to see the same sysfs diff. Thanks
-Justin

Quoted 54 lines

-----Original Message-----
From: Borislav Petkov [mailto:[email protected]]
Sent: Tuesday, November 6, 2018 8:46 AM
To: Ernst, Justin <[email protected]>; Luck, Tony <[email protected]>
Cc: Greg KH <[email protected]>; Anderson, Russ <[email protected]>; Mauro Carvalho Chehab <[email protected]>; [email protected]; linux- [email protected]; Aristeu Rozanski Filho <[email protected]>
Subject: Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac
Quoted 8 lines · Borislav Petkov
Quoted 2 lines · Justin
The combined patches work on a 20 socket system.
Thanks!
Cool, thanks for testing.
Nevertheless, I'll queue them for 4.21 so that we have a full cycle of testing before we really kill the bus thing.
Ok, I've pushed the two patches ontop of EDAC's for-next branch, here:
https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=edac-for-
4.21-bus
and I'd appreciate testing them one more time on the big boxes you guys have.
Diffing sysfs here shows this:
edac.after renamed from edac.before +0 -1
@@ -37,7 +37,6 @@
 /sys/devices/system/edac/mc/mc0/dimm3/power/control
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_dev_type
 /sys/devices/system/edac/mc/mc0/dimm3/size
-/sys/devices/system/edac/mc/mc0/dimm3/subsystem
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_ce_count
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_label
 /sys/devices/system/edac/mc/mc0/dimm3/dimm_location
which is the bus symlink:
subsystem -> ../../../../../../bus/mc0
and I think that's ok as nothing should be using it.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.

Borislav Petkov

maintainer

2018-11-13 19:15 UTC #23 lore raw

Re: [PATCH] EDAC: Don't add devices under /sys/bus/edac

Quoted 2 lines · Justin

Looks good on a 32 socket system. All 64 memory controllers show up and I'm able to see the same sysfs diff. Thanks

Thanks for testing, much appreciated!

I'm queuing the stuff for 4.21.

Zhuo, Qiuxu

2018-09-26 07:55 UTC #24 lore raw

In reply to Justin Ernst

Hi Justin,

Quoted 1 line

[ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1

Just curious, has the system(two memory controllers per socket) got more than 8 sockets? Normally, the number "1" in the above string "Skylake Socekt#1 IMC#1" should be 7 (that was 15/2), but it was 1 here.

Thanks!
-Qiuxu

Russ Anderson

2018-09-26 13:53 UTC #25 lore raw

Quoted 5 lines · Qiuxu

Hi Justin,
Quoted 1 line
[ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
Just curious, has the system(two memory controllers per socket) got more than 8 sockets? Normally, the number "1" in the above string "Skylake Socekt#1 IMC#1" should be 7 (that was 15/2), but it was 1 here.

Yes, that is from a 32 socket system.

Thanks.

Aristeu Rozanski

2018-09-26 16:13 UTC #26 lore raw

In reply to Justin Ernst

Quoted 1 line · Justin Ernst

We observe an oops in the skx_edac module during boot.

That's happening also on sb_edac too and the oops comes from memory corruption after trying to load the module several times during boot.

Borislav Petkov

maintainer Applied

2018-09-27 05:56 UTC #27 lore raw

In reply to Justin Ernst

Quoted 21 lines · Justin Ernst

We observe an oops in the skx_edac module during boot. Examining /var/log/messages:
[ 3401.985757] EDAC MC0: Giving out device to module skx_edac controller Skylake Socket#0 IMC#0
[ 3401.985887] EDAC MC1: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.986014] EDAC MC2: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
...
[ 3401.987318] EDAC MC13: Giving out device to module skx_edac controller Skylake Socket#0 IMC#1
[ 3401.987435] EDAC MC14: Giving out device to module skx_edac controller Skylake Socket#1 IMC#0
[ 3401.987556] EDAC MC15: Giving out device to module skx_edac controller Skylake Socket#1 IMC#1
[ 3401.987579] Too many memory controllers: 16
[ 3402.042614] EDAC MC: Removed device 0 for skx_edac Skylake Socket#0 IMC#0
We observe there are two memory controllers per socket, with a limit of 16. Raise the maximum number of memory controllers from 16 to 2 * MAX_NUMNODES (1024).
Cc Borislav Petkov <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
Cc [email protected]
Cc [email protected]
Acked-by Russ Anderson <[email protected]>
Signed-off-by Justin Ernst <[email protected]>

Quoted 3 lines

---
 include/linux/edac.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Applied, thanks.