Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1231854 > unrolled thread

[Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

Back to article view | Back to linux.kernel

Contents

  [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts Ashok Raj <ashok.raj@intel.com> - 2015-09-24 07:00 +0200
    [Patch V1 2/3] x86, mce: Refactor parts of mce_log() to reuse when logging from offline CPUs Ashok Raj <ashok.raj@intel.com> - 2015-09-24 07:00 +0200
    Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core  parts Borislav Petkov <bp@alien8.de> - 2015-09-24 17:50 +0200
      RE: [Patch V1 1/3] x86, mce: MCE log size not enough for high core  parts "Luck, Tony" <tony.luck@intel.com> - 2015-09-24 20:50 +0200
        Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core  parts Borislav Petkov <bp@alien8.de> - 2015-09-24 21:00 +0200
          RE: [Patch V1 1/3] x86, mce: MCE log size not enough for high core  parts "Luck, Tony" <tony.luck@intel.com> - 2015-09-24 21:10 +0200
            Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core  parts Borislav Petkov <bp@alien8.de> - 2015-09-24 21:30 +0200
              Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core  parts "Raj, Ashok" <ashok.raj@intel.com> - 2015-09-24 22:30 +0200
                Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core  parts Borislav Petkov <bp@alien8.de> - 2015-09-24 23:10 +0200
                  Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core  parts "Raj, Ashok" <ashok.raj@intel.com> - 2015-09-24 23:30 +0200
                    Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core  parts Borislav Petkov <bp@alien8.de> - 2015-09-25 10:30 +0200
                      Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core  parts "Raj, Ashok" <ashok.raj@intel.com> - 2015-09-25 18:30 +0200

#1231854 — [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

MCE_LOG_LEN appears to be short for high core count parts. Especially when
handling fatal errors, we don't clear MCE banks. Socket level MC banks
are visible to all CPUs that share banks.

Assuming 18 core part, 2 threads per core 2 banks per thread and couple uncore
MSRs. Rounding to 128 with some fudge to grow in future.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Suggested-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/mce.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 2dbc0bf..4293ae7 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -88,7 +88,7 @@
 #define MCE_EXTENDED_BANK	128
 #define MCE_THERMAL_BANK	(MCE_EXTENDED_BANK + 0)
 
-#define MCE_LOG_LEN 32
+#define MCE_LOG_LEN 		128
 #define MCE_LOG_SIGNATURE	"MACHINECHECK"
 
 /*
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1231855 — [Patch V1 2/3] x86, mce: Refactor parts of mce_log() to reuse when logging from offline CPUs

Simply refactoring part of mce_log() to facilitate logging from offline
CPUs.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 48bd244..2df073d 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -152,16 +152,10 @@ static struct mce_log mcelog = {
 	.recordlen	= sizeof(struct mce),
 };
 
-void mce_log(struct mce *mce)
+static void mce_log_add(struct mce *mce)
 {
 	unsigned next, entry;
 
-	/* Emit the trace record: */
-	trace_mce_record(mce);
-
-	if (!mce_gen_pool_add(mce))
-		irq_work_queue(&mce_irq_work);
-
 	mce->finished = 0;
 	wmb();
 	for (;;) {
@@ -199,6 +193,19 @@ void mce_log(struct mce *mce)
 	set_bit(0, &mce_need_notify);
 }
 
+void mce_log(struct mce *mce)
+{
+	unsigned next, entry;
+
+	/* Emit the trace record: */
+	trace_mce_record(mce);
+
+	if (!mce_gen_pool_add(mce))
+		irq_work_queue(&mce_irq_work);
+
+	mce_log_add(mce);
+}
+
 void mce_inject_log(struct mce *m)
 {
 	mutex_lock(&mce_chrdev_read_mutex);
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232201 — Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

On Thu, Sep 24, 2015 at 01:48:38AM -0400, Ashok Raj wrote:
> MCE_LOG_LEN appears to be short for high core count parts. Especially when
> handling fatal errors, we don't clear MCE banks. Socket level MC banks
> are visible to all CPUs that share banks.
> 
> Assuming 18 core part, 2 threads per core 2 banks per thread and couple uncore
> MSRs. Rounding to 128 with some fudge to grow in future.
> 
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Suggested-by: Tony Luck <tony.luck@intel.com>
> ---
>  arch/x86/include/asm/mce.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
> index 2dbc0bf..4293ae7 100644
> --- a/arch/x86/include/asm/mce.h
> +++ b/arch/x86/include/asm/mce.h
> @@ -88,7 +88,7 @@
>  #define MCE_EXTENDED_BANK	128
>  #define MCE_THERMAL_BANK	(MCE_EXTENDED_BANK + 0)
>  
> -#define MCE_LOG_LEN 32
> +#define MCE_LOG_LEN 		128
>  #define MCE_LOG_SIGNATURE	"MACHINECHECK"

Hmm, I don't think this is what I meant when we talked about it
previously. So let me try again:

Now that we have this shiny 2-pages sized lockless gen_pool, why are we
still dealing with struct mce_log mcelog? Why can't we rip it out and
kill it finally? And switch to the gen_pool?

All code that reads from mcelog - /dev/mcelog chrdev - should switch to
the lockless buffer and will iterate through the logged MCEs there.

I think this way we're much better prepared for future machine sizes.
We can even use memblock to allocate appropriate memory at boot for the
gen_pool if the 2 pages are not enough.

Hmmm?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232332 — RE: [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

PiBOb3cgdGhhdCB3ZSBoYXZlIHRoaXMgc2hpbnkgMi1wYWdlcyBzaXplZCBsb2NrbGVzcyBnZW5f
cG9vbCwgd2h5IGFyZSB3ZQ0KPiBzdGlsbCBkZWFsaW5nIHdpdGggc3RydWN0IG1jZV9sb2cgbWNl
bG9nPyBXaHkgY2FuJ3Qgd2UgcmlwIGl0IG91dCBhbmQNCj4ga2lsbCBpdCBmaW5hbGx5PyBBbmQg
c3dpdGNoIHRvIHRoZSBnZW5fcG9vbD8NCj4NCj4gQWxsIGNvZGUgdGhhdCByZWFkcyBmcm9tIG1j
ZWxvZyAtIC9kZXYvbWNlbG9nIGNocmRldiAtIHNob3VsZCBzd2l0Y2ggdG8NCj4gdGhlIGxvY2ts
ZXNzIGJ1ZmZlciBhbmQgd2lsbCBpdGVyYXRlIHRocm91Z2ggdGhlIGxvZ2dlZCBNQ0VzIHRoZXJl
Lg0KDQpJIHRoaW5rIHdlIGhhdmUgYSBwcm9ibGVtIG9mIHdoZW4gdG8gZGVsZXRlIGVudHJpZXMg
Li4uIHdlIGNhbiBvbmx5IGRvIHRoYXQNCndoZW4gYWxsIHRoZSBpbnRlcmVzdGVkIGNvbnN1bWVy
cyBvZiBsb2dzIGhhdmUgc2VlbiBhbiBlbnRyeS4gQnV0IHdlIGhhdmUNCm5vIGNvbnRyb2wgaW4g
dGhlIGtlcm5lbCBvbiBjb25zdW1wdGlvbiBmcm9tIC9kZXYvbWNlbG9nLg0KDQpIaXN0b3JpYyBz
ZW1hbnRpY3Mgd2FzIHRoYXQgdGhlIGZpcnN0IE1DRV9MT0dfTEVOIGVycm9ycyB3b3VsZCBzaXQN
CmluIHRoZSBidWZmZXIgd2FpdGluZyBmb3IgdXNlcnNwYWNlIHRvIGJlZ2luIHJ1bm5pbmcgYSBk
YWVtb24gdG8gcmVhZA0KdGhlbS4NCg0KLVRvbnkNCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232334 — Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

On Thu, Sep 24, 2015 at 06:44:25PM +0000, Luck, Tony wrote:
> > Now that we have this shiny 2-pages sized lockless gen_pool, why are we
> > still dealing with struct mce_log mcelog? Why can't we rip it out and
> > kill it finally? And switch to the gen_pool?
> >
> > All code that reads from mcelog - /dev/mcelog chrdev - should switch to
> > the lockless buffer and will iterate through the logged MCEs there.
> 
> I think we have a problem of when to delete entries ... we can only do that
> when all the interested consumers of logs have seen an entry. But we have
> no control in the kernel on consumption from /dev/mcelog.
> 
> Historic semantics was that the first MCE_LOG_LEN errors would sit
> in the buffer waiting for userspace to begin running a daemon to read
> them.

Right, we can tag them with various flags when iterating over them in
the gen_pool. The in-kernel consumers can look at them, modify, update
the information, etc.

Userspace can then consume them and delete them.

If we get new ones logged in the meantime and userspace hasn't managed
to consume and delete the present ones yet, we overwrite the oldest ones
and set MCE_OVERFLOW like mce_log does now for mcelog. And that's no
difference in functionality than what we have now.

The advantage is that we get in-kernel consumers to look at them first
and we keep all MCE records concentrated in one place.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232341 — RE: [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

PiBJZiB3ZSBnZXQgbmV3IG9uZXMgbG9nZ2VkIGluIHRoZSBtZWFudGltZSBhbmQgdXNlcnNwYWNl
IGhhc24ndCBtYW5hZ2VkDQo+IHRvIGNvbnN1bWUgYW5kIGRlbGV0ZSB0aGUgcHJlc2VudCBvbmVz
IHlldCwgd2Ugb3ZlcndyaXRlIHRoZSBvbGRlc3Qgb25lcw0KPiBhbmQgc2V0IE1DRV9PVkVSRkxP
VyBsaWtlIG1jZV9sb2cgZG9lcyBub3cgZm9yIG1jZWxvZy4gQW5kIHRoYXQncyBubw0KPiBkaWZm
ZXJlbmNlIGluIGZ1bmN0aW9uYWxpdHkgdGhhbiB3aGF0IHdlIGhhdmUgbm93Lg0KDQpVbW1tbS4g
Tm8uDQoNCiAgICAgICAgICAgICAgICBmb3IgKDs7KSB7DQoNCiAgICAgICAgICAgICAgICAgICAg
ICAgIC8qDQogICAgICAgICAgICAgICAgICAgICAgICAgKiBXaGVuIHRoZSBidWZmZXIgZmlsbHMg
dXAgZGlzY2FyZCBuZXcgZW50cmllcy4NCiAgICAgICAgICAgICAgICAgICAgICAgICAqIEFzc3Vt
ZSB0aGF0IHRoZSBlYXJsaWVyIGVycm9ycyBhcmUgdGhlIG1vcmUNCiAgICAgICAgICAgICAgICAg
ICAgICAgICAqIGludGVyZXN0aW5nIG9uZXM6DQogICAgICAgICAgICAgICAgICAgICAgICAgKi8N
CiAgICAgICAgICAgICAgICAgICAgICAgIGlmIChlbnRyeSA+PSBNQ0VfTE9HX0xFTikgew0KICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICBzZXRfYml0KE1DRV9PVkVSRkxPVywNCiAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAodW5zaWduZWQgbG9uZyAqKSZtY2Vs
b2cuZmxhZ3MpOw0KICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICByZXR1cm47DQogICAg
ICAgICAgICAgICAgICAgICAgICB9DQoNCi1Ub255DQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232372 — Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

On Thu, Sep 24, 2015 at 07:00:46PM +0000, Luck, Tony wrote:
> > If we get new ones logged in the meantime and userspace hasn't managed
> > to consume and delete the present ones yet, we overwrite the oldest ones
> > and set MCE_OVERFLOW like mce_log does now for mcelog. And that's no
> > difference in functionality than what we have now.
> 
> Ummmm. No.
> 
>                 for (;;) {
> 
>                         /*
>                          * When the buffer fills up discard new entries.
>                          * Assume that the earlier errors are the more
>                          * interesting ones:
>                          */
>                         if (entry >= MCE_LOG_LEN) {
>                                 set_bit(MCE_OVERFLOW,
>                                         (unsigned long *)&mcelog.flags);
>                                 return;
>                         }

Ah, we return. But we shouldn't return - we should overwrite. I believe
we've talked about the policy of overwriting old errors with new ones.

TBH, I don't think there's a 100%-correct policy to act according to
when our error logging buffers are full:

- we can overwrite old errors with new but then this way we might lose
the one important error record with which it all started.

- if we don't overwrite, we might fill up with "unimportant" correctable
error records and miss other, more important ones which happen now

- ...

We could try to implement some cheap heuristics which decide what and
when to overwrite but I'm sceptical it'll be always correct...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232420 — Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

Hi Boris

On Thu, Sep 24, 2015 at 09:22:24PM +0200, Borislav Petkov wrote:
> 
> Ah, we return. But we shouldn't return - we should overwrite. I believe
> we've talked about the policy of overwriting old errors with new ones.
> 

Another reason i had a separate buffer in my earlier patch was to avoid 
calling rcu() functions from the offline CPU. I had an offline discussion 
with Paul McKenney  he said don't do that... 

mce_gen_pool_add()->gen_pool_alloc() which calls rcu_read_lock() and such. 
So it didn't seem approprite. 

Also the function doesn't seem safe to be called in NMI context. Although
MCE is different, for all intentional purposes we should treat both as same
priority. The old style log is simple and tested in those cases.

I like everything you say below... something we could do as our next phase
of improving logging and might need more careful work to build it right.

just like how MC banks have overwrite rules, we can possibly do something
like that if the buffer fills up.

> TBH, I don't think there's a 100%-correct policy to act according to
> when our error logging buffers are full:
> 
> - we can overwrite old errors with new but then this way we might lose
> the one important error record with which it all started.
> 
> - if we don't overwrite, we might fill up with "unimportant" correctable
> error records and miss other, more important ones which happen now
> 
> - ...
> 
> We could try to implement some cheap heuristics which decide what and
> when to overwrite but I'm sceptical it'll be always correct...
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232435 — Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

On Thu, Sep 24, 2015 at 01:22:12PM -0700, Raj, Ashok wrote:
> Another reason i had a separate buffer in my earlier patch was to avoid 
> calling rcu() functions from the offline CPU. I had an offline discussion 
> with Paul McKenney  he said don't do that... 
> 
> mce_gen_pool_add()->gen_pool_alloc() which calls rcu_read_lock() and such. 
> So it didn't seem approprite.

How are you ever going to call into those from an offlined CPU?!

And that's easy:

	if (!cpu_online(cpu))
		return;

> Also the function doesn't seem safe to be called in NMI context. Although

That's why it is a lockless buffer - we added it *exactly* because we didn't
want to call printk in an NMI context. So please expand...

> MCE is different, for all intentional purposes we should treat both as same
> priority. The old style log is simple and tested in those cases.
> 
> I like everything you say below... something we could do as our next phase
> of improving logging and might need more careful work to build it right.
> 
> just like how MC banks have overwrite rules, we can possibly do something
> like that if the buffer fills up.

Right.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232440 — Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

Hi Boris

I should have expanded on it.. 

On Thu, Sep 24, 2015 at 11:07:33PM +0200, Borislav Petkov wrote:
> 
> How are you ever going to call into those from an offlined CPU?!
> 
> And that's easy:
> 
> 	if (!cpu_online(cpu))
> 		return;
> 

The last patch of that series had 2 changes.

1. Allow offline cpu's to participate in the rendezvous. Since in the odd
chance the offline cpus have any errors collected we can still report them.
(we changed mce_start/mce_end to use cpu_present_mask instead of just 
online map). 

Without this change today if i were to inject an broadcast MCE
it ends up hanging, since the offline cpu is also incrementing mce_callin.
It will always end up more than cpu_online_mask by the number of cpu's
logically offlined

Consider for e.g. if 2 thread of the core are offline. And the MLC picks up
an error. Other cpus in the socket can't access them. Only way is to let those 
CPUs read and report their own banks as they are core scoped. In upcoming CPUs
we have some banks that can be thread scoped as well.

Its understood OS doesn't execute any code on those CPUs. But SMI can still
run on them, and could collect errors that can be logged.

2. If the cpu is offline, we copied them to mce_log buffer, and them copy 
those out from the rendezvous master during mce_reign().

If we were to replace this mce_log_add() with gen_pool_add(), then i would 
have to call mce_gen_pool_add() from the offline CPU. This will end up calling
RCU functions. 

We don't want to leave any errors reported by the offline CPU for purpose
of logging. It is rare, but still interested in capturing those errors if they
were to happen.

Does this help?

Cheers,
Ashok


> > Also the function doesn't seem safe to be called in NMI context. Although
> 
> That's why it is a lockless buffer - we added it *exactly* because we didn't
> want to call printk in an NMI context. So please expand...
> 

S
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232652 — Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

+ x86@kernel.org

On Thu, Sep 24, 2015 at 02:25:41PM -0700, Raj, Ashok wrote:
> Hi Boris
> 
> I should have expanded on it.. 
> 
> On Thu, Sep 24, 2015 at 11:07:33PM +0200, Borislav Petkov wrote:
> > 
> > How are you ever going to call into those from an offlined CPU?!
> > 
> > And that's easy:
> > 
> > 	if (!cpu_online(cpu))
> > 		return;
> > 
> 
> The last patch of that series had 2 changes.
> 
> 1. Allow offline cpu's to participate in the rendezvous. Since in the odd
> chance the offline cpus have any errors collected we can still report them.
> (we changed mce_start/mce_end to use cpu_present_mask instead of just 
> online map).

This is not necessarily wrong - it is just unusual.

> Without this change today if i were to inject an broadcast MCE
> it ends up hanging, since the offline cpu is also incrementing mce_callin.
> It will always end up more than cpu_online_mask by the number of cpu's
> logically offlined

Yeah, I'd like to have a bit somewhere which says "don't report MCEs on this
core." But we talked about this already.

> Consider for e.g. if 2 thread of the core are offline. And the MLC picks up

What is MLC?

> an error. Other cpus in the socket can't access them. Only way is to let those 
> CPUs read and report their own banks as they are core scoped. In upcoming CPUs
> we have some banks that can be thread scoped as well.
> 
> Its understood OS doesn't execute any code on those CPUs. But SMI can still
> run on them, and could collect errors that can be logged.

Well, that is not our problem, is it?

I mean, SMM wants to stay undetected. When all of a sudden offlined
cores start reporting MCEs, that's going to raise some brows.

Regardless, there are other reasons why offlined cores might report MCEs
- the fact that logical cores share functional units and data flow goes
through them might trip the reporting on those cores. Yadda yadda...

> 2. If the cpu is offline, we copied them to mce_log buffer, and them copy 
> those out from the rendezvous master during mce_reign().
> 
> If we were to replace this mce_log_add() with gen_pool_add(), then i would 
> have to call mce_gen_pool_add() from the offline CPU. This will end up calling
> RCU functions. 
> 
> We don't want to leave any errors reported by the offline CPU for purpose
> of logging. It is rare, but still interested in capturing those errors if they
> were to happen.
> 
> Does this help?

So first of all, we need to hold this down somewhere, maybe in
Documentation/ to explain why we're running on offlined cores. This is
certainly unusual code and people will ask WTF is going on there.

Then, I really really don't like a static buffer which we will have
to increase with each new bigger machine configuration. This is just
clumsy.

It'd be probably much better to make that MCE buffer per CPU. We can
say, we're allowed to log 2-3, hell, 5 errors in it and when we're done
with the rendezvous, an online core goes and flushes out the error
records to gen_pool.

This scales much better than any artificial MCE_LOG_LEN size.

Oh, and we either overwrite old errors when we fill up the percpu buffer
or we return. that's something we can discuss later. Or we come up with
a bit smarter strategy of selecting which ones to overwrite.

Just artificially increasing a static buffer is not good design IMO.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232927 — Re: [Patch V1 1/3] x86, mce: MCE log size not enough for high core parts

On Fri, Sep 25, 2015 at 10:29:01AM +0200, Borislav Petkov wrote:
> > > 
> > 
> > The last patch of that series had 2 changes.
> > 
> > 1. Allow offline cpu's to participate in the rendezvous. Since in the odd
> > chance the offline cpus have any errors collected we can still report them.
> > (we changed mce_start/mce_end to use cpu_present_mask instead of just 
> > online map).
> 
> This is not necessarily wrong - it is just unusual.

Correct!.
> 
> > Without this change today if i were to inject an broadcast MCE
> > it ends up hanging, since the offline cpu is also incrementing mce_callin.
> > It will always end up more than cpu_online_mask by the number of cpu's
> > logically offlined
> 
> Yeah, I'd like to have a bit somewhere which says "don't report MCEs on this
> core." But we talked about this already.
> 
> > Consider for e.g. if 2 thread of the core are offline. And the MLC picks up
> 
> What is MLC?

Mid Level Cache. This is shared between the 2 threads in that core. As opposed
to the Last Level Cache (LLC) which is shared between all the threads in the
socket.

> 
> > an error. Other cpus in the socket can't access them. Only way is to let those 
> > CPUs read and report their own banks as they are core scoped. In upcoming CPUs
> > we have some banks that can be thread scoped as well.
> > 
> > Its understood OS doesn't execute any code on those CPUs. But SMI can still
> > run on them, and could collect errors that can be logged.
> 
> Well, that is not our problem, is it?
> 
> I mean, SMM wants to stay undetected. When all of a sudden offlined
> cores start reporting MCEs, that's going to raise some brows.

You are right.. i was simply trying to state how an offline CPU from the
OS perspective could still be collecting errors. Only trying to highlight
what happens from a platform level.

> 
> Regardless, there are other reasons why offlined cores might report MCEs
> - the fact that logical cores share functional units and data flow goes
> through them might trip the reporting on those cores. Yadda yadda...

Yep!
> 
> > 2. If the cpu is offline, we copied them to mce_log buffer, and them copy 
> > those out from the rendezvous master during mce_reign().
> > 
> > If we were to replace this mce_log_add() with gen_pool_add(), then i would 
> > have to call mce_gen_pool_add() from the offline CPU. This will end up calling
> > RCU functions. 
> > 
> > We don't want to leave any errors reported by the offline CPU for purpose
> > of logging. It is rare, but still interested in capturing those errors if they
> > were to happen.
> > 
> > Does this help?
> 
> So first of all, we need to hold this down somewhere, maybe in
> Documentation/ to explain why we're running on offlined cores. This is
> certainly unusual code and people will ask WTF is going on there.

Good idea to document these weird cases. I can do it in either the 
cpu-hotplug.txt, or in the x86/x86_64/machinecheck file as appropriate.
Will add that in my next update.

> 
> Then, I really really don't like a static buffer which we will have
> to increase with each new bigger machine configuration. This is just
> clumsy.
> 
> It'd be probably much better to make that MCE buffer per CPU. We can
> say, we're allowed to log 2-3, hell, 5 errors in it and when we're done
> with the rendezvous, an online core goes and flushes out the error
> records to gen_pool.

That makes good sense.. i will make these per-cpu and also look at 
clearing them during mce_panic to make sure we flush them in fatal cases.

per-cpu certainly scales better than a static number.. 

> 
> This scales much better than any artificial MCE_LOG_LEN size.
> 
> Oh, and we either overwrite old errors when we fill up the percpu buffer
> or we return. that's something we can discuss later. Or we come up with
> a bit smarter strategy of selecting which ones to overwrite.
> 
> Just artificially increasing a static buffer is not good design IMO.

Agreed.. thanks, i will get another rev rolling.

Cheers,
Ashok

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web