LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] tracing: add trace event for memory-failure
@ 2015-03-13 10:10 Xie XiuQi
  2015-03-13 16:37 ` Tony Luck
  2015-03-16  9:27 ` Naoya Horiguchi
  0 siblings, 2 replies; 13+ messages in thread
From: Xie XiuQi @ 2015-03-13 10:10 UTC (permalink / raw)
  To: n-horiguchi, gong.chen, bhelgaas, bp, tony.luck
  Cc: linux-kernel, linux-mm, jingle.chen

Memory-failure as the high level machine check handler, it's necessary
to report memory page recovery action result to user space by ftrace.

This patch add a event at ras group for memory-failure.

The output like below:
# tracer: nop
#
# entries-in-buffer/entries-written: 2/2   #P:24
#
#                              _-----=> irqs-off
#                             / _----=> need-resched
#                            | / _---=> hardirq/softirq
#                            || / _--=> preempt-depth
#                            ||| /     delay
#           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
#              | |       |   ||||       |         |
      mce-inject-13150 [001] ....   277.019359: memory_failure_event: pfn 0x19869: free buddy page recovery: Delayed

Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
---
 include/ras/ras_event.h |   36 ++++++++++++++++++++++++++++++++++++
 mm/memory-failure.c     |    3 +++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 79abb9c..0a6c8f3 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -232,6 +232,42 @@ TRACE_EVENT(aer_event,
 		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
 );
 
+/*
+ * memory-failure recovery action result event
+ *
+ * unsigned long pfn -	Page Number of the corrupted page
+ * char * action -	Recovery action: "free buddy", "free huge", "high
+ *			order kernel", "free buddy, 2nd try", "different
+ *			compound page after locking", "hugepage already
+ *			hardware poisoned", "unmapping failed", "already
+ *			truncated LRU", etc.
+ * char * result -	Action result: Ignored, Failed, Delayed, Recovered.
+ */
+TRACE_EVENT(memory_failure_event,
+	TP_PROTO(const unsigned long pfn,
+		 const char *action,
+		 const char *result),
+
+	TP_ARGS(pfn, action, result),
+
+	TP_STRUCT__entry(
+		__field(unsigned long, pfn)
+		__string(action, action)
+		__string(result, result)
+	),
+
+	TP_fast_assign(
+		__entry->pfn = pfn;
+		__assign_str(action, action);
+		__assign_str(result, result);
+	),
+
+	TP_printk("pfn %#lx: %s page recovery: %s",
+		__entry->pfn,
+		__get_str(action),
+		__get_str(result)
+	)
+);
 #endif /* _TRACE_HW_EVENT_MC_H */
 
 /* This part must be outside protection */
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index d487f8d..86a9cce 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -56,6 +56,7 @@
 #include <linux/mm_inline.h>
 #include <linux/kfifo.h>
 #include "internal.h"
+#include <ras/ras_event.h>
 
 int sysctl_memory_failure_early_kill __read_mostly = 0;
 
@@ -837,6 +838,8 @@ static struct page_state {
  */
 static void action_result(unsigned long pfn, char *msg, int result)
 {
+	trace_memory_failure_event(pfn, msg, action_name[result]);
+
 	pr_err("MCE %#lx: %s page recovery: %s\n",
 		pfn, msg, action_name[result]);
 }
-- 
1.7.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing: add trace event for memory-failure
  2015-03-13 10:10 [PATCH] tracing: add trace event for memory-failure Xie XiuQi
@ 2015-03-13 16:37 ` Tony Luck
  2015-03-13 19:32   ` Steven Rostedt
  2015-03-16  9:27 ` Naoya Horiguchi
  1 sibling, 1 reply; 13+ messages in thread
From: Tony Luck @ 2015-03-13 16:37 UTC (permalink / raw)
  To: Xie XiuQi, Steven Rostedt
  Cc: Naoya Horiguchi, Chen Gong, Bjorn Helgaas, Borislav Petkov,
	Linux Kernel Mailing List, linux-mm, jingle.chen

On Fri, Mar 13, 2015 at 3:10 AM, Xie XiuQi <xiexiuqi@huawei.com> wrote:
> Memory-failure as the high level machine check handler, it's necessary
> to report memory page recovery action result to user space by ftrace.
>
> This patch add a event at ras group for memory-failure.
>
> The output like below:
> # tracer: nop
> #
> # entries-in-buffer/entries-written: 2/2   #P:24
> #
> #                              _-----=> irqs-off
> #                             / _----=> need-resched
> #                            | / _---=> hardirq/softirq
> #                            || / _--=> preempt-depth
> #                            ||| /     delay
> #           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
> #              | |       |   ||||       |         |
>       mce-inject-13150 [001] ....   277.019359: memory_failure_event: pfn 0x19869: free buddy page recovery: Delayed
>
> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
> ---
>  include/ras/ras_event.h |   36 ++++++++++++++++++++++++++++++++++++
>  mm/memory-failure.c     |    3 +++
>  2 files changed, 39 insertions(+), 0 deletions(-)
>
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index 79abb9c..0a6c8f3 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -232,6 +232,42 @@ TRACE_EVENT(aer_event,
>                 __print_flags(__entry->status, "|", aer_uncorrectable_errors))
>  );
>
> +/*
> + * memory-failure recovery action result event
> + *
> + * unsigned long pfn - Page Number of the corrupted page
> + * char * action -     Recovery action: "free buddy", "free huge", "high
> + *                     order kernel", "free buddy, 2nd try", "different
> + *                     compound page after locking", "hugepage already
> + *                     hardware poisoned", "unmapping failed", "already
> + *                     truncated LRU", etc.
> + * char * result -     Action result: Ignored, Failed, Delayed, Recovered.
> + */
> +TRACE_EVENT(memory_failure_event,
> +       TP_PROTO(const unsigned long pfn,
> +                const char *action,
> +                const char *result),
> +
> +       TP_ARGS(pfn, action, result),
> +
> +       TP_STRUCT__entry(
> +               __field(unsigned long, pfn)
> +               __string(action, action)
> +               __string(result, result)
> +       ),
> +
> +       TP_fast_assign(
> +               __entry->pfn = pfn;
> +               __assign_str(action, action);
> +               __assign_str(result, result);
> +       ),
> +
> +       TP_printk("pfn %#lx: %s page recovery: %s",
> +               __entry->pfn,
> +               __get_str(action),
> +               __get_str(result)
> +       )
> +);
>  #endif /* _TRACE_HW_EVENT_MC_H */
>
>  /* This part must be outside protection */
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index d487f8d..86a9cce 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -56,6 +56,7 @@
>  #include <linux/mm_inline.h>
>  #include <linux/kfifo.h>
>  #include "internal.h"
> +#include <ras/ras_event.h>
>
>  int sysctl_memory_failure_early_kill __read_mostly = 0;
>
> @@ -837,6 +838,8 @@ static struct page_state {
>   */
>  static void action_result(unsigned long pfn, char *msg, int result)
>  {
> +       trace_memory_failure_event(pfn, msg, action_name[result]);
> +
>         pr_err("MCE %#lx: %s page recovery: %s\n",
>                 pfn, msg, action_name[result]);
>  }
> --
> 1.7.1
>
> --

Concept looks good to me. Adding Steven Rostedt as we've historically had
challenges adding new trace points in the cleanest way.

-Tony

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing: add trace event for memory-failure
  2015-03-13 16:37 ` Tony Luck
@ 2015-03-13 19:32   ` Steven Rostedt
  2015-03-17 10:47     ` Xie XiuQi
  0 siblings, 1 reply; 13+ messages in thread
From: Steven Rostedt @ 2015-03-13 19:32 UTC (permalink / raw)
  To: Tony Luck
  Cc: Xie XiuQi, Naoya Horiguchi, Chen Gong, Bjorn Helgaas,
	Borislav Petkov, Linux Kernel Mailing List, linux-mm,
	jingle.chen

On Fri, 13 Mar 2015 09:37:34 -0700
Tony Luck <tony.luck@gmail.com> wrote:


> >  int sysctl_memory_failure_early_kill __read_mostly = 0;
> >
> > @@ -837,6 +838,8 @@ static struct page_state {
> >   */
> >  static void action_result(unsigned long pfn, char *msg, int result)
> >  {
> > +       trace_memory_failure_event(pfn, msg, action_name[result]);
> > +
> >         pr_err("MCE %#lx: %s page recovery: %s\n",
> >                 pfn, msg, action_name[result]);
> >  }
> > --
> > 1.7.1
> >
> > --
> 
> Concept looks good to me. Adding Steven Rostedt as we've historically had
> challenges adding new trace points in the cleanest way.

Hehe, thank you :-) I actually do have a recommendation. How about just
passing in "result" and doing:


	TP_printk("pfn %#lx: %s page recovery: %s",
		__entry->pfn,
		__get_str(action),
		__print_symbolic(result, 0, "Ignored",
				1, "Failed",
				2, "Delayed",
				3, "Recovered"))


Now it is hard coded here because trace-cmd and perf do not have a way
to process enums (yet, I need to fix that).

I also need a way to just submit print strings on module load and boot
up such that you only need to pass in the address of the action field
instead of the string. That is also a todo of mine that I may soon
change.

-- Steve



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing: add trace event for memory-failure
  2015-03-13 10:10 [PATCH] tracing: add trace event for memory-failure Xie XiuQi
  2015-03-13 16:37 ` Tony Luck
@ 2015-03-16  9:27 ` Naoya Horiguchi
  2015-03-16 13:04   ` Xie XiuQi
  1 sibling, 1 reply; 13+ messages in thread
From: Naoya Horiguchi @ 2015-03-16  9:27 UTC (permalink / raw)
  To: Xie XiuQi
  Cc: gong.chen, bhelgaas, bp, tony.luck, linux-kernel, linux-mm, jingle.chen

On Fri, Mar 13, 2015 at 06:10:51PM +0800, Xie XiuQi wrote:
> Memory-failure as the high level machine check handler, it's necessary
> to report memory page recovery action result to user space by ftrace.
> 
> This patch add a event at ras group for memory-failure.
> 
> The output like below:
> # tracer: nop
> #
> # entries-in-buffer/entries-written: 2/2   #P:24
> #
> #                              _-----=> irqs-off
> #                             / _----=> need-resched
> #                            | / _---=> hardirq/softirq
> #                            || / _--=> preempt-depth
> #                            ||| /     delay
> #           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
> #              | |       |   ||||       |         |
>       mce-inject-13150 [001] ....   277.019359: memory_failure_event: pfn 0x19869: free buddy page recovery: Delayed
> 
> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
> ---
>  include/ras/ras_event.h |   36 ++++++++++++++++++++++++++++++++++++
>  mm/memory-failure.c     |    3 +++
>  2 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index 79abb9c..0a6c8f3 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -232,6 +232,42 @@ TRACE_EVENT(aer_event,
>  		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
>  );
>  
> +/*
> + * memory-failure recovery action result event
> + *
> + * unsigned long pfn -	Page Number of the corrupted page
> + * char * action -	Recovery action: "free buddy", "free huge", "high
> + *			order kernel", "free buddy, 2nd try", "different
> + *			compound page after locking", "hugepage already
> + *			hardware poisoned", "unmapping failed", "already
> + *			truncated LRU", etc.

Listing all possible values here might be prone to become out of date when
someone try to add/remove/change action_result() call sites.
So I'd like to have some enumerator to bundle these strings in one place
in mm/memory-failure.c.
# I feel like doing this later.

> + * char * result -	Action result: Ignored, Failed, Delayed, Recovered.

mm/memory-failure.c has good explanation:

  /*
   * Error handlers for various types of pages.
   */
  enum outcome {
          IGNORED,        /* Error: cannot be handled */
          FAILED,         /* Error: handling failed */
          DELAYED,        /* Will be handled later */
          RECOVERED,      /* Successfully recovered */
  };

So adding a reference to here looks better to me.

Thanks,
Naoya Horiguchi

> + */
> +TRACE_EVENT(memory_failure_event,
> +	TP_PROTO(const unsigned long pfn,
> +		 const char *action,
> +		 const char *result),
> +
> +	TP_ARGS(pfn, action, result),
> +
> +	TP_STRUCT__entry(
> +		__field(unsigned long, pfn)
> +		__string(action, action)
> +		__string(result, result)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->pfn = pfn;
> +		__assign_str(action, action);
> +		__assign_str(result, result);
> +	),
> +
> +	TP_printk("pfn %#lx: %s page recovery: %s",
> +		__entry->pfn,
> +		__get_str(action),
> +		__get_str(result)
> +	)
> +);
>  #endif /* _TRACE_HW_EVENT_MC_H */
>  
>  /* This part must be outside protection */
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index d487f8d..86a9cce 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -56,6 +56,7 @@
>  #include <linux/mm_inline.h>
>  #include <linux/kfifo.h>
>  #include "internal.h"
> +#include <ras/ras_event.h>
>  
>  int sysctl_memory_failure_early_kill __read_mostly = 0;
>  
> @@ -837,6 +838,8 @@ static struct page_state {
>   */
>  static void action_result(unsigned long pfn, char *msg, int result)
>  {
> +	trace_memory_failure_event(pfn, msg, action_name[result]);
> +
>  	pr_err("MCE %#lx: %s page recovery: %s\n",
>  		pfn, msg, action_name[result]);
>  }
> -- 
> 1.7.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing: add trace event for memory-failure
  2015-03-16  9:27 ` Naoya Horiguchi
@ 2015-03-16 13:04   ` Xie XiuQi
  0 siblings, 0 replies; 13+ messages in thread
From: Xie XiuQi @ 2015-03-16 13:04 UTC (permalink / raw)
  To: Naoya Horiguchi
  Cc: gong.chen, bhelgaas, bp, tony.luck, linux-kernel, linux-mm, jingle.chen

On 2015/3/16 17:27, Naoya Horiguchi wrote:
> On Fri, Mar 13, 2015 at 06:10:51PM +0800, Xie XiuQi wrote:
>> Memory-failure as the high level machine check handler, it's necessary
>> to report memory page recovery action result to user space by ftrace.
>>
>> This patch add a event at ras group for memory-failure.
>>
>> The output like below:
>> # tracer: nop
>> #
>> # entries-in-buffer/entries-written: 2/2   #P:24
>> #
>> #                              _-----=> irqs-off
>> #                             / _----=> need-resched
>> #                            | / _---=> hardirq/softirq
>> #                            || / _--=> preempt-depth
>> #                            ||| /     delay
>> #           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
>> #              | |       |   ||||       |         |
>>       mce-inject-13150 [001] ....   277.019359: memory_failure_event: pfn 0x19869: free buddy page recovery: Delayed
>>
>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>> ---
>>  include/ras/ras_event.h |   36 ++++++++++++++++++++++++++++++++++++
>>  mm/memory-failure.c     |    3 +++
>>  2 files changed, 39 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
>> index 79abb9c..0a6c8f3 100644
>> --- a/include/ras/ras_event.h
>> +++ b/include/ras/ras_event.h
>> @@ -232,6 +232,42 @@ TRACE_EVENT(aer_event,
>>  		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
>>  );
>>  
>> +/*
>> + * memory-failure recovery action result event
>> + *
>> + * unsigned long pfn -	Page Number of the corrupted page
>> + * char * action -	Recovery action: "free buddy", "free huge", "high
>> + *			order kernel", "free buddy, 2nd try", "different
>> + *			compound page after locking", "hugepage already
>> + *			hardware poisoned", "unmapping failed", "already
>> + *			truncated LRU", etc.
> 
> Listing all possible values here might be prone to become out of date when
> someone try to add/remove/change action_result() call sites.
> So I'd like to have some enumerator to bundle these strings in one place
> in mm/memory-failure.c.
> # I feel like doing this later.

Good, we need this.

> 
>> + * char * result -	Action result: Ignored, Failed, Delayed, Recovered.
> 
> mm/memory-failure.c has good explanation:
> 
>   /*
>    * Error handlers for various types of pages.
>    */
>   enum outcome {
>           IGNORED,        /* Error: cannot be handled */
>           FAILED,         /* Error: handling failed */
>           DELAYED,        /* Will be handled later */
>           RECOVERED,      /* Successfully recovered */
>   };
> 
> So adding a reference to here looks better to me.

Thanks for you comments. I'll change it.

> 
> Thanks,
> Naoya Horiguchi
> 
>> + */
>> +TRACE_EVENT(memory_failure_event,
>> +	TP_PROTO(const unsigned long pfn,
>> +		 const char *action,
>> +		 const char *result),
>> +
>> +	TP_ARGS(pfn, action, result),
>> +
>> +	TP_STRUCT__entry(
>> +		__field(unsigned long, pfn)
>> +		__string(action, action)
>> +		__string(result, result)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__entry->pfn = pfn;
>> +		__assign_str(action, action);
>> +		__assign_str(result, result);
>> +	),
>> +
>> +	TP_printk("pfn %#lx: %s page recovery: %s",
>> +		__entry->pfn,
>> +		__get_str(action),
>> +		__get_str(result)
>> +	)
>> +);
>>  #endif /* _TRACE_HW_EVENT_MC_H */
>>  
>>  /* This part must be outside protection */
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index d487f8d..86a9cce 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -56,6 +56,7 @@
>>  #include <linux/mm_inline.h>
>>  #include <linux/kfifo.h>
>>  #include "internal.h"
>> +#include <ras/ras_event.h>
>>  
>>  int sysctl_memory_failure_early_kill __read_mostly = 0;
>>  
>> @@ -837,6 +838,8 @@ static struct page_state {
>>   */
>>  static void action_result(unsigned long pfn, char *msg, int result)
>>  {
>> +	trace_memory_failure_event(pfn, msg, action_name[result]);
>> +
>>  	pr_err("MCE %#lx: %s page recovery: %s\n",
>>  		pfn, msg, action_name[result]);
>>  }
>> -- 
>> 1.7.1
>>
> .
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing: add trace event for memory-failure
  2015-03-13 19:32   ` Steven Rostedt
@ 2015-03-17 10:47     ` Xie XiuQi
  2015-03-18  0:55       ` Steven Rostedt
  0 siblings, 1 reply; 13+ messages in thread
From: Xie XiuQi @ 2015-03-17 10:47 UTC (permalink / raw)
  To: Steven Rostedt, Tony Luck
  Cc: Naoya Horiguchi, Chen Gong, Bjorn Helgaas, Borislav Petkov,
	Linux Kernel Mailing List, linux-mm, jingle.chen

On 2015/3/14 3:32, Steven Rostedt wrote:
> On Fri, 13 Mar 2015 09:37:34 -0700
> Tony Luck <tony.luck@gmail.com> wrote:
> 
> 
>>>  int sysctl_memory_failure_early_kill __read_mostly = 0;
>>>
>>> @@ -837,6 +838,8 @@ static struct page_state {
>>>   */
>>>  static void action_result(unsigned long pfn, char *msg, int result)
>>>  {
>>> +       trace_memory_failure_event(pfn, msg, action_name[result]);
>>> +
>>>         pr_err("MCE %#lx: %s page recovery: %s\n",
>>>                 pfn, msg, action_name[result]);
>>>  }
>>> --
>>> 1.7.1
>>>
>>> --
>>
>> Concept looks good to me. Adding Steven Rostedt as we've historically had
>> challenges adding new trace points in the cleanest way.
> 
> Hehe, thank you :-) I actually do have a recommendation. How about just
> passing in "result" and doing:
> 
> 
> 	TP_printk("pfn %#lx: %s page recovery: %s",
> 		__entry->pfn,
> 		__get_str(action),
> 		__print_symbolic(result, 0, "Ignored",
> 				1, "Failed",
> 				2, "Delayed",
> 				3, "Recovered"))
> 
> 
> Now it is hard coded here because trace-cmd and perf do not have a way
> to process enums (yet, I need to fix that).

Hi Steve,

Thanks for you comments.

I'm not clearly why we need a hard coded here. As the strings or "result" have
defined in mm/memory-failure.c, so passing "action_name[result]" would be more
clean and more flexible here?

Thanks,
	Xie XiuQi

> 
> I also need a way to just submit print strings on module load and boot
> up such that you only need to pass in the address of the action field
> instead of the string. That is also a todo of mine that I may soon
> change.
> 
> -- Steve
> 
> 
> 
> .
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing: add trace event for memory-failure
  2015-03-17 10:47     ` Xie XiuQi
@ 2015-03-18  0:55       ` Steven Rostedt
  0 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2015-03-18  0:55 UTC (permalink / raw)
  To: Xie XiuQi
  Cc: Tony Luck, Naoya Horiguchi, Chen Gong, Bjorn Helgaas,
	Borislav Petkov, Linux Kernel Mailing List, linux-mm,
	jingle.chen

On Tue, 17 Mar 2015 18:47:40 +0800
Xie XiuQi <xiexiuqi@huawei.com> wrote:

> I'm not clearly why we need a hard coded here. As the strings or "result" have
> defined in mm/memory-failure.c, so passing "action_name[result]" would be more
> clean and more flexible here?

The TP_printk() is what will be shown in the print format of the event
"format" file, and is what trace-cmd and perf use to parse the data and
know what to print. If you use "action_name[result]" that will be what
the user space tools see, and will have no idea what to do with
"action_name[result]". The hard coded output is a bit more explicit in
how to interpret the raw data.

Another way around this is to create a "plugin" that can be loaded and
will override the TP_printk() parsing.

-- Steve

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing: add trace event for memory-failure
  2015-03-20 17:24     ` Luck, Tony
@ 2015-03-21  5:44       ` Xie XiuQi
  0 siblings, 0 replies; 13+ messages in thread
From: Xie XiuQi @ 2015-03-21  5:44 UTC (permalink / raw)
  To: Luck, Tony, Borislav Petkov
  Cc: n-horiguchi, gong.chen, bhelgaas, rostedt, linux-kernel,
	linux-mm, jingle.chen

On 2015/3/21 1:24, Luck, Tony wrote:
>> RAS user space tools like rasdaemon which base on trace event, could
>> receive mce error event, but no memory recovery result event. So, I
>> want to add this event to make this scenario complete.
> 
> Excellent answer.  Are you going to write that patch for rasdaemon?

Yes, I will ;-)

> 
> -Tony
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] tracing: add trace event for memory-failure
  2015-03-20  4:15   ` Xie XiuQi
@ 2015-03-20 17:24     ` Luck, Tony
  2015-03-21  5:44       ` Xie XiuQi
  0 siblings, 1 reply; 13+ messages in thread
From: Luck, Tony @ 2015-03-20 17:24 UTC (permalink / raw)
  To: Xie XiuQi, Borislav Petkov
  Cc: n-horiguchi, gong.chen, bhelgaas, rostedt, linux-kernel,
	linux-mm, jingle.chen

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 434 bytes --]

> RAS user space tools like rasdaemon which base on trace event, could
> receive mce error event, but no memory recovery result event. So, I
> want to add this event to make this scenario complete.

Excellent answer.  Are you going to write that patch for rasdaemon?

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing: add trace event for memory-failure
  2015-03-19 10:39 ` Borislav Petkov
@ 2015-03-20  4:15   ` Xie XiuQi
  2015-03-20 17:24     ` Luck, Tony
  0 siblings, 1 reply; 13+ messages in thread
From: Xie XiuQi @ 2015-03-20  4:15 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: n-horiguchi, gong.chen, bhelgaas, tony.luck, rostedt,
	linux-kernel, linux-mm, jingle.chen

On 2015/3/19 18:39, Borislav Petkov wrote:
> On Thu, Mar 19, 2015 at 11:04:30AM +0800, Xie XiuQi wrote:
>> Memory-failure as the high level machine check handler, it's necessary
>> to report memory page recovery action result to user space by ftrace.
>>
>> This patch add a event at ras group for memory-failure.
>>
>> The output like below:
>> #  tracer: nop
>> # 
>> #  entries-in-buffer/entries-written: 2/2   #P:24
>> # 
>> #                               _-----=> irqs-off
>> #                              / _----=> need-resched
>> #                             | / _---=> hardirq/softirq
>> #                             || / _--=> preempt-depth
>> #                             ||| /     delay
>> #            TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
>> #               | |       |   ||||       |         |
>>        mce-inject-13150 [001] ....   277.019359: memory_failure_event: pfn 0x19869: free buddy page recovery: Delayed
>>
>> ---
>> v1->v2:
>>  - Comment update
>>  - Just passing 'result' instead of 'action_name[result]',
>>    suggested by Steve. And hard coded there because trace-cmd
>>    and perf do not have a way to process enums.
>>
>> Cc: Tony Luck <tony.luck@intel.com>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
>> ---
>>  include/ras/ras_event.h | 38 ++++++++++++++++++++++++++++++++++++++
>>  mm/memory-failure.c     |  3 +++
>>  2 files changed, 41 insertions(+)
>>
>> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
>> index 79abb9c..ebb05f3 100644
>> --- a/include/ras/ras_event.h
>> +++ b/include/ras/ras_event.h
>> @@ -232,6 +232,44 @@ TRACE_EVENT(aer_event,
>>  		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
>>  );
>>  
>> +/*
>> + * memory-failure recovery action result event
>> + *
>> + * unsigned long pfn -	Page Number of the corrupted page
>> + * char * action -	Recovery action for various type of pages
>> + * int result	 -	Action result
>> + *
>> + * NOTE: 'action' and 'result' are defined at mm/memory-failure.c
>> + */
>> +TRACE_EVENT(memory_failure_event,
> 
> What is the real reason for adding this TP? Real-life use cases please.
> Add those to the commit message too.
> 
> "Just because" is not a proper justification.

RAS user space tools like rasdaemon which base on trace event, could
receive mce error event, but no memory recovery result event. So, I
want to add this event to make this scenario complete.

I'll add it to commit message, thanks.

> 
>> +	TP_PROTO(const unsigned long pfn,
>> +		 const char *action,
>> +		 const int result),
>> +
>> +	TP_ARGS(pfn, action, result),
>> +
>> +	TP_STRUCT__entry(
>> +		__field(unsigned long, pfn)
>> +		__string(action, action)
>> +		__field(int, result)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__entry->pfn	= pfn;
>> +		__assign_str(action, action);
>> +		__entry->result	= result;
>> +	),
>> +
>> +	TP_printk("pfn %#lx: %s page recovery: %s",
>> +		__entry->pfn,
>> +		__get_str(action),
>> +		__print_symbolic(__entry->result,
>> +				{0, "Ignored"},
>> +				{1, "Failed"},
>> +				{2, "Delayed"},
>> +				{3, "Recovered"})
> 
> If you're going to do this, please add a comment above it like this:
> 
> /*
>  * Keep those in sync with static const char *action_name[] in
>  * mm/memory-failure.c
>  */

Thanks. I will ;-)

> 
> Thanks.
> 



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing: add trace event for memory-failure
  2015-03-19  3:04 Xie XiuQi
  2015-03-19  3:22 ` Steven Rostedt
@ 2015-03-19 10:39 ` Borislav Petkov
  2015-03-20  4:15   ` Xie XiuQi
  1 sibling, 1 reply; 13+ messages in thread
From: Borislav Petkov @ 2015-03-19 10:39 UTC (permalink / raw)
  To: Xie XiuQi
  Cc: n-horiguchi, gong.chen, bhelgaas, tony.luck, rostedt,
	linux-kernel, linux-mm, jingle.chen

On Thu, Mar 19, 2015 at 11:04:30AM +0800, Xie XiuQi wrote:
> Memory-failure as the high level machine check handler, it's necessary
> to report memory page recovery action result to user space by ftrace.
> 
> This patch add a event at ras group for memory-failure.
> 
> The output like below:
> #  tracer: nop
> # 
> #  entries-in-buffer/entries-written: 2/2   #P:24
> # 
> #                               _-----=> irqs-off
> #                              / _----=> need-resched
> #                             | / _---=> hardirq/softirq
> #                             || / _--=> preempt-depth
> #                             ||| /     delay
> #            TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
> #               | |       |   ||||       |         |
>        mce-inject-13150 [001] ....   277.019359: memory_failure_event: pfn 0x19869: free buddy page recovery: Delayed
> 
> ---
> v1->v2:
>  - Comment update
>  - Just passing 'result' instead of 'action_name[result]',
>    suggested by Steve. And hard coded there because trace-cmd
>    and perf do not have a way to process enums.
> 
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
> ---
>  include/ras/ras_event.h | 38 ++++++++++++++++++++++++++++++++++++++
>  mm/memory-failure.c     |  3 +++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index 79abb9c..ebb05f3 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -232,6 +232,44 @@ TRACE_EVENT(aer_event,
>  		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
>  );
>  
> +/*
> + * memory-failure recovery action result event
> + *
> + * unsigned long pfn -	Page Number of the corrupted page
> + * char * action -	Recovery action for various type of pages
> + * int result	 -	Action result
> + *
> + * NOTE: 'action' and 'result' are defined at mm/memory-failure.c
> + */
> +TRACE_EVENT(memory_failure_event,

What is the real reason for adding this TP? Real-life use cases please.
Add those to the commit message too.

"Just because" is not a proper justification.

> +	TP_PROTO(const unsigned long pfn,
> +		 const char *action,
> +		 const int result),
> +
> +	TP_ARGS(pfn, action, result),
> +
> +	TP_STRUCT__entry(
> +		__field(unsigned long, pfn)
> +		__string(action, action)
> +		__field(int, result)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->pfn	= pfn;
> +		__assign_str(action, action);
> +		__entry->result	= result;
> +	),
> +
> +	TP_printk("pfn %#lx: %s page recovery: %s",
> +		__entry->pfn,
> +		__get_str(action),
> +		__print_symbolic(__entry->result,
> +				{0, "Ignored"},
> +				{1, "Failed"},
> +				{2, "Delayed"},
> +				{3, "Recovered"})

If you're going to do this, please add a comment above it like this:

/*
 * Keep those in sync with static const char *action_name[] in
 * mm/memory-failure.c
 */

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] tracing: add trace event for memory-failure
  2015-03-19  3:04 Xie XiuQi
@ 2015-03-19  3:22 ` Steven Rostedt
  2015-03-19 10:39 ` Borislav Petkov
  1 sibling, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2015-03-19  3:22 UTC (permalink / raw)
  To: Xie XiuQi
  Cc: n-horiguchi, gong.chen, bhelgaas, bp, tony.luck, linux-kernel,
	linux-mm, jingle.chen

On Thu, 19 Mar 2015 11:04:30 +0800
Xie XiuQi <xiexiuqi@huawei.com> wrote:

> Memory-failure as the high level machine check handler, it's necessary
> to report memory page recovery action result to user space by ftrace.
> 
> This patch add a event at ras group for memory-failure.
> 
> The output like below:
> #  tracer: nop
> # 
> #  entries-in-buffer/entries-written: 2/2   #P:24
> # 
> #                               _-----=> irqs-off
> #                              / _----=> need-resched
> #                             | / _---=> hardirq/softirq
> #                             || / _--=> preempt-depth
> #                             ||| /     delay
> #            TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
> #               | |       |   ||||       |         |
>        mce-inject-13150 [001] ....   277.019359: memory_failure_event: pfn 0x19869: free buddy page recovery: Delayed
> 
> ---
> v1->v2:
>  - Comment update
>  - Just passing 'result' instead of 'action_name[result]',
>    suggested by Steve. And hard coded there because trace-cmd
>    and perf do not have a way to process enums.
> 

I'll try to fix that issue soon, such that enums will work.

> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
> ---
>  include/ras/ras_event.h | 38 ++++++++++++++++++++++++++++++++++++++
>  mm/memory-failure.c     |  3 +++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index 79abb9c..ebb05f3 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -232,6 +232,44 @@ TRACE_EVENT(aer_event,
>  		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
>  );
>  
> +/*
> + * memory-failure recovery action result event
> + *
> + * unsigned long pfn -	Page Number of the corrupted page
> + * char * action -	Recovery action for various type of pages
> + * int result	 -	Action result
> + *
> + * NOTE: 'action' and 'result' are defined at mm/memory-failure.c
> + */
> +TRACE_EVENT(memory_failure_event,
> +	TP_PROTO(const unsigned long pfn,
> +		 const char *action,
> +		 const int result),

"const unsigned long" and "const int" is that really needed? These are
passed by value parameters. There's no need to make them const. The
"const char *" is required though.

-- Steve

> +
> +	TP_ARGS(pfn, action, result),
> +
> +	TP_STRUCT__entry(
> +		__field(unsigned long, pfn)
> +		__string(action, action)
> +		__field(int, result)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->pfn	= pfn;
> +		__assign_str(action, action);
> +		__entry->result	= result;
> +	),
> +
> +	TP_printk("pfn %#lx: %s page recovery: %s",
> +		__entry->pfn,
> +		__get_str(action),
> +		__print_symbolic(__entry->result,
> +				{0, "Ignored"},
> +				{1, "Failed"},
> +				{2, "Delayed"},
> +				{3, "Recovered"})
> +	)
> +);
>  #endif /* _TRACE_HW_EVENT_MC_H */
>  
>  /* This part must be outside protection */
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index feb803b..3a71668 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -56,6 +56,7 @@
>  #include <linux/mm_inline.h>
>  #include <linux/kfifo.h>
>  #include "internal.h"
> +#include <ras/ras_event.h>
>  
>  int sysctl_memory_failure_early_kill __read_mostly = 0;
>  
> @@ -844,6 +845,8 @@ static struct page_state {
>   */
>  static void action_result(unsigned long pfn, char *msg, int result)
>  {
> +	trace_memory_failure_event(pfn, msg, result);
> +
>  	pr_err("MCE %#lx: %s page recovery: %s\n",
>  		pfn, msg, action_name[result]);
>  }


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] tracing: add trace event for memory-failure
@ 2015-03-19  3:04 Xie XiuQi
  2015-03-19  3:22 ` Steven Rostedt
  2015-03-19 10:39 ` Borislav Petkov
  0 siblings, 2 replies; 13+ messages in thread
From: Xie XiuQi @ 2015-03-19  3:04 UTC (permalink / raw)
  To: n-horiguchi
  Cc: gong.chen, bhelgaas, bp, tony.luck, rostedt, linux-kernel,
	linux-mm, jingle.chen

Memory-failure as the high level machine check handler, it's necessary
to report memory page recovery action result to user space by ftrace.

This patch add a event at ras group for memory-failure.

The output like below:
#  tracer: nop
# 
#  entries-in-buffer/entries-written: 2/2   #P:24
# 
#                               _-----=> irqs-off
#                              / _----=> need-resched
#                             | / _---=> hardirq/softirq
#                             || / _--=> preempt-depth
#                             ||| /     delay
#            TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
#               | |       |   ||||       |         |
       mce-inject-13150 [001] ....   277.019359: memory_failure_event: pfn 0x19869: free buddy page recovery: Delayed

---
v1->v2:
 - Comment update
 - Just passing 'result' instead of 'action_name[result]',
   suggested by Steve. And hard coded there because trace-cmd
   and perf do not have a way to process enums.

Cc: Tony Luck <tony.luck@intel.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Xie XiuQi <xiexiuqi@huawei.com>
---
 include/ras/ras_event.h | 38 ++++++++++++++++++++++++++++++++++++++
 mm/memory-failure.c     |  3 +++
 2 files changed, 41 insertions(+)

diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 79abb9c..ebb05f3 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -232,6 +232,44 @@ TRACE_EVENT(aer_event,
 		__print_flags(__entry->status, "|", aer_uncorrectable_errors))
 );
 
+/*
+ * memory-failure recovery action result event
+ *
+ * unsigned long pfn -	Page Number of the corrupted page
+ * char * action -	Recovery action for various type of pages
+ * int result	 -	Action result
+ *
+ * NOTE: 'action' and 'result' are defined at mm/memory-failure.c
+ */
+TRACE_EVENT(memory_failure_event,
+	TP_PROTO(const unsigned long pfn,
+		 const char *action,
+		 const int result),
+
+	TP_ARGS(pfn, action, result),
+
+	TP_STRUCT__entry(
+		__field(unsigned long, pfn)
+		__string(action, action)
+		__field(int, result)
+	),
+
+	TP_fast_assign(
+		__entry->pfn	= pfn;
+		__assign_str(action, action);
+		__entry->result	= result;
+	),
+
+	TP_printk("pfn %#lx: %s page recovery: %s",
+		__entry->pfn,
+		__get_str(action),
+		__print_symbolic(__entry->result,
+				{0, "Ignored"},
+				{1, "Failed"},
+				{2, "Delayed"},
+				{3, "Recovered"})
+	)
+);
 #endif /* _TRACE_HW_EVENT_MC_H */
 
 /* This part must be outside protection */
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index feb803b..3a71668 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -56,6 +56,7 @@
 #include <linux/mm_inline.h>
 #include <linux/kfifo.h>
 #include "internal.h"
+#include <ras/ras_event.h>
 
 int sysctl_memory_failure_early_kill __read_mostly = 0;
 
@@ -844,6 +845,8 @@ static struct page_state {
  */
 static void action_result(unsigned long pfn, char *msg, int result)
 {
+	trace_memory_failure_event(pfn, msg, result);
+
 	pr_err("MCE %#lx: %s page recovery: %s\n",
 		pfn, msg, action_name[result]);
 }
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-03-21  5:50 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-13 10:10 [PATCH] tracing: add trace event for memory-failure Xie XiuQi
2015-03-13 16:37 ` Tony Luck
2015-03-13 19:32   ` Steven Rostedt
2015-03-17 10:47     ` Xie XiuQi
2015-03-18  0:55       ` Steven Rostedt
2015-03-16  9:27 ` Naoya Horiguchi
2015-03-16 13:04   ` Xie XiuQi
2015-03-19  3:04 Xie XiuQi
2015-03-19  3:22 ` Steven Rostedt
2015-03-19 10:39 ` Borislav Petkov
2015-03-20  4:15   ` Xie XiuQi
2015-03-20 17:24     ` Luck, Tony
2015-03-21  5:44       ` Xie XiuQi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).