lkml.org 
[lkml]   [2012]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v22] edac, ras/hw_event.h: use events to handle hw issues
Em 10-05-2012 19:37, Luck, Tony escreveu:
> kworker/u:6-201 [007] .N.. 186.197280: mc_error: [Hardware Error]: mem_ctl#0: Corrected error memory read error on memory stick "DIMM_A1" (channel:0 slot:1 page:0x2f1eb3 offset:0x446 grain:32 syndrome:0x0 1 error(s): Unknown: Err=0001:0090 socket=0 channel=0/mask=1 rank=5)
>
> The word "error" appears *five* times on this line (once with a capital E).
> I feel beaten, bruised and ready to give up on this machine with just one
> actual error reported :-)

:)

Several of them come from the driver-provided details.

The edac-mc core contributes with "mc_error", "[Hardware Error]" and "Corrected error".
The sb-edac driver contributes with "memory read error" and "1 error(s)".

We can get easily get rid of "[Hardware Error]" by removing HW_ERR from:

TP_printk(HW_ERR "mem_ctl#%d: %s error %s on memory stick \"%s\" (%s %s %s)",

replacing mc_error by something else is not hard, but this is the name of the trace call:

TRACE_EVENT(mc_error,
...

Maybe the better is to do s/mc_error/mc_event/g.

The error count msg ("1 error(s)") could be replaced by "count:1".

>
> We could get rid of one by:
> s/Corrected error memory read error/Corrected memory read error/

This is the hardest possible solution ;) Changing it will cause weird messages
all over EDAC drivers ;)

This is how sb_edac.c provides the "memory read error" string:

switch (optypenum) {
case 0:
optype = "generic undef request error";
break;
case 1:
optype = "memory read error";
break;
case 2:
optype = "memory write error";
break;
case 3:
optype = "addr/cmd error";
break;
case 4:
optype = "memory scrubbing error";
break;
default:
optype = "reserved";
break;

In the last case of switch, for this driver, the error would be printed as "Corrected reserved".

On i7core_edac, there's also one error that would be weird:

switch (optypenum) {
case 0:
optype = "generic undef request";
break;

Drivers like i5100_edac also provide error messages without the word "error" on it, like:

static const char *i5100_err_msg(unsigned err)
{
static const char *merrs[] = {
"unknown", /* 0 */
"uncorrectable data ECC on replay", /* 1 */
"unknown", /* 2 */
"unknown", /* 3 */
"aliased uncorrectable demand data ECC", /* 4 */
"aliased uncorrectable spare-copy data ECC", /* 5 */
"aliased uncorrectable patrol data ECC", /* 6 */
"unknown", /* 7 */
"unknown", /* 8 */
"unknown", /* 9 */
"non-aliased uncorrectable demand data ECC", /* 10 */
"non-aliased uncorrectable spare-copy data ECC", /* 11 */
"non-aliased uncorrectable patrol data ECC", /* 12 */
...

On _several_ drivers, the error type is simply the name of the driver, or blank:

amd76x_edac.c:
edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
mci->csrows[row]->first_page, 0, 0,
row, 0, -1,
mci->ctl_name, "", NULL);

i3200_edac:
edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci,
0, 0, 0,
eccerrlog_row(channel, log),
-1, -1,
"i3000 UE", "", NULL);

Btw, you should not forget that, while simple usecases will be to just read the
/sys/kernel/debug/tracing/trace file, a monitoring tool will use the
binary data information, and store each trace field on a separate database
field.

So, the contents of the error message field should be consistent.

>
> (though we'd need to see if things still read well for all other "msg" options.
>
> Or perhaps it could say:
> ... Corrected error: memory read on memory stick ...
> or even:
> ... Corrected error: read on memory stick ...
>
> This part could get shortened too:
> mc_error: [Hardware Error]:
> will mc_error ever report something that isn't a "Hardware Error"?
> I don't think we have to preserve this legacy string when moving
> to a new reporting mechanism.
>
>
>> There are still some space to improve the fields provided by the drivers.
> Apart from reporting "channel" twice, that doesn't look too bad. Maybe
> the "1 error(s)" could say "count: 1"?

Agreed.

See the enclosed patch. The TP_printk() message after it is:

mc_event: Corrected error:memory read error on memory stick "DIMM_A1" (mc:0 channel:0 slot:0 page:0x1a3706 offset:0xff1 grain:32 syndrome:0x0 count:1 area:DRAM err_code:0001:0090 socket:0 channel_mask:8 rank:0)

If ok, I'll merge the edac core part together with this changeset, and the
sb_edac part together with the patch that cleans the sb_edac logs.

Regards,
Mauro

--

edac: Improve error messages on sb-edac and edac-mc

From: Mauro Carvalho Chehab <mchehab@redhat.com>

After this patch, /sys/kernel/debug/tracing/trace displays:

kworker/u:6-201 [007] .N.. 161.136624: mc_event: Corrected error:memory read error on memory stick "DIMM_A1" (mc:0 channel:0 slot:0 page:0x586b6e offset:0xa66 grain:32 syndrome:0x0 count:1 area:DRAM err_code:0001:0090 socket:0 channel_mask:1 rank:1)
kworker/u:6-201 [007] .N.. 161.155708: mc_event: Corrected error:memory read error on memory stick "DIMM_E1" (mc:1 channel:0 slot:0 page:0x987f45 offset:0x14c grain:32 syndrome:0x0 count:1 area:DRAM err_code:0001:0090 socket:1 channel_mask:4 rank:0)
kworker/u:6-201 [007] .N.. 161.174817: mc_event: Corrected error:memory read error on memory stick "DIMM_C1" (mc:0 channel:0 slot:1 page:0x2bf618 offset:0xb2e grain:32 syndrome:0x0 count:1 area:DRAM err_code:0001:0090 socket:0 channel_mask:4 rank:4)

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 0550cb4..b7492e8 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -35,7 +35,7 @@

#define CREATE_TRACE_POINTS
#define TRACE_INCLUDE_PATH ../../include/ras
-#include <ras/ras.h>
+#include <ras/ras_event.h>

/* lock to memory controller's control array */
static DEFINE_MUTEX(mem_ctls_mutex);
@@ -1174,7 +1174,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
page_frame_number, offset_in_page, grain);

/* Report the error via the trace interface */
- trace_mc_error(type, mci->mc_idx, msg, label, location,
+ trace_mc_event(type, mci->mc_idx, msg, label, location,
detail, other_detail);

/* Report the error via the edac_mc_printk() interface */
diff --git a/drivers/edac/sb_edac.c b/drivers/edac/sb_edac.c
index 69c807c..60dbefe 100644
--- a/drivers/edac/sb_edac.c
+++ b/drivers/edac/sb_edac.c
@@ -786,7 +786,7 @@ static int get_memory_error_data(struct mem_ctl_info *mci,
u8 *socket,
long *channel_mask,
u8 *rank,
- char *area_type, char *msg)
+ char **area_type, char *msg)
{
struct mem_ctl_info *new_mci;
struct sbridge_pvt *pvt = mci->pvt_info;
@@ -841,7 +841,7 @@ static int get_memory_error_data(struct mem_ctl_info *mci,
sprintf(msg, "Can't discover the memory socket");
return -EINVAL;
}
- area_type = get_dram_attr(reg);
+ *area_type = get_dram_attr(reg);
interleave_mode = INTERLEAVE_MODE(reg);

pci_read_config_dword(pvt->pci_sad0, interleave_list[n_sads],
@@ -1339,7 +1339,7 @@ static void sbridge_mce_output_error(struct mem_ctl_info *mci,
struct mem_ctl_info *new_mci;
struct sbridge_pvt *pvt = mci->pvt_info;
enum hw_event_mc_err_type tp_event;
- char *type, *optype, msg[256], *recoverable_msg;
+ char *type, *optype, msg[256];
bool ripv = GET_BITFIELD(m->mcgstatus, 0, 0);
bool overflow = GET_BITFIELD(m->status, 62, 62);
bool uncorrected_error = GET_BITFIELD(m->status, 61, 61);
@@ -1352,7 +1352,7 @@ static void sbridge_mce_output_error(struct mem_ctl_info *mci,
long channel_mask, first_channel;
u8 rank, socket;
int rc, dimm;
- char *area_type = "Unknown";
+ char *area_type = NULL;

if (uncorrected_error) {
if (ripv) {
@@ -1404,7 +1404,7 @@ static void sbridge_mce_output_error(struct mem_ctl_info *mci,
}

rc = get_memory_error_data(mci, m->addr, &socket,
- &channel_mask, &rank, area_type, msg);
+ &channel_mask, &rank, &area_type, msg);
if (rc < 0)
goto err_parsing;
new_mci = get_mci_for_node_id(socket);
@@ -1424,10 +1424,6 @@ static void sbridge_mce_output_error(struct mem_ctl_info *mci,
else
dimm = 2;

- if (uncorrected_error && recoverable)
- recoverable_msg = " recoverable";
- else
- recoverable_msg = "";

/*
* FIXME: On some memory configurations (mirror, lockstep), the
@@ -1436,14 +1432,13 @@ static void sbridge_mce_output_error(struct mem_ctl_info *mci,
* to the group of dimm's where the error may be happening.
*/
snprintf(msg, sizeof(msg),
- "%d error(s)%s: %s%s: Err=%04x:%04x socket=%d channel=%ld/mask=%ld rank=%d",
+ "count:%d%s%s area:%s err_code:%04x:%04x socket:%d channel_mask:%ld rank:%d",
core_err_cnt,
overflow ? " OVERFLOW" : "",
+ (uncorrected_error && recoverable) ? " recoverable" : "",
area_type,
- recoverable_msg,
mscod, errcode,
socket,
- first_channel,
channel_mask,
rank);

diff --git a/include/ras/ras.h b/include/ras/ras_event.h
similarity index 80%
rename from include/ras/ras.h
rename to include/ras/ras_event.h
index 13ea4ee..66f6a43 100644
--- a/include/ras/ras.h
+++ b/include/ras/ras_event.h
@@ -1,5 +1,6 @@
#undef TRACE_SYSTEM
#define TRACE_SYSTEM ras
+#define TRACE_INCLUDE_FILE ras_event

#if !defined(_TRACE_HW_EVENT_MC_H) || defined(TRACE_HEADER_MULTI_READ)
#define _TRACE_HW_EVENT_MC_H
@@ -26,25 +27,25 @@
/*
* Default error mechanisms for Memory Controller errors (CE and UE)
*/
-TRACE_EVENT(mc_error,
+TRACE_EVENT(mc_event,

TP_PROTO(const unsigned int err_type,
const unsigned int mc_index,
- const char *msg,
+ const char *error_msg,
const char *label,
const char *location,
- const char *detail,
+ const char *core_detail,
const char *driver_detail),

- TP_ARGS(err_type, mc_index, msg, label, location,
- detail, driver_detail),
+ TP_ARGS(err_type, mc_index, error_msg, label, location,
+ core_detail, driver_detail),

TP_STRUCT__entry(
__field( unsigned int, err_type )
__field( unsigned int, mc_index )
- __string( msg, msg )
+ __string( msg, error_msg )
__string( label, label )
- __string( detail, detail )
+ __string( detail, core_detail )
__string( location, location )
__string( driver_detail, driver_detail )
),
@@ -52,20 +53,20 @@ TRACE_EVENT(mc_error,
TP_fast_assign(
__entry->err_type = err_type;
__entry->mc_index = mc_index;
- __assign_str(msg, msg);
+ __assign_str(msg, error_msg);
__assign_str(label, label);
__assign_str(location, location);
- __assign_str(detail, detail);
+ __assign_str(detail, core_detail);
__assign_str(driver_detail, driver_detail);
),

- TP_printk(HW_ERR "mem_ctl#%d: %s error %s on memory stick \"%s\" (%s %s %s)",
- __entry->mc_index,
+ TP_printk("%s error:%s on memory stick \"%s\" (mc:%d %s %s %s)",
(__entry->err_type == HW_EVENT_ERR_CORRECTED) ? "Corrected" :
((__entry->err_type == HW_EVENT_ERR_FATAL) ?
"Fatal" : "Uncorrected"),
__get_str(msg),
__get_str(label),
+ __entry->mc_index,
__get_str(location),
__get_str(detail),
__get_str(driver_detail))

\
 
 \ /
  Last update: 2012-05-11 04:21    [W:0.214 / U:0.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site