| Message ID | 20170803215753.30553-5-toshi.kani@hpe.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Thu, Aug 03, 2017 at 03:57:50PM -0600, Toshi Kani wrote: > ghes_edac_register() is called for each GHES platform device > instantiated per a GHES entry in ACPI HEST table. dmi_walk() > counts the number of DIMMs on the system, and there is no need > to call it multiple times. > > Change ghes_edac_register() to call dmi_walk() only when > 'num_dimm' is uninitialized. > > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > Suggested-by: Borislav Petkov <bp@alien8.de> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > --- > drivers/edac/ghes_edac.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c > index 4e61a62..2e9ce9c 100644 > --- a/drivers/edac/ghes_edac.c > +++ b/drivers/edac/ghes_edac.c > @@ -407,15 +407,18 @@ EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error); > > int ghes_edac_register(struct ghes *ghes, struct device *dev) > { > - bool fake = false; > - int rc, num_dimm = 0; > struct mem_ctl_info *mci; > struct edac_mc_layer layers[1]; > struct ghes_edac_pvt *pvt; > struct ghes_edac_dimm_fill dimm_fill; > + int rc; > + > + static int num_dimm; > + static bool fake; > > /* Get the number of DIMMs */ > - dmi_walk(ghes_edac_count_dimms, &num_dimm); > + if (num_dimm == 0) > + dmi_walk(ghes_edac_count_dimms, &num_dimm); So the problem is that ghes_edac_register() gets called multiple times depending on how many GHES platform devices are on the system. But yet they all scan *all* DIMMs. So instead you should return if the DIMMs have been counted already and not register a second time. Which makes that whole mc counting kinda useless. So you could rip that out too. Unless I'm missing something...
On Fri, 2017-08-04 at 06:05 +0200, Borislav Petkov wrote: > On Thu, Aug 03, 2017 at 03:57:50PM -0600, Toshi Kani wrote: > > ghes_edac_register() is called for each GHES platform device > > instantiated per a GHES entry in ACPI HEST table. dmi_walk() > > counts the number of DIMMs on the system, and there is no need > > to call it multiple times. > > > > Change ghes_edac_register() to call dmi_walk() only when > > 'num_dimm' is uninitialized. > > > > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > > Suggested-by: Borislav Petkov <bp@alien8.de> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > > --- > > drivers/edac/ghes_edac.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c > > index 4e61a62..2e9ce9c 100644 > > --- a/drivers/edac/ghes_edac.c > > +++ b/drivers/edac/ghes_edac.c > > @@ -407,15 +407,18 @@ > > EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error); > > > > int ghes_edac_register(struct ghes *ghes, struct device *dev) > > { > > - bool fake = false; > > - int rc, num_dimm = 0; > > struct mem_ctl_info *mci; > > struct edac_mc_layer layers[1]; > > struct ghes_edac_pvt *pvt; > > struct ghes_edac_dimm_fill dimm_fill; > > + int rc; > > + > > + static int num_dimm; > > + static bool fake; > > > > /* Get the number of DIMMs */ > > - dmi_walk(ghes_edac_count_dimms, &num_dimm); > > + if (num_dimm == 0) > > + dmi_walk(ghes_edac_count_dimms, &num_dimm); > > So the problem is that ghes_edac_register() gets called multiple > times depending on how many GHES platform devices are on the system. > But yet they all scan *all* DIMMs. So instead you should return if > the DIMMs have been counted already and not register a second time. > > Which makes that whole mc counting kinda useless. So you could rip > that out too. > > Unless I'm missing something... GHES platform devices correspond to GHES entries, which define firmware interfaces to report generic memory errors to the OS, such as NMI and SCI. These devices are associated with all DIMMs, not a particular DIMM. Thanks, -Toshi
On Fri, Aug 04, 2017 at 09:02:17PM +0000, Kani, Toshimitsu wrote: > GHES platform devices correspond to GHES entries, which define firmware > interfaces to report generic memory errors to the OS, such as NMI and > SCI. These devices are associated with all DIMMs, not a particular > DIMM. And? Stating the obvious brings you what exactly? IOW, you still haven't answered my question.
On Sat, 2017-08-05 at 07:16 +0200, Borislav Petkov wrote: > On Fri, Aug 04, 2017 at 09:02:17PM +0000, Kani, Toshimitsu wrote: > > GHES platform devices correspond to GHES entries, which define > > firmware interfaces to report generic memory errors to the OS, such > > as NMI and SCI. These devices are associated with all DIMMs, not a > > particular DIMM. > > And? Stating the obvious brings you what exactly? > > IOW, you still haven't answered my question. Sorry about that. Let me copy your original comments to make sure I answer your questions this time. > So the problem is that ghes_edac_register() gets called multiple > times depending on how many GHES platform devices are on the system. > But yet they all scan *all* DIMMs. Right, and this patch changes ghes_edac_register() to scan all DIMMs at the first time and do not scan them next times. > So instead you should return if > the DIMMs have been counted already and not register a second time. > Which makes that whole mc counting kinda useless. So you could rip > that out too. Oh, I see. So, ghes_edac_register() should return no-op a second time, and does not call edac_mc_add_mc() to register with a separate mci. I think we should keep the current scheme, which registers an mci for each GHES entry. ghes_edac_report_mem_error() expects that error- reporting is serialized per a GHES entry. Sharing a single mci among all GHES entries / error interfaces might lead to a race condition. Thanks, -Toshi
On Mon, Aug 07, 2017 at 05:59:15PM +0000, Kani, Toshimitsu wrote: > I think we should keep the current scheme, which registers an mci for No we shouldn't. > each GHES entry. ghes_edac_report_mem_error() expects that error- > reporting is serialized per a GHES entry. Sharing a single mci among > all GHES entries / error interfaces might lead to a race condition. See how I solved it in my patchset and feel free to reuse it.
T24gRnJpLCAyMDE3LTA4LTExIGF0IDExOjA0ICswMjAwLCBCb3Jpc2xhdiBQZXRrb3Ygd3JvdGU6 DQo+IE9uIE1vbiwgQXVnIDA3LCAyMDE3IGF0IDA1OjU5OjE1UE0gKzAwMDAsIEthbmksIFRvc2hp bWl0c3Ugd3JvdGU6DQo+ID4gSSB0aGluayB3ZSBzaG91bGQga2VlcCB0aGUgY3VycmVudCBzY2hl bWUsIHdoaWNoIHJlZ2lzdGVycyBhbiBtY2kNCj4gPiBmb3INCj4gDQo+IE5vIHdlIHNob3VsZG4n dC4NCj4gDQo+ID4gZWFjaCBHSEVTIGVudHJ5LsKgwqBnaGVzX2VkYWNfcmVwb3J0X21lbV9lcnJv cigpIGV4cGVjdHMgdGhhdCBlcnJvci0NCj4gPiByZXBvcnRpbmcgaXMgc2VyaWFsaXplZCBwZXIg YSBHSEVTIGVudHJ5LsKgwqBTaGFyaW5nIGEgc2luZ2xlIG1jaQ0KPiA+IGFtb25nIGFsbCBHSEVT IGVudHJpZXMgLyBlcnJvciBpbnRlcmZhY2VzIG1pZ2h0IGxlYWQgdG8gYSByYWNlDQo+ID4gY29u ZGl0aW9uLg0KPiANCj4gU2VlIGhvdyBJIHNvbHZlZCBpdCBpbiBteSBwYXRjaHNldCBhbmQgZmVl bCBmcmVlIHRvIHJldXNlIGl0Lg0KDQpIbW0uLi4gU29ycnksIEkgZmFpbGVkIHRvIHNlZSBob3cg eW91ciBwYXRjaHNldCBzb2x2ZWQgaXQuICBXb3VsZCB5b3UNCm1pbmQgdG/CoGV4cGxhaW4gaG93 IGl0IGlzIGRvbmU/DQoNClRoYW5rcyENCi1Ub3NoaQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 14, 2017 at 03:57:35PM +0000, Kani, Toshimitsu wrote: > Hmm... Sorry, I failed to see how your patchset solved it. Would you > mind to explain how it is done? +static int __init ghes_edac_register(void) { + struct ghes_edac_pvt *pvt = ghes_pvt; Only one local ghes_pvt structure. And you handle multiple calls into ghes_edac_register() by exiting all those which are not the first one as the first one already did all the init.
T24gTW9uLCAyMDE3LTA4LTE0IGF0IDE4OjI0ICswMjAwLCBCb3Jpc2xhdiBQZXRrb3Ygd3JvdGU6 DQo+IE9uIE1vbiwgQXVnIDE0LCAyMDE3IGF0IDAzOjU3OjM1UE0gKzAwMDAsIEthbmksIFRvc2hp bWl0c3Ugd3JvdGU6DQo+ID4gSG1tLi4uIFNvcnJ5LCBJIGZhaWxlZCB0byBzZWUgaG93IHlvdXIg cGF0Y2hzZXQgc29sdmVkIGl0LsKgwqBXb3VsZA0KPiA+IHlvdSBtaW5kIHRvwqBleHBsYWluIGhv dyBpdCBpcyBkb25lPw0KPiANCj4gK3N0YXRpYyBpbnQgX19pbml0IGdoZXNfZWRhY19yZWdpc3Rl cih2b2lkKQ0KPiDCoHsNCj4gK8KgwqDCoMKgwqDCoMKgc3RydWN0IGdoZXNfZWRhY19wdnQgKnB2 dCA9IGdoZXNfcHZ0Ow0KPiANCj4gT25seSBvbmUgbG9jYWwgZ2hlc19wdnQgc3RydWN0dXJlLg0K PiANCj4gQW5kIHlvdSBoYW5kbGUgbXVsdGlwbGUgY2FsbHMgaW50byBnaGVzX2VkYWNfcmVnaXN0 ZXIoKSBieSBleGl0aW5nDQo+IGFsbCB0aG9zZSB3aGljaCBhcmUgbm90IHRoZSBmaXJzdCBvbmUg YXMgdGhlIGZpcnN0IG9uZSBhbHJlYWR5IGRpZA0KPiBhbGwgdGhlIGluaXQuDQoNClJpZ2h0LCBi dXQgdGhlIGlzc3VlIGlzIGhvdyBbZ2hlc19lZGFjX11yZXBvcnRfbWVtX2Vycm9yKCkgcHJvdGVj dHMNCmZyb20gcG9zc2libGUgY29uY3VycmVudCBjYWxscyBmcm9tIG11bHRpcGxlIEdIRVMgc291 cmNlcyB3aGVuIHRoZXJlIGlzDQpvbmx5IGEgc2luZ2xlIG1jaS4NCg0KVGhhbmtzLA0KLVRvc2hp DQog -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 14, 2017 at 04:48:57PM +0000, Kani, Toshimitsu wrote: > Right, but the issue is how [ghes_edac_]report_mem_error() protects > from possible concurrent calls from multiple GHES sources when there is > only a single mci. Do you know of an actual firmware reporting multiple errors concurrently? GHES v2 even needs to ACK the current error first before it can read the next one.
On Mon, 2017-08-14 at 19:05 +0200, Borislav Petkov wrote: > On Mon, Aug 14, 2017 at 04:48:57PM +0000, Kani, Toshimitsu wrote: > > Right, but the issue is how [ghes_edac_]report_mem_error() protects > > from possible concurrent calls from multiple GHES sources when > > there is only a single mci. > > Do you know of an actual firmware reporting multiple errors > concurrently? I do not know. We have multiple GHES entries, but they all use SCI. Since ACPICA uses a single threaded workqueue for notify handlers, they are serialized among SCIs. ACPI 6.2 defines multiple notification types in Table 18-383, and ghes_proc() can be called from ghes_poll_func(), ghes_irq_func(), and ghes_notify_sci(). So, I think it is safe to operate per an entry basis. > GHES v2 even needs to ACK the current error first before it can read > the next one. Yes, but this ACK is done per a GHES entry as well. Thanks, -Toshi
On Mon, Aug 14, 2017 at 05:52:25PM +0000, Kani, Toshimitsu wrote:
> Yes, but this ACK is done per a GHES entry as well.
So is the ghes_edac_report_mem_error() call.
T24gTW9uLCAyMDE3LTA4LTE0IGF0IDIwOjA1ICswMjAwLCBCb3Jpc2xhdiBQZXRrb3Ygd3JvdGU6 DQo+IE9uIE1vbiwgQXVnIDE0LCAyMDE3IGF0IDA1OjUyOjI1UE0gKzAwMDAsIEthbmksIFRvc2hp bWl0c3Ugd3JvdGU6DQo+ID4gWWVzLCBidXQgdGhpcyBBQ0sgaXMgZG9uZSBwZXIgYSBHSEVTIGVu dHJ5IGFzIHdlbGwuDQo+IA0KPiBTbyBpcyB0aGUgZ2hlc19lZGFjX3JlcG9ydF9tZW1fZXJyb3Io KSBjYWxsLg0KDQpSaWdodCwgZ2hlc19lZGFjX3JlcG9ydF9tZW1fZXJyb3IoKSBnZXRzIHNlcmlh bGl6ZWQgcGVyIGEgR0hFUyBlbnRyeSwNCmJ1dCBub3QgZ2xvYmFsbHkuDQoNClRoYW5rcywNCi1U b3NoaQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 14, 2017 at 06:17:47PM +0000, Kani, Toshimitsu wrote: > Right, ghes_edac_report_mem_error() gets serialized per a GHES entry, > but not globally. Globally what? What is the actual potential scenario for concurrency issues you see? Example pls.
On Mon, 2017-08-14 at 20:35 +0200, Borislav Petkov wrote: > On Mon, Aug 14, 2017 at 06:17:47PM +0000, Kani, Toshimitsu wrote: > > Right, ghes_edac_report_mem_error() gets serialized per a GHES > > entry, but not globally. > > Globally what? GHES v2's ACK is not a global lock. So, it does not guarantee that ghes_edac_report_mem_error() never gets called concurrently. > What is the actual potential scenario for concurrency issues you see? > Example pls. ghes_probe() supports multiple sources defined in acpi_hest_notify_types. Say, there are two entries for memory errors, one with ACPI_HEST_NOTIFY_EXTERNAL and the other with ACPI_HEST_NOTIFY_SCI. They may report errors independently. While ghes_edac_report_mem_error() is being called from the SCI, it can be called from the ext interrupt at a same time. I do not know how likely we see such case, but the code should be written according to the spec. Thanks, -Toshi
On Mon, Aug 14, 2017 at 07:02:15PM +0000, Kani, Toshimitsu wrote: > I do not know how likely we see such case, but the code should be > written according to the spec. Well, then you'll have to make ghes_edac_report_mem_error() reentrant. Which doesn't look that hard as the only thing it really needs from struct ghes_edac_pvt are those string buffers. I guess you can try to do the simplest thing first and allocate them on the stack.
On Mon, 2017-08-14 at 21:34 +0200, Borislav Petkov wrote: > On Mon, Aug 14, 2017 at 07:02:15PM +0000, Kani, Toshimitsu wrote: > > I do not know how likely we see such case, but the code should be > > written according to the spec. > > Well, then you'll have to make ghes_edac_report_mem_error() > reentrant. Which doesn't look that hard as the only thing it really > needs from struct ghes_edac_pvt are those string buffers. I guess you > can try to do the simplest thing first and allocate them on the > stack. ghes_edac_report_mem_error() is reentrant as it is now. I think the current code design of allocating mci & ghes_edac_pvt for each GHES source entry makes sense. edac_raw_mc_handle_error() also has the same expectation that the call is serialized per mci. Thanks, -Toshi
On Mon, Aug 14, 2017 at 08:17:54PM +0000, Kani, Toshimitsu wrote: > I think the current code design of allocating mci & ghes_edac_pvt for > each GHES source entry makes sense. And I don't. > edac_raw_mc_handle_error() also has the same expectation that the call > is serialized per mci. There's no such thing as "per mci" if the driver scans *all DIMMs* per register call. If it does it this way, then it is only one mci. It is actually wrong right now because if you register more than one mci and you do edac_inc_ce_error()/edac_inc_ue_error(), potentially different counters get incremented for the same errors. Exactly because each instance registered is *wrongly* responsible for all DIMMs on the system. So you either need to partition the DIMMs per mci (which I can't imagine how it would work) or introduce locking when incrementing the mci-> counters.
On Mon, 2017-08-14 at 22:39 +0200, Borislav Petkov wrote: > On Mon, Aug 14, 2017 at 08:17:54PM +0000, Kani, Toshimitsu wrote: > > I think the current code design of allocating mci & ghes_edac_pvt > > for each GHES source entry makes sense. > > And I don't. > > > edac_raw_mc_handle_error() also has the same expectation that the > > call is serialized per mci. > > There's no such thing as "per mci" if the driver scans *all DIMMs* > per register call. If it does it this way, then it is only one mci. ghes_edac instantiates an mci as a pseudo device representing a GHES error source. Each error source associates with all DIMMs, and may report errors independently. As ghes_edac is an GHES error-reporting wrapper to edac, this abstraction makes sense. > It is actually wrong right now because if you register more than one > mci and you do edac_inc_ce_error()/edac_inc_ue_error(), potentially > different counters get incremented for the same errors. Exactly > because each instance registered is *wrongly* responsible for all > DIMMs on the system. I do not see a problem in having counters for each GHES error source. This is just statistics info, and ghes_edac does not expect any OS action from the counters. > So you either need to partition the DIMMs per mci (which I can't > imagine how it would work) or introduce locking when incrementing the > mci->counters. I do not think changing the calling convention to edac library interfaces is a good idea for a special case like ghes_edac. Such changes can be a burden for us going forward. I think ghes_edac just needs to work with the current prerequisite. User apps like ras-mc-ctl works as expected for a given (not-so-great) DIMM info from SMBIOS as well. I do not see a probelm from user perspective, either. Thanks, -Toshi
On Tue, Aug 15, 2017 at 08:35:51AM -0700, Kani, Toshimitsu wrote: > User apps like ras-mc-ctl works as expected for a given (not-so-great) > DIMM info from SMBIOS as well. I do not see a probelm from user > perspective, either. Won't the user see all their DIMMs reported for each memory controller under /sys/devices/system/edac/mc/mc*/dimm* ? That sounds confusing. -Tony -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 15, 2017 at 03:35:51PM +0000, Kani, Toshimitsu wrote: > ghes_edac instantiates an mci as a pseudo device representing a GHES > error source. Each error source associates with all DIMMs, and may > report errors independently. As ghes_edac is an GHES error-reporting > wrapper to edac, this abstraction makes sense. Bullshit. An MCI is a memory controller descriptor. That doesn't fit the GHES platform devices that get probed. GHES platform device != MCI. How many times do I need to say this for it to get through to you? > I do not see a problem in having counters for each GHES error source. And the error counters of that "simulated" mci get incremented depending on which pointer gets passed in from GHES? More bullshit. > This is just statistics info, and ghes_edac does not expect any OS > action from the counters. So let me know if you don't want to do it and rather would prefer to pointlessly debate. I certainly don't want to waste my time debating.
On Tue, 2017-08-15 at 08:48 -0700, Luck, Tony wrote: > On Tue, Aug 15, 2017 at 08:35:51AM -0700, Kani, Toshimitsu wrote: > > User apps like ras-mc-ctl works as expected for a given (not-so- > > great) DIMM info from SMBIOS as well. I do not see a probelm from > > user perspective, either. > > Won't the user see all their DIMMs reported for each memory > controller under /sys/devices/system/edac/mc/mc*/dimm* ? > > That sounds confusing. ghes_edac only fills dimm_info to the first mci. So, users do not see duplicated info. Thanks, -Toshi
On Tue, 2017-08-15 at 17:50 +0200, Borislav Petkov wrote: > On Tue, Aug 15, 2017 at 03:35:51PM +0000, Kani, Toshimitsu wrote: > > ghes_edac instantiates an mci as a pseudo device representing a > > GHES error source. Each error source associates with all DIMMs, > > and may report errors independently. As ghes_edac is an GHES > > error-reporting wrapper to edac, this abstraction makes sense. > > Bullshit. > > An MCI is a memory controller descriptor. That doesn't fit the GHES > platform devices that get probed. GHES platform device != MCI. How > many times do I need to say this for it to get through to you? Right, but it has to be a "pseudo" device for ghes_edac. There is no memory controller info available. A single mci does not make it a real memory controller, either. > > I do not see a problem in having counters for each GHES error > > source. > > And the error counters of that "simulated" mci get incremented > depending on which pointer gets passed in from GHES? More bullshit. > > > This is just statistics info, and ghes_edac does not expect any OS > > action from the counters. > > So let me know if you don't want to do it and rather would prefer to > pointlessly debate. I certainly don't want to waste my time debating. Yes, ghes_edac refactoring like this should be considered a separate item. My patchset is aimed to introduce a platform-check to attach ghes_edac on supported platforms. Thanks, -Toshi
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index 4e61a62..2e9ce9c 100644 --- a/drivers/edac/ghes_edac.c +++ b/drivers/edac/ghes_edac.c @@ -407,15 +407,18 @@ EXPORT_SYMBOL_GPL(ghes_edac_report_mem_error); int ghes_edac_register(struct ghes *ghes, struct device *dev) { - bool fake = false; - int rc, num_dimm = 0; struct mem_ctl_info *mci; struct edac_mc_layer layers[1]; struct ghes_edac_pvt *pvt; struct ghes_edac_dimm_fill dimm_fill; + int rc; + + static int num_dimm; + static bool fake; /* Get the number of DIMMs */ - dmi_walk(ghes_edac_count_dimms, &num_dimm); + if (num_dimm == 0) + dmi_walk(ghes_edac_count_dimms, &num_dimm); /* Check if we've got a bogus BIOS */ if (num_dimm == 0) {
ghes_edac_register() is called for each GHES platform device instantiated per a GHES entry in ACPI HEST table. dmi_walk() counts the number of DIMMs on the system, and there is no need to call it multiple times. Change ghes_edac_register() to call dmi_walk() only when 'num_dimm' is uninitialized. Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Suggested-by: Borislav Petkov <bp@alien8.de> Cc: Borislav Petkov <bp@alien8.de> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> --- drivers/edac/ghes_edac.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html