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

Groups > linux.kernel > #1721634 > unrolled thread

Re: [PATCH 0/7] EDAC, mce_amd: Issue decoded MCE through the tracepoint

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/7] EDAC, mce_amd: Issue decoded MCE through the  tracepoint Borislav Petkov <bp@alien8.de> - 2017-08-28 15:50 +0200
    Re: [PATCH 0/7] EDAC, mce_amd: Issue decoded MCE through the  tracepoint Borislav Petkov <bp@alien8.de> - 2017-08-30 13:50 +0200
      Re: [PATCH 0/7] EDAC, mce_amd: Issue decoded MCE through the  tracepoint mark gross <mgross@linux.intel.com> - 2017-08-30 23:50 +0200
        Re: [PATCH 0/7] EDAC, mce_amd: Issue decoded MCE through the  tracepoint Borislav Petkov <bp@alien8.de> - 2017-08-31 00:10 +0200
          Re: [PATCH 0/7] EDAC, mce_amd: Issue decoded MCE through the  tracepoint Borislav Petkov <bp@alien8.de> - 2017-08-31 21:20 +0200
        Re: [PATCH 0/7] EDAC, mce_amd: Issue decoded MCE through the  tracepoint Steven Rostedt <rostedt@goodmis.org> - 2017-08-31 01:40 +0200
          Re: [PATCH 0/7] EDAC, mce_amd: Issue decoded MCE through the  tracepoint mark gross <mgross@linux.intel.com> - 2017-09-04 01:40 +0200
            Re: [PATCH 0/7] EDAC, mce_amd: Issue decoded MCE through the  tracepoint Borislav Petkov <bp@alien8.de> - 2017-09-04 12:50 +0200

#1721634 — Re: [PATCH 0/7] EDAC, mce_amd: Issue decoded MCE through the tracepoint

On Fri, Aug 25, 2017 at 12:24:04PM +0200, Borislav Petkov wrote:
> Next step is adding that to rasdaemon.

Ok, below is the dirty version of the changes that need to go into
rasdaemon, I'll clean that up later. With it, I get:

# ./rasdaemon -f
overriding event (541) ras:mc_event with new print handler
rasdaemon: ras:mc_event event enabled
rasdaemon: Enabled event ras:mc_event
overriding event (58) mce:mce_record with new print handler
rasdaemon: mce:mce_record event enabled
rasdaemon: Enabled event mce:mce_record
overriding event (542) ras:extlog_mem_event with new print handler
rasdaemon: ras:extlog_mem_event event enabled
rasdaemon: Enabled event ras:extlog_mem_event
rasdaemon: Listening to events for cpus 0 to 7
cpu 07:           <...>-104   [1433913776]     0.000006: mce_record:           2017-08-28 17:41:01 +0200 bank=4, status= 9c7d410092080813, MC4 Error (node 2): DRAM ECC error detected on the NB.
, cpu_type= generic CPU, cpu= 2, socketid= 0, misc= 0, addr= 6d3d483b, , apicid= 0

and looking at it now, I don't need that "MC%d Error..:" thing either.

All queued for the next version.

---
diff --git a/ras-mce-handler.c b/ras-mce-handler.c
index 2e520d3663ac..ff6f4b373e56 100644
--- a/ras-mce-handler.c
+++ b/ras-mce-handler.c
@@ -23,6 +23,7 @@
 #include <unistd.h>
 #include <stdint.h>
 #include "libtrace/kbuffer.h"
+#include "libtrace/event-utils.h"
 #include "ras-mce-handler.h"
 #include "ras-record.h"
 #include "ras-logger.h"
@@ -185,6 +186,10 @@ static int detect_cpu(struct ras_events *ras)
 	ret = 0;
 
 	if (!strcmp(mce->vendor, "AuthenticAMD")) {
+
+		ret = 0;
+		goto ret;
+
 		if (mce->family == 15)
 			mce->cputype = CPU_K8;
 		if (mce->family > 15) {
@@ -357,8 +362,9 @@ int ras_mce_event_handler(struct trace_seq *s,
 	unsigned long long val;
 	struct ras_events *ras = context;
 	struct mce_priv *mce = ras->mce_priv;
+	const char *decoded_mce;
 	struct mce_event e;
-	int rc = 0;
+	int rc = 0, len;
 
 	memset(&e, 0, sizeof(e));
 
@@ -422,6 +428,10 @@ int ras_mce_event_handler(struct trace_seq *s,
 	if (rc)
 		return rc;
 
+	decoded_mce = pevent_get_field_raw(s, event, "decoded_str", record, &len, 1);
+	if (decoded_mce)
+		strncpy(e.error_msg, decoded_mce, min(len, 4096));
+
 	if (!*e.error_msg && *e.mcastatus_msg)
 		mce_snprintf(e.error_msg, "%s", e.mcastatus_msg);
 
-- 
Regards/Gruss,
    Boris.

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

[toc] | [next] | [standalone]

#1723231

Btw,

how about communicating stuff to the userspace daemon like this?

This'll simplify a lot of detection in userspace.

Thoughts?

---
diff --git a/drivers/ras/debugfs.c b/drivers/ras/debugfs.c
index 501603057dff..62d3da9d256f 100644
--- a/drivers/ras/debugfs.c
+++ b/drivers/ras/debugfs.c
@@ -1,5 +1,7 @@
 #include <linux/debugfs.h>
 
+#include "../../arch/x86/kernel/cpu/mcheck/mce-internal.h"
+
 struct dentry *ras_debugfs_dir;
 
 static atomic_t trace_count = ATOMIC_INIT(0);
@@ -12,7 +14,9 @@ EXPORT_SYMBOL_GPL(ras_userspace_consumers);
 
 static int trace_show(struct seq_file *m, void *v)
 {
-	return atomic_read(&trace_count);
+	seq_printf(m, "readers:%d\n", atomic_read(&trace_count));
+	seq_printf(m, "has decoder:%d\n", mca_cfg.has_decoder);
+	return 0;
 }
 
 static int trace_open(struct inode *inode, struct file *file)

-- 
Regards/Gruss,
    Boris.

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

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

#1723685

On Wed, Aug 30, 2017 at 01:48:43PM +0200, Borislav Petkov wrote:
> Btw,
> 
> how about communicating stuff to the userspace daemon like this?
> 
> This'll simplify a lot of detection in userspace.
> 
> Thoughts?
> 
> ---
> diff --git a/drivers/ras/debugfs.c b/drivers/ras/debugfs.c
> index 501603057dff..62d3da9d256f 100644
> --- a/drivers/ras/debugfs.c
> +++ b/drivers/ras/debugfs.c
> @@ -1,5 +1,7 @@
>  #include <linux/debugfs.h>
>  
> +#include "../../arch/x86/kernel/cpu/mcheck/mce-internal.h"
FWIW I this looks fishy in arch independent code.
I assume this include is for the definition of the mca_cfg global used in the
printf below. 


> +
>  struct dentry *ras_debugfs_dir;
>  
>  static atomic_t trace_count = ATOMIC_INIT(0);
> @@ -12,7 +14,9 @@ EXPORT_SYMBOL_GPL(ras_userspace_consumers);
>  
>  static int trace_show(struct seq_file *m, void *v)
>  {
> -	return atomic_read(&trace_count);
> +	seq_printf(m, "readers:%d\n", atomic_read(&trace_count));
> +	seq_printf(m, "has decoder:%d\n", mca_cfg.has_decoder);

do you want to worry about this debugfs interfaces as ABI?
debugfs changes have bitten me on one specific OS in irritating ways.

I'm not sure what the word is for debugfs interfaces as ABI these days so this
feedback may be not so useful.

--mark

> +	return 0;
>  }
>  
>  static int trace_open(struct inode *inode, struct file *file)
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-edac" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1723699

On Wed, Aug 30, 2017 at 02:47:19PM -0700, mark gross wrote:
> FWIW I this looks fishy in arch independent code. I assume this
> include is for the definition of the mca_cfg global used in the printf
> below.

Yeah, it is a bit ad-hoc. I guess I can add an arch_has_decoder()
wrapper which each arch can define. That would be cleaner.

> do you want to worry about this debugfs interfaces as ABI?
> debugfs changes have bitten me on one specific OS in irritating ways.
> 
> I'm not sure what the word is for debugfs interfaces as ABI these days so this
> feedback may be not so useful.

Yeah, the moment something starts using it, it is ABI. The good thing
about it is, though, that it can get stuff appended to it, just like
tracepoints get fields added. And tools should be able to handle parsing
appended lines fine. Removing lines OTOH is always problematic. But
we'll make that append-only.

Thx!

-- 
Regards/Gruss,
    Boris.

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

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

#1724512

On Thu, Aug 31, 2017 at 12:02:54AM +0200, Borislav Petkov wrote:
> Yeah, it is a bit ad-hoc. I guess I can add an arch_has_decoder()
> wrapper which each arch can define. That would be cleaner.

Something like this, I guess (ontop):

---
diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index 67f951f0c840..b4855edac729 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -388,4 +388,6 @@ static inline int mce_threshold_remove_device(unsigned int cpu) { return 0; };
 
 #endif
 
+#define ras_arch_has_decoder ras_arch_has_decoder
+bool ras_arch_has_decoder(void);
 #endif /* _ASM_X86_MCE_H */
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 3ecc2b2db8f3..6414fa70fd42 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -182,6 +182,12 @@ void mce_unregister_decode_chain(struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(mce_unregister_decode_chain);
 
+bool ras_arch_has_decoder(void)
+{
+	return mca_cfg.has_decoder;
+}
+EXPORT_SYMBOL_GPL(ras_arch_has_decoder);
+
 static inline u32 ctl_reg(int bank)
 {
 	return MSR_IA32_MCx_CTL(bank);
diff --git a/drivers/ras/debugfs.c b/drivers/ras/debugfs.c
index 62d3da9d256f..b368e8ae4738 100644
--- a/drivers/ras/debugfs.c
+++ b/drivers/ras/debugfs.c
@@ -1,6 +1,6 @@
 #include <linux/debugfs.h>
 
-#include "../../arch/x86/kernel/cpu/mcheck/mce-internal.h"
+#include <asm/mce.h>
 
 struct dentry *ras_debugfs_dir;
 
@@ -15,7 +15,7 @@ EXPORT_SYMBOL_GPL(ras_userspace_consumers);
 static int trace_show(struct seq_file *m, void *v)
 {
 	seq_printf(m, "readers:%d\n", atomic_read(&trace_count));
-	seq_printf(m, "has decoder:%d\n", mca_cfg.has_decoder);
+	seq_printf(m, "has decoder:%d\n", ras_arch_has_decoder());
 	return 0;
 }
 
diff --git a/drivers/ras/debugfs.h b/drivers/ras/debugfs.h
index db72e4513191..9c75c4c5b84d 100644
--- a/drivers/ras/debugfs.h
+++ b/drivers/ras/debugfs.h
@@ -5,4 +5,9 @@
 
 extern struct dentry *ras_debugfs_dir;
 
+#ifndef ras_arch_has_decoder
+#define ras_arch_has_decoder ras_arch_has_decoder
+static inline bool ras_arch_has_decoder(void)	{ return false; }
+#endif
+
 #endif /* __RAS_DEBUGFS_H__ */

-- 
Regards/Gruss,
    Boris.

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

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

#1723735

On Wed, 30 Aug 2017 14:47:19 -0700
mark gross <mgross@linux.intel.com> wrote:


> >  struct dentry *ras_debugfs_dir;
> >  
> >  static atomic_t trace_count = ATOMIC_INIT(0);
> > @@ -12,7 +14,9 @@ EXPORT_SYMBOL_GPL(ras_userspace_consumers);
> >  
> >  static int trace_show(struct seq_file *m, void *v)
> >  {
> > -	return atomic_read(&trace_count);
> > +	seq_printf(m, "readers:%d\n", atomic_read(&trace_count));
> > +	seq_printf(m, "has decoder:%d\n", mca_cfg.has_decoder);  
> 
> do you want to worry about this debugfs interfaces as ABI?

It's the old, if a tree falls in the forest issue. If you break the ABI
but nobody is around to notice, did it really break?

> debugfs changes have bitten me on one specific OS in irritating ways.
> 
> I'm not sure what the word is for debugfs interfaces as ABI these days so this
> feedback may be not so useful.

I discussed this with Boris before he sent it out. The current code is
actually broken. The trace_show() looks to be just a stub for a hack to
have userspace tell the kernel it's reading something (the
trace_count). The trace_show() function here returns the trace_count,
which is just wrong. The seq_file show function is suppose to return
less than zero on error, zero on success, or greater than zero if the
show is suppose to skip the current field but not error out. This
trace_show() function doesn't actually show anything. If you cat the
file, it will be blank. Returning zero or greater than zero has the
same effect. Which is nothing.

-- Steve

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

#1725784

On Wed, Aug 30, 2017 at 07:30:58PM -0400, Steven Rostedt wrote:
> On Wed, 30 Aug 2017 14:47:19 -0700
> mark gross <mgross@linux.intel.com> wrote:
> 
> 
> > >  struct dentry *ras_debugfs_dir;
> > >  
> > >  static atomic_t trace_count = ATOMIC_INIT(0);
> > > @@ -12,7 +14,9 @@ EXPORT_SYMBOL_GPL(ras_userspace_consumers);
> > >  
> > >  static int trace_show(struct seq_file *m, void *v)
> > >  {
> > > -	return atomic_read(&trace_count);
> > > +	seq_printf(m, "readers:%d\n", atomic_read(&trace_count));
> > > +	seq_printf(m, "has decoder:%d\n", mca_cfg.has_decoder);  
> > 
> > do you want to worry about this debugfs interfaces as ABI?
> 
> It's the old, if a tree falls in the forest issue. If you break the ABI
> but nobody is around to notice, did it really break?

perhaps, but what I was trying to point out was: "multi line debugFS printf's
like this are very easy to change to append other information and you might
want to worry about the ABI implications that could happen in the future."

--mark
> 
> > debugfs changes have bitten me on one specific OS in irritating ways.
> > 
> > I'm not sure what the word is for debugfs interfaces as ABI these days so this
> > feedback may be not so useful.
> 
> I discussed this with Boris before he sent it out. The current code is
> actually broken. The trace_show() looks to be just a stub for a hack to
> have userspace tell the kernel it's reading something (the
> trace_count). The trace_show() function here returns the trace_count,
> which is just wrong. The seq_file show function is suppose to return
> less than zero on error, zero on success, or greater than zero if the
> show is suppose to skip the current field but not error out. This
> trace_show() function doesn't actually show anything. If you cat the
> file, it will be blank. Returning zero or greater than zero has the
> same effect. Which is nothing.
> 
> -- Steve
> --
> To unsubscribe from this list: send the line "unsubscribe linux-edac" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1725993

On Sun, Sep 03, 2017 at 04:37:04PM -0700, mark gross wrote:
> > It's the old, if a tree falls in the forest issue. If you break the ABI
> > but nobody is around to notice, did it really break?
> 
> perhaps, but what I was trying to point out was: "multi line debugFS printf's
> like this are very easy to change to append other information and you might
> want to worry about the ABI implications that could happen in the future."

Right, as I said to Mark earlier:

"Yeah, the moment something starts using it, it is ABI. The good thing
about it is, though, that it can get stuff appended to it, just like
tracepoints get fields added. And tools should be able to handle parsing
appended lines fine. Removing lines OTOH is always problematic. But
we'll make that append-only."

It is basically the same principle as with tracepoints where we can add
members easily.

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web