[2/2] rasdaemon: add support for memory Corrected Error predictive failure analysis

Commit Message

Lv Ying May 29, 2020, 8:24 a.m. UTC

From: wuyun <[e-mail]>

Memory Corrected Error was corrected by hardware. These errors do not
require immediate software actions, but are still reported for
accounting and predictive failure analysis.

Based on statistical results, some actions can be taken to prevent
Corrected Error from evoluting to Uncorrected Error.

Signed-off-by: lvying <[e-mail]>
---
 Makefile.am               |   4 +-
 misc/rasdaemon.env        |  29 +++++
 misc/rasdaemon.service.in |   1 +
 ras-events.c              |   3 +
 ras-mc-handler.c          |   5 +
 ras-page-isolation.c      | 308 ++++++++++++++++++++++++++++++++++++++++++++++
 ras-page-isolation.h      |  68 ++++++++++
 7 files changed, 416 insertions(+), 2 deletions(-)
 create mode 100644 misc/rasdaemon.env
 create mode 100644 ras-page-isolation.c
 create mode 100644 ras-page-isolation.h

Comments

Mauro Carvalho Chehab May 29, 2020, 2:31 p.m. UTC | #1

Em Fri, 29 May 2020 16:24:23 +0800
lvying6 <[e-mail]> escreveu:

> From: wuyun <[e-mail]>
> 
> Memory Corrected Error was corrected by hardware. These errors do not
> require immediate software actions, but are still reported for
> accounting and predictive failure analysis.
> 
> Based on statistical results, some actions can be taken to prevent
> Corrected Error from evoluting to Uncorrected Error.
> 
> Signed-off-by: lvying <[e-mail]>
> ---
>  Makefile.am               |   4 +-
>  misc/rasdaemon.env        |  29 +++++
>  misc/rasdaemon.service.in |   1 +
>  ras-events.c              |   3 +
>  ras-mc-handler.c          |   5 +
>  ras-page-isolation.c      | 308 ++++++++++++++++++++++++++++++++++++++++++++++
>  ras-page-isolation.h      |  68 ++++++++++
>  7 files changed, 416 insertions(+), 2 deletions(-)
>  create mode 100644 misc/rasdaemon.env
>  create mode 100644 ras-page-isolation.c
>  create mode 100644 ras-page-isolation.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index 2ff742d..6fc39f2 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -17,7 +17,7 @@ all-local: $(SYSTEMD_SERVICES)
>  
>  sbin_PROGRAMS = rasdaemon
>  rasdaemon_SOURCES = rasdaemon.c ras-events.c ras-mc-handler.c \
> -		    bitfield.c rbtree.c
> +		    bitfield.c rbtree.c ras-page-isolation.c
>  if WITH_SQLITE3
>     rasdaemon_SOURCES += ras-record.c
>  endif
> @@ -59,7 +59,7 @@ rasdaemon_LDADD = -lpthread $(SQLITE3_LIBS) libtrace/libtrace.a
>  include_HEADERS = config.h  ras-events.h  ras-logger.h  ras-mc-handler.h \
>  		  ras-aer-handler.h ras-mce-handler.h ras-record.h bitfield.h ras-report.h \
>  		  ras-extlog-handler.h ras-arm-handler.h ras-non-standard-handler.h \
> -		  ras-devlink-handler.h ras-diskerror-handler.h rbtree.h
> +		  ras-devlink-handler.h ras-diskerror-handler.h rbtree.h ras-page-isolation.h
>  
>  # This rule can't be called with more than one Makefile job (like make -j8)
>  # I can't figure out a way to fix that
> diff --git a/misc/rasdaemon.env b/misc/rasdaemon.env
> new file mode 100644
> index 0000000..0c05af0
> --- /dev/null
> +++ b/misc/rasdaemon.env
> @@ -0,0 +1,29 @@
> +# Page Isolation
> +# Note: Run-time configuration is unsupported, service restart needed.


> +# Note: this file should be installed at /etc/sysconfig/rasdaemon

If so, be sure to add it to Makefile.am, at "install-data-local"
target.

It would also be nice if you patch could touch[1]:

	misc/rasdaemon.spec.in

[1] I'll end doing this if your patch series doesn't, as
    my release scripts depend on having it built into
    Fedora rawhide, as I want to ensure that all build
    time dependencies are properly mapped - at least at the
    *.spec file.

> +
> +# Specify the threshold of isolating buggy pages.
> +#
> +# Format:
> +#   [0-9]+[unit]


> +# WARNING: please make sure perfectly match this format.

I would improve the parser and remove this warning.

> +#
> +# Supported units:
> +# PAGE_CE_REFRESH_CYCLE: D|d (day), H|h (hour), M|m (min), default is in hour
> +# PAGE_CE_THRESHOLD: K|k (x1000), M|m (x1000k), default is none
> +#
> +# The two configs will only take no effect when PAGE_CE_ACTION is "off".
> +PAGE_CE_REFRESH_CYCLE="24h"
> +PAGE_CE_THRESHOLD="50"
> +
> +# Specify the internal action in rasdaemon to exceeding a page error threshold.
> +#
> +# off      no action
> +# account  only account errors
> +# soft     try to soft-offline page without killing any processes
> +#          This requires an uptodate kernel. Might not be successfull.
> +# hard     try to hard-offline page by killing processes
> +#          Requires an uptodate kernel. Might not be successfull.
> +# soft-then-hard   First try to soft offline, then try hard offlining.
> +# Note: default offline choice is "soft".
> +PAGE_CE_ACTION="soft"
> diff --git a/misc/rasdaemon.service.in b/misc/rasdaemon.service.in
> index be9ad5a..e73a08a 100644
> --- a/misc/rasdaemon.service.in
> +++ b/misc/rasdaemon.service.in
> @@ -3,6 +3,7 @@ Description=RAS daemon to log the RAS events
>  After=syslog.target
>  
>  [Service]
> +EnvironmentFile=/etc/sysconfig/rasdaemon
>  ExecStart=@sbindir@/rasdaemon -f -r
>  ExecStartPost=@sbindir@/rasdaemon --enable
>  ExecStop=@sbindir@/rasdaemon --disable
> diff --git a/ras-events.c b/ras-events.c
> index 511c93d..c034f6c 100644
> --- a/ras-events.c
> +++ b/ras-events.c
> @@ -798,6 +798,9 @@ int handle_ras_events(int record_events)
>  	ras->page_size = page_size;
>  	ras->record_events = record_events;
>  


> +	/* FIXME: enable memory isolation unconditionally */
> +	ras_page_account_init();
> +

As I commented on patch 1, I'd like to be able to allow building
rasdaemon with a feature subset.

>  	rc = add_event_handler(ras, pevent, page_size, "ras", "mc_event",
>  			       ras_mc_event_handler, NULL, MC_EVENT);
>  	if (!rc)
> diff --git a/ras-mc-handler.c b/ras-mc-handler.c
> index deb7e05..bfbe1ef 100644
> --- a/ras-mc-handler.c
> +++ b/ras-mc-handler.c
> @@ -23,6 +23,7 @@
>  #include "ras-mc-handler.h"
>  #include "ras-record.h"
>  #include "ras-logger.h"
> +#include "ras-page-isolation.h"
>  #include "ras-report.h"
>  
>  int ras_mc_event_handler(struct trace_seq *s,
> @@ -183,6 +184,10 @@ int ras_mc_event_handler(struct trace_seq *s,
>  
>  	ras_store_mc_event(ras, &ev);
>  
> +	/* Account page corrected errors */
> +	if (!strcmp(ev.error_type, "Corrected"))
> +		ras_record_page_error(ev.address, ev.error_count, now);
> +

Same here: the code should be called only if the predictive
failure analysis feature is enabled, e. g.:

	#ifdef HAVE_feature_name
		/* Account page corrected errors */
		if (!strcmp(ev.error_type, "Corrected"))
			ras_record_page_error(ev.address, ev.error_count, now);

	#endif

>  #ifdef HAVE_ABRT_REPORT
>  	/* Report event to ABRT */
>  	ras_report_mc_event(ras, &ev);
> diff --git a/ras-page-isolation.c b/ras-page-isolation.c
> new file mode 100644
> index 0000000..1bd04e4
> --- /dev/null
> +++ b/ras-page-isolation.c
> @@ -0,0 +1,308 @@
> +/*
> + * Copyright (C) 2015 Yun Wu (Abel)  <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> +*/
> +
> +#include <ctype.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include "ras-logger.h"
> +#include "ras-page-isolation.h"
> +
> +static const struct config threshold_units[] = {
> +	{ "m",	1000 },
> +	{ "k",	1000 },
> +	{ "",	1    },
> +	{}
> +};
> +
> +static const struct config cycle_units[] = {
> +	{ "d",	24 },
> +	{ "h",	60 },
> +	{ "m",	60 },
> +	{}
> +};
> +
> +static struct isolation threshold = {
> +	.name = "PAGE_CE_THRESHOLD",
> +	.units = threshold_units,
> +	.env = "50",
> +	.unit = "",
> +};
> +
> +static struct isolation cycle = {
> +	.name = "PAGE_CE_REFRESH_CYCLE",
> +	.units = cycle_units,
> +	.env = "24h",
> +	.unit = "h",
> +};
> +
> +static const char *kernel_offline[] = {
> +	[OFFLINE_SOFT]		 = "/sys/devices/system/memory/soft_offline_page",
> +	[OFFLINE_HARD]		 = "/sys/devices/system/memory/hard_offline_page",
> +	[OFFLINE_SOFT_THEN_HARD] = "/sys/devices/system/memory/soft_offline_page",
> +};
> +
> +static const struct config offline_choice[] = {
> +	{ "off",		OFFLINE_OFF },
> +	{ "account",		OFFLINE_ACCOUNT },
> +	{ "soft",		OFFLINE_SOFT },
> +	{ "hard",		OFFLINE_HARD },
> +	{ "soft-then-hard",	OFFLINE_SOFT_THEN_HARD },
> +	{}
> +};
> +
> +static const char *page_state[] = {
> +	[PAGE_ONLINE]		= "online",
> +	[PAGE_OFFLINE]		= "offlined",
> +	[PAGE_OFFLINE_FAILED]	= "offline-failed",
> +};
> +
> +static enum otype offline = OFFLINE_SOFT;
> +static struct rb_root page_records;
> +
> +static void page_offline_init(void)
> +{
> +	const char *env = "PAGE_CE_ACTION";
> +	char *choice = getenv(env);
> +	const struct config *c = NULL;
> +	int matched = 0;
> +
> +	if (choice) {
> +		for (c = offline_choice; c->name; c++) {
> +			if (!strcasecmp(choice, c->name)) {
> +				offline = c->val;
> +				matched = 1;
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (!matched)
> +		log(TERM, LOG_INFO, "Improper %s, set to default soft\n", env);
> +
> +	if (offline > OFFLINE_ACCOUNT && access(kernel_offline[offline], W_OK)) {
> +		log(TERM, LOG_INFO, "Kernel does not support page offline interface\n");
> +		offline = OFFLINE_ACCOUNT;
> +	}
> +
> +	log(TERM, LOG_INFO, "Page offline choice on Corrected Errors is %s\n",
> +	    offline_choice[offline].name);
> +}
> +
> +static void parse_isolation_env(struct isolation *config)
> +{
> +	char *env = getenv(config->name), *unit = NULL;
> +	const struct config *units = NULL;
> +	unsigned long value;
> +	int no_unit, unit_matched;
> +	int last, i;
> +


> +reparse:

Instead of a goto-based loop, could you please change the code to use
a normal loop?

E. g. Something like:

	do {
		done = try_set_isolation_env();
	} while(!done);

> +	/* Start a new round */
> +	no_unit = unit_matched = 0;

Nitpick: Please place one statement per line, e. g.:

	no_unit = 0;
	unit_patched = 0;

> +
> +	/* Environments could be un-configured */
> +	if (!env || !strlen(env))
> +		goto use_default;

Could you please verify if your code won't have endless loops?

On a first glance, it seems that, if env is zero, the loop
will run forever.

> +
> +	/* Index of the last char of environment */
> +	last = strlen(env) - 1;
> +	unit = env + last;
> +	if (isdigit(*unit)) {
> +		unit = config->unit;
> +		no_unit = 1;
> +	}

I guess you need to add:

	} else {
		last--;
	}

> +
> +	/* Only decimal digit can be accepted */
> +	for (i = 0; i < last; i++) {
> +		if (!isdigit(env[i]))
> +			goto use_default;
> +	}

.. as the last character won't be a digit if no_unit == 0.

> +
> +	/* Check if value is valid or not */
> +	if (sscanf(env, "%lu", &value) < 1 || !value)
> +		goto use_default;

Also here, you should discard the last byte, if it is an unit.

> +
> +	for (units = config->units; units->name; units++) {
> +		if (!strcasecmp(unit, units->name))
> +			unit_matched = 1;
> +		if (unit_matched)
> +			value *= units->val;
> +	}
> +
> +	/* Improper unit */
> +	if (!unit_matched)
> +		goto use_default;
> +
> +	config->env = env;
> +	config->val = value;
> +	config->unit = no_unit ? unit : "";
> +	return;
> +
> +use_default:
> +	log(TERM, LOG_INFO, "Improper %s, set to default %s.\n",
> +	    config->name, config->env);
> +
> +	env = config->env;
> +	goto reparse;
> +}

Btw, after re-reading this code a couple of times, I guess this logic 
could be re-written on a different way, with should avoid the reparse
loop and making it simpler for reviewers to analyze it (code untested):

	static int parse_isolation_env(struct isolation *config)
	{
		char *env = getenv(config->name), *unit = NULL;
		unsigned long value;
		char *s[255];
		int len;

		strncpy(s, env, sizeof(s));
		s[255] = '\0';

		len = strlen(s);

		if (!isdigit(s[len - 1])) {
			for (units = config->units; units->name; units++) {
				if (!strcasecmp(&s[len - 1], units->name)) {
					unit = units->val;
					break;
				}
			}

			/* Return error if the unit is unknown */

			if (!unit)
				return -1;
			else
				s[len - 1] = '\0';
		}

		/* Return error if value is not valid or if it is zero */
		if (sscanf(s, "%lu", &value) < 1 || !value)
			return -1;

		value *= unit->val;
	
		config->env = env;
		config->val = value;
		config->unit = unit ? unit : "";

		return 0;
	}		

Btw, instead of setting a default, I would instead print some error message,
as some action will need to be taken in order to avoid parsing it wrongly.

> +
> +static void page_isolation_init(void)
> +{
> +	/**
> +	 * It's unnecessary to parse threshold configuration when offline
> +	 * choice is off.
> +	 */
> +	if (offline == OFFLINE_OFF)
> +		return;
> +
> +	parse_isolation_env(&threshold);
> +	parse_isolation_env(&cycle);
> +	log(TERM, LOG_INFO, "Threshold of memory Corrected Errors is %s%s / %s%s\n",
> +	    threshold.env, threshold.unit, cycle.env, cycle.unit);
> +}
> +
> +void ras_page_account_init(void)
> +{
> +	page_offline_init();
> +	page_isolation_init();
> +}
> +
> +static int do_page_offline(unsigned long long addr, enum otype type)
> +{
> +	FILE *offline_file;
> +	int err;
> +
> +	offline_file = fopen(kernel_offline[type], "w");
> +	if (!offline_file)
> +		return -1;
> +
> +	fprintf(offline_file, "%#llx", addr);
> +	err = ferror(offline_file) ? -1 : 0;
> +	fclose(offline_file);
> +
> +	return err;
> +}
> +
> +static void page_offline(struct page_record *pr)
> +{
> +	unsigned long long addr = pr->addr;
> +	int ret;
> +
> +	/* Offlining page is not required */
> +	if (offline <= OFFLINE_ACCOUNT)
> +		return;
> +
> +	/* Ignore offlined pages */
> +	if (pr->offlined != PAGE_ONLINE)
> +		return;
> +
> +	/* Time to silence this noisy page */
> +	if (offline == OFFLINE_SOFT_THEN_HARD) {
> +		ret = do_page_offline(addr, OFFLINE_SOFT);
> +		if (ret < 0)
> +			ret = do_page_offline(addr, OFFLINE_HARD);
> +	} else {
> +		ret = do_page_offline(addr, offline);
> +	}
> +
> +	pr->offlined = ret < 0 ? PAGE_OFFLINE_FAILED : PAGE_OFFLINE;
> +
> +	log(TERM, LOG_INFO, "Result of offlining page at %#llx: %s\n",
> +	    addr, page_state[pr->offlined]);
> +}
> +
> +static void page_record(struct page_record *pr, unsigned count, time_t time)
> +{
> +	unsigned long period = time - pr->start;
> +	unsigned long tolerate;
> +
> +	if (period >= cycle.val) {
> +		/**
> +		 * Since we don't refresh automatically, it is possible that the period
> +		 * between two occurences longer than the pre-configured refresh cycle.

Nitpick:
		longer than -> will be longer than

> +		 * In this case, we tolerate the frequency of the whole period up to
> +		 * the pre-configured threshold.
> +		 */
> +		tolerate = (period / (double)cycle.val) * threshold.val;
> +		pr->count -= (tolerate > pr->count) ? pr->count : tolerate;
> +		pr->start = time;
> +		pr->excess = 0;
> +	}
> +
> +	pr->count += count;
> +	if (pr->count >= threshold.val) {
> +		log(TERM, LOG_INFO, "Corrected Errors at %#llx exceed threshold\n", pr->addr);

Nitpick:
	exceed -> exceeded

> +
> +		/**
> +		 * Backup ce count of current cycle to enable next round, which actually
> +		 * should never happen if we can disable overflow completely in the same
> +		 * time unit (but sadly we can't).
> +		 */
> +		pr->excess += pr->count;
> +		pr->count = 0;
> +		page_offline(pr);
> +	}
> +}
> +
> +static struct page_record *page_lookup_insert(unsigned long long addr)
> +{
> +	struct rb_node **entry = &page_records.rb_node;
> +	struct rb_node *parent = NULL;
> +	struct page_record *pr = NULL, *find = NULL;
> +
> +	while (*entry) {
> +		parent = *entry;
> +		pr = rb_entry(parent, struct page_record, entry);
> +		if (addr == pr->addr) {
> +			return pr;
> +		} else if (addr < pr->addr) {
> +			entry = &(*entry)->rb_left;
> +		} else {
> +			entry = &(*entry)->rb_right;
> +		}
> +	}
> +
> +	find = calloc(1, sizeof(struct page_record));
> +	if (!find) {
> +		log(TERM, LOG_ERR, "No memory for page records\n");
> +		return NULL;
> +	}
> +
> +	find->addr = addr;
> +	rb_link_node(&find->entry, parent, entry);
> +	rb_insert_color(&find->entry, &page_records);
> +
> +	return find;
> +}
> +
> +void ras_record_page_error(unsigned long long addr, unsigned count, time_t time)
> +{
> +	struct page_record *pr = NULL;
> +
> +	if (offline == OFFLINE_OFF)
> +		return;
> +
> +	pr = page_lookup_insert(addr & PAGE_MASK);
> +	if (pr) {
> +		if (!pr->start)
> +			pr->start = time;
> +		page_record(pr, count, time);
> +	}
> +}
> diff --git a/ras-page-isolation.h b/ras-page-isolation.h
> new file mode 100644
> index 0000000..6aefa1e
> --- /dev/null
> +++ b/ras-page-isolation.h
> @@ -0,0 +1,68 @@
> +/*
> + * Copyright (C) 2015 Yun Wu (Abel)  <[e-mail]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> +*/
> +
> +#ifndef __RAS_PAGE_ISOLATION_H
> +#define __RAS_PAGE_ISOLATION_H
> +
> +#include <time.h>
> +#include "rbtree.h"
> +
> +#define PAGE_SHIFT		12
> +#define PAGE_SIZE		(1 << PAGE_SHIFT)
> +#define PAGE_MASK		(~(PAGE_SIZE-1))
> +
> +struct config {
> +	char			*name;
> +	int			val;
> +};
> +
> +enum otype {
> +	OFFLINE_OFF,
> +	OFFLINE_ACCOUNT,
> +	OFFLINE_SOFT,
> +	OFFLINE_HARD,
> +	OFFLINE_SOFT_THEN_HARD,
> +};
> +
> +enum pstate {
> +	PAGE_ONLINE,
> +	PAGE_OFFLINE,
> +	PAGE_OFFLINE_FAILED,
> +};
> +
> +struct page_record {
> +	struct rb_node		entry;
> +	unsigned long long	addr;
> +	time_t			start;
> +	enum pstate		offlined;
> +	unsigned long		count;
> +	unsigned long		excess;
> +};
> +
> +struct isolation {
> +	char			*name;
> +	char			*env;
> +	const struct config	*units;
> +	unsigned long		val;
> +	char			*unit;
> +};
> +
> +void ras_page_account_init(void);
> +void ras_record_page_error(unsigned long long addr, unsigned count, time_t time);
> +
> +#endif



Thanks,
Mauro

Lv Ying June 20, 2020, 11:59 a.m. UTC | #2

>> diff --git a/misc/rasdaemon.env b/misc/rasdaemon.env
>> new file mode 100644
>> index 0000000..0c05af0
>> --- /dev/null
>> +++ b/misc/rasdaemon.env
>> @@ -0,0 +1,29 @@
>> +# Page Isolation
>> +# Note: Run-time configuration is unsupported, service restart needed.
> 
> 
>> +# Note: this file should be installed at /etc/sysconfig/rasdaemon
> 
> If so, be sure to add it to Makefile.am, at "install-data-local"
> target.
> 
> It would also be nice if you patch could touch[1]:
> 
> 	misc/rasdaemon.spec.in
> 
> [1] I'll end doing this if your patch series doesn't, as
>      my release scripts depend on having it built into
>      Fedora rawhide, as I want to ensure that all build
>      time dependencies are properly mapped - at least at the
>      *.spec file.
> 

OK, I add %install and %files content:
%install
...
install -D -p -m 0644 misc/rasdaemon.env 
%{buildroot}%{_sysconfdir}/sysconfig/%{name}
...
%file
...
%config(noreplace) %{_sysconfdir}/sysconfig/%{name}
...
Also, I will add instructions for use into man file.

>> +
>> +# Specify the threshold of isolating buggy pages.
>> +#
>> +# Format:
>> +#   [0-9]+[unit]
> 
> 
>> +# WARNING: please make sure perfectly match this format.
> 
> I would improve the parser and remove this warning.
> 

the parser can handle exception input cases. This message is ambiguous.
Just like this:
# Notice: please make sure match this format, rasdaemon will use default 
value for exception input cases.

>> diff --git a/ras-events.c b/ras-events.c
>> index 511c93d..c034f6c 100644
>> --- a/ras-events.c
>> +++ b/ras-events.c
>> @@ -798,6 +798,9 @@ int handle_ras_events(int record_events)
>>   	ras->page_size = page_size;
>>   	ras->record_events = record_events;
>>   
> 
> 
>> +	/* FIXME: enable memory isolation unconditionally */
>> +	ras_page_account_init();
>> +
> 
> As I commented on patch 1, I'd like to be able to allow building
> rasdaemon with a feature subset.
> 

Ok, I rewrite:
#ifdef HAVE_MEMORY_CE_PFA
     /* FIXME: enable memory isolation unconditionally */
     ras_page_account_init();
#endif

>>   
>> +	/* Account page corrected errors */
>> +	if (!strcmp(ev.error_type, "Corrected"))
>> +		ras_record_page_error(ev.address, ev.error_count, now);
>> +
> 
> Same here: the code should be called only if the predictive
> failure analysis feature is enabled, e. g.:
> 
> 	#ifdef HAVE_feature_name
> 		/* Account page corrected errors */
> 		if (!strcmp(ev.error_type, "Corrected"))
> 			ras_record_page_error(ev.address, ev.error_count, now);
> 
> 	#endif
> 
Ok, I use same macro "HAVE_MEMORY_CE_PFA" here.

> 
> Instead of a goto-based loop, could you please change the code to use
> a normal loop?
> 

Ok, I will use less goto-based loop to make this code logic clearly, 
maybe only use goto-base loop to handle exception case.

> E. g. Something like:
> 
> 	do {
> 		done = try_set_isolation_env();
> 	} while(!done);
> 
>> +	/* Start a new round */
>> +	no_unit = unit_matched = 0;
> 
> Nitpick: Please place one statement per line, e. g.:
> 
> 	no_unit = 0;
> 	unit_patched = 0;

OK, I will follow this rule

> 
>> +
>> +	/* Environments could be un-configured */
>> +	if (!env || !strlen(env))
>> +		goto use_default;
> 
> Could you please verify if your code won't have endless loops?
> 
> On a first glance, it seems that, if env is zero, the loop
> will run forever.

No, when the env is zero, it will use default env value:
if (sscanf(env, "%lu", &value) < 1 || !value)
	goto use_default;

> 
>> +
>> +	/* Index of the last char of environment */
>> +	last = strlen(env) - 1;
>> +	unit = env + last;
>> +	if (isdigit(*unit)) {
>> +		unit = config->unit;
>> +		no_unit = 1;
>> +	}
> 
> I guess you need to add:
> 
> 	} else {
> 		last--;
> 	}
> 

No need here, this place just judge whether the env has valid unit, if 
not use the default unit.

>> +
>> +	/* Only decimal digit can be accepted */
>> +	for (i = 0; i < last; i++) {
>> +		if (!isdigit(env[i]))
>> +			goto use_default;
>> +	}
> 
> .. as the last character won't be a digit if no_unit == 0.
> 

It doesn't matter, here only check the characters before the last 
character are all digit.

> 
> Btw, after re-reading this code a couple of times, I guess this logic
> could be re-written on a different way, with should avoid the reparse
> loop and making it simpler for reviewers to analyze it (code untested):
> 
> 	static int parse_isolation_env(struct isolation *config)
> 	{
> 		char *env = getenv(config->name), *unit = NULL;
> 		unsigned long value;
> 		char *s[255];
> 		int len;
> 
> 		strncpy(s, env, sizeof(s));
> 		s[255] = '\0';
> 
> 		len = strlen(s);
> 
> 		if (!isdigit(s[len - 1])) {
> 			for (units = config->units; units->name; units++) {
> 				if (!strcasecmp(&s[len - 1], units->name)) {
> 					unit = units->val;
> 					break;
> 				}
> 			}
> 
> 			/* Return error if the unit is unknown */
> 
> 			if (!unit)
> 				return -1;
> 			else
> 				s[len - 1] = '\0';
> 		}
> 
> 		/* Return error if value is not valid or if it is zero */
> 		if (sscanf(s, "%lu", &value) < 1 || !value)
> 			return -1;
> 
> 		value *= unit->val;
> 	
> 		config->env = env;
> 		config->val = value;
> 		config->unit = unit ? unit : "";
> 
> 		return 0;
> 	}		
> 
> Btw, instead of setting a default, I would instead print some error message,
> as some action will need to be taken in order to avoid parsing it wrongly.

And I rewrite this function and reduce the reparse goto-base statement 
to make it clearly. The first part to check if env characters are valid. 
The second part to parse the env value(if not valid use the default value):

static void parse_isolation_env(struct isolation *config)
{
     char *env = getenv(config->name);
     char *unit = NULL;
     const struct config *units = NULL;
     int i, no_unit;
     int valid = 0;
     int unit_matched = 0;
     unsigned long value, tmp;

     /* check if env is vaild */
     if (env && strlen(env)) {
         /* All the character before unit must be digit */
         for (i = 0; i < strlen(env) - 1; i++) {
             if (!isdigit(env[i]))
                 goto parse;
         }
         if (sscanf(env, "%lu", &value) < 1 || !value)
             goto parse;
         /* check if the unit is vaild */
         unit = env + strlen(env) - 1;
         /* no unit, all the character are value character */
         if (isdigit(*unit)) {
             valid = 1;
             no_unit = 1;
             goto parse;
         }
         for (units = config->units; units->name; units++) {
             /* value character and unit character are both valid */
             if (!strcasecmp(unit, units->name)) {
                 valid = 1;
                 no_unit = 0;
                 break;
             }
         }
     }

parse:
     /* if invalid, use default env */
     if (valid) {
         config->env = env;
         if (!no_unit)
             config->unit = unit;
     } else {
          log(TERM, LOG_INFO, "Improper %s, set to default %s.\n",
                  config->name, config->env);
     }

     /* if env value string is greater than ulong_max, truncate the last 
digit */
     sscanf(config->env, "%lu", &value);
     for (units = config->units; units->name; units++) {
         if (!strcasecmp(config->unit, units->name))
             unit_matched = 1;
         if (unit_matched) {
             tmp = value;
             value *= units->val;
             if (tmp != 0 && value / tmp != units->val)
                 config->overflow = true;
         }
     }
     config->val = value;
     /* In order to output value and unit perfectly */
     config->unit = no_unit ? config->unit : "";
}

I will send patch version2 to give more detail about env parse.

>> +	if (period >= cycle.val) {
>> +		/**
>> +		 * Since we don't refresh automatically, it is possible that the period
>> +		 * between two occurences longer than the pre-configured refresh cycle.
> 
> Nitpick:
> 		longer than -> will be longer than
> 
Ok, I will fix it


>> +	pr->count += count;
>> +	if (pr->count >= threshold.val) {
>> +		log(TERM, LOG_INFO, "Corrected Errors at %#llx exceed threshold\n", pr->addr);
> 
> Nitpick:
> 	exceed -> exceeded
> 
Ok, I will fix it


> 
> Thanks,
> Mauro
> .
>

Patch

diff --git a/Makefile.am b/Makefile.am
index 2ff742d..6fc39f2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -17,7 +17,7 @@  all-local: $(SYSTEMD_SERVICES)
 
 sbin_PROGRAMS = rasdaemon
 rasdaemon_SOURCES = rasdaemon.c ras-events.c ras-mc-handler.c \
-		    bitfield.c rbtree.c
+		    bitfield.c rbtree.c ras-page-isolation.c
 if WITH_SQLITE3
    rasdaemon_SOURCES += ras-record.c
 endif
@@ -59,7 +59,7 @@  rasdaemon_LDADD = -lpthread $(SQLITE3_LIBS) libtrace/libtrace.a
 include_HEADERS = config.h  ras-events.h  ras-logger.h  ras-mc-handler.h \
 		  ras-aer-handler.h ras-mce-handler.h ras-record.h bitfield.h ras-report.h \
 		  ras-extlog-handler.h ras-arm-handler.h ras-non-standard-handler.h \
-		  ras-devlink-handler.h ras-diskerror-handler.h rbtree.h
+		  ras-devlink-handler.h ras-diskerror-handler.h rbtree.h ras-page-isolation.h
 
 # This rule can't be called with more than one Makefile job (like make -j8)
 # I can't figure out a way to fix that
diff --git a/misc/rasdaemon.env b/misc/rasdaemon.env
new file mode 100644
index 0000000..0c05af0
--- /dev/null
+++ b/misc/rasdaemon.env
@@ -0,0 +1,29 @@ 
+# Page Isolation
+# Note: Run-time configuration is unsupported, service restart needed.
+# Note: this file should be installed at /etc/sysconfig/rasdaemon
+
+# Specify the threshold of isolating buggy pages.
+#
+# Format:
+#   [0-9]+[unit]
+# WARNING: please make sure perfectly match this format.
+#
+# Supported units:
+# PAGE_CE_REFRESH_CYCLE: D|d (day), H|h (hour), M|m (min), default is in hour
+# PAGE_CE_THRESHOLD: K|k (x1000), M|m (x1000k), default is none
+#
+# The two configs will only take no effect when PAGE_CE_ACTION is "off".
+PAGE_CE_REFRESH_CYCLE="24h"
+PAGE_CE_THRESHOLD="50"
+
+# Specify the internal action in rasdaemon to exceeding a page error threshold.
+#
+# off      no action
+# account  only account errors
+# soft     try to soft-offline page without killing any processes
+#          This requires an uptodate kernel. Might not be successfull.
+# hard     try to hard-offline page by killing processes
+#          Requires an uptodate kernel. Might not be successfull.
+# soft-then-hard   First try to soft offline, then try hard offlining.
+# Note: default offline choice is "soft".
+PAGE_CE_ACTION="soft"
diff --git a/misc/rasdaemon.service.in b/misc/rasdaemon.service.in
index be9ad5a..e73a08a 100644
--- a/misc/rasdaemon.service.in
+++ b/misc/rasdaemon.service.in
@@ -3,6 +3,7 @@  Description=RAS daemon to log the RAS events
 After=syslog.target
 
 [Service]
+EnvironmentFile=/etc/sysconfig/rasdaemon
 ExecStart=@sbindir@/rasdaemon -f -r
 ExecStartPost=@sbindir@/rasdaemon --enable
 ExecStop=@sbindir@/rasdaemon --disable
diff --git a/ras-events.c b/ras-events.c
index 511c93d..c034f6c 100644
--- a/ras-events.c
+++ b/ras-events.c
@@ -798,6 +798,9 @@  int handle_ras_events(int record_events)
 	ras->page_size = page_size;
 	ras->record_events = record_events;
 
+	/* FIXME: enable memory isolation unconditionally */
+	ras_page_account_init();
+
 	rc = add_event_handler(ras, pevent, page_size, "ras", "mc_event",
 			       ras_mc_event_handler, NULL, MC_EVENT);
 	if (!rc)
diff --git a/ras-mc-handler.c b/ras-mc-handler.c
index deb7e05..bfbe1ef 100644
--- a/ras-mc-handler.c
+++ b/ras-mc-handler.c
@@ -23,6 +23,7 @@ 
 #include "ras-mc-handler.h"
 #include "ras-record.h"
 #include "ras-logger.h"
+#include "ras-page-isolation.h"
 #include "ras-report.h"
 
 int ras_mc_event_handler(struct trace_seq *s,
@@ -183,6 +184,10 @@  int ras_mc_event_handler(struct trace_seq *s,
 
 	ras_store_mc_event(ras, &ev);
 
+	/* Account page corrected errors */
+	if (!strcmp(ev.error_type, "Corrected"))
+		ras_record_page_error(ev.address, ev.error_count, now);
+
 #ifdef HAVE_ABRT_REPORT
 	/* Report event to ABRT */
 	ras_report_mc_event(ras, &ev);
diff --git a/ras-page-isolation.c b/ras-page-isolation.c
new file mode 100644
index 0000000..1bd04e4
--- /dev/null
+++ b/ras-page-isolation.c
@@ -0,0 +1,308 @@ 
+/*
+ * Copyright (C) 2015 Yun Wu (Abel)  <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+*/
+
+#include <ctype.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include "ras-logger.h"
+#include "ras-page-isolation.h"
+
+static const struct config threshold_units[] = {
+	{ "m",	1000 },
+	{ "k",	1000 },
+	{ "",	1    },
+	{}
+};
+
+static const struct config cycle_units[] = {
+	{ "d",	24 },
+	{ "h",	60 },
+	{ "m",	60 },
+	{}
+};
+
+static struct isolation threshold = {
+	.name = "PAGE_CE_THRESHOLD",
+	.units = threshold_units,
+	.env = "50",
+	.unit = "",
+};
+
+static struct isolation cycle = {
+	.name = "PAGE_CE_REFRESH_CYCLE",
+	.units = cycle_units,
+	.env = "24h",
+	.unit = "h",
+};
+
+static const char *kernel_offline[] = {
+	[OFFLINE_SOFT]		 = "/sys/devices/system/memory/soft_offline_page",
+	[OFFLINE_HARD]		 = "/sys/devices/system/memory/hard_offline_page",
+	[OFFLINE_SOFT_THEN_HARD] = "/sys/devices/system/memory/soft_offline_page",
+};
+
+static const struct config offline_choice[] = {
+	{ "off",		OFFLINE_OFF },
+	{ "account",		OFFLINE_ACCOUNT },
+	{ "soft",		OFFLINE_SOFT },
+	{ "hard",		OFFLINE_HARD },
+	{ "soft-then-hard",	OFFLINE_SOFT_THEN_HARD },
+	{}
+};
+
+static const char *page_state[] = {
+	[PAGE_ONLINE]		= "online",
+	[PAGE_OFFLINE]		= "offlined",
+	[PAGE_OFFLINE_FAILED]	= "offline-failed",
+};
+
+static enum otype offline = OFFLINE_SOFT;
+static struct rb_root page_records;
+
+static void page_offline_init(void)
+{
+	const char *env = "PAGE_CE_ACTION";
+	char *choice = getenv(env);
+	const struct config *c = NULL;
+	int matched = 0;
+
+	if (choice) {
+		for (c = offline_choice; c->name; c++) {
+			if (!strcasecmp(choice, c->name)) {
+				offline = c->val;
+				matched = 1;
+				break;
+			}
+		}
+	}
+
+	if (!matched)
+		log(TERM, LOG_INFO, "Improper %s, set to default soft\n", env);
+
+	if (offline > OFFLINE_ACCOUNT && access(kernel_offline[offline], W_OK)) {
+		log(TERM, LOG_INFO, "Kernel does not support page offline interface\n");
+		offline = OFFLINE_ACCOUNT;
+	}
+
+	log(TERM, LOG_INFO, "Page offline choice on Corrected Errors is %s\n",
+	    offline_choice[offline].name);
+}
+
+static void parse_isolation_env(struct isolation *config)
+{
+	char *env = getenv(config->name), *unit = NULL;
+	const struct config *units = NULL;
+	unsigned long value;
+	int no_unit, unit_matched;
+	int last, i;
+
+reparse:
+	/* Start a new round */
+	no_unit = unit_matched = 0;
+
+	/* Environments could be un-configured */
+	if (!env || !strlen(env))
+		goto use_default;
+
+	/* Index of the last char of environment */
+	last = strlen(env) - 1;
+	unit = env + last;
+	if (isdigit(*unit)) {
+		unit = config->unit;
+		no_unit = 1;
+	}
+
+	/* Only decimal digit can be accepted */
+	for (i = 0; i < last; i++) {
+		if (!isdigit(env[i]))
+			goto use_default;
+	}
+
+	/* Check if value is valid or not */
+	if (sscanf(env, "%lu", &value) < 1 || !value)
+		goto use_default;
+
+	for (units = config->units; units->name; units++) {
+		if (!strcasecmp(unit, units->name))
+			unit_matched = 1;
+		if (unit_matched)
+			value *= units->val;
+	}
+
+	/* Improper unit */
+	if (!unit_matched)
+		goto use_default;
+
+	config->env = env;
+	config->val = value;
+	config->unit = no_unit ? unit : "";
+	return;
+
+use_default:
+	log(TERM, LOG_INFO, "Improper %s, set to default %s.\n",
+	    config->name, config->env);
+
+	env = config->env;
+	goto reparse;
+}
+
+static void page_isolation_init(void)
+{
+	/**
+	 * It's unnecessary to parse threshold configuration when offline
+	 * choice is off.
+	 */
+	if (offline == OFFLINE_OFF)
+		return;
+
+	parse_isolation_env(&threshold);
+	parse_isolation_env(&cycle);
+	log(TERM, LOG_INFO, "Threshold of memory Corrected Errors is %s%s / %s%s\n",
+	    threshold.env, threshold.unit, cycle.env, cycle.unit);
+}
+
+void ras_page_account_init(void)
+{
+	page_offline_init();
+	page_isolation_init();
+}
+
+static int do_page_offline(unsigned long long addr, enum otype type)
+{
+	FILE *offline_file;
+	int err;
+
+	offline_file = fopen(kernel_offline[type], "w");
+	if (!offline_file)
+		return -1;
+
+	fprintf(offline_file, "%#llx", addr);
+	err = ferror(offline_file) ? -1 : 0;
+	fclose(offline_file);
+
+	return err;
+}
+
+static void page_offline(struct page_record *pr)
+{
+	unsigned long long addr = pr->addr;
+	int ret;
+
+	/* Offlining page is not required */
+	if (offline <= OFFLINE_ACCOUNT)
+		return;
+
+	/* Ignore offlined pages */
+	if (pr->offlined != PAGE_ONLINE)
+		return;
+
+	/* Time to silence this noisy page */
+	if (offline == OFFLINE_SOFT_THEN_HARD) {
+		ret = do_page_offline(addr, OFFLINE_SOFT);
+		if (ret < 0)
+			ret = do_page_offline(addr, OFFLINE_HARD);
+	} else {
+		ret = do_page_offline(addr, offline);
+	}
+
+	pr->offlined = ret < 0 ? PAGE_OFFLINE_FAILED : PAGE_OFFLINE;
+
+	log(TERM, LOG_INFO, "Result of offlining page at %#llx: %s\n",
+	    addr, page_state[pr->offlined]);
+}
+
+static void page_record(struct page_record *pr, unsigned count, time_t time)
+{
+	unsigned long period = time - pr->start;
+	unsigned long tolerate;
+
+	if (period >= cycle.val) {
+		/**
+		 * Since we don't refresh automatically, it is possible that the period
+		 * between two occurences longer than the pre-configured refresh cycle.
+		 * In this case, we tolerate the frequency of the whole period up to
+		 * the pre-configured threshold.
+		 */
+		tolerate = (period / (double)cycle.val) * threshold.val;
+		pr->count -= (tolerate > pr->count) ? pr->count : tolerate;
+		pr->start = time;
+		pr->excess = 0;
+	}
+
+	pr->count += count;
+	if (pr->count >= threshold.val) {
+		log(TERM, LOG_INFO, "Corrected Errors at %#llx exceed threshold\n", pr->addr);
+
+		/**
+		 * Backup ce count of current cycle to enable next round, which actually
+		 * should never happen if we can disable overflow completely in the same
+		 * time unit (but sadly we can't).
+		 */
+		pr->excess += pr->count;
+		pr->count = 0;
+		page_offline(pr);
+	}
+}
+
+static struct page_record *page_lookup_insert(unsigned long long addr)
+{
+	struct rb_node **entry = &page_records.rb_node;
+	struct rb_node *parent = NULL;
+	struct page_record *pr = NULL, *find = NULL;
+
+	while (*entry) {
+		parent = *entry;
+		pr = rb_entry(parent, struct page_record, entry);
+		if (addr == pr->addr) {
+			return pr;
+		} else if (addr < pr->addr) {
+			entry = &(*entry)->rb_left;
+		} else {
+			entry = &(*entry)->rb_right;
+		}
+	}
+
+	find = calloc(1, sizeof(struct page_record));
+	if (!find) {
+		log(TERM, LOG_ERR, "No memory for page records\n");
+		return NULL;
+	}
+
+	find->addr = addr;
+	rb_link_node(&find->entry, parent, entry);
+	rb_insert_color(&find->entry, &page_records);
+
+	return find;
+}
+
+void ras_record_page_error(unsigned long long addr, unsigned count, time_t time)
+{
+	struct page_record *pr = NULL;
+
+	if (offline == OFFLINE_OFF)
+		return;
+
+	pr = page_lookup_insert(addr & PAGE_MASK);
+	if (pr) {
+		if (!pr->start)
+			pr->start = time;
+		page_record(pr, count, time);
+	}
+}
diff --git a/ras-page-isolation.h b/ras-page-isolation.h
new file mode 100644
index 0000000..6aefa1e
--- /dev/null
+++ b/ras-page-isolation.h
@@ -0,0 +1,68 @@ 
+/*
+ * Copyright (C) 2015 Yun Wu (Abel)  <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+*/
+
+#ifndef __RAS_PAGE_ISOLATION_H
+#define __RAS_PAGE_ISOLATION_H
+
+#include <time.h>
+#include "rbtree.h"
+
+#define PAGE_SHIFT		12
+#define PAGE_SIZE		(1 << PAGE_SHIFT)
+#define PAGE_MASK		(~(PAGE_SIZE-1))
+
+struct config {
+	char			*name;
+	int			val;
+};
+
+enum otype {
+	OFFLINE_OFF,
+	OFFLINE_ACCOUNT,
+	OFFLINE_SOFT,
+	OFFLINE_HARD,
+	OFFLINE_SOFT_THEN_HARD,
+};
+
+enum pstate {
+	PAGE_ONLINE,
+	PAGE_OFFLINE,
+	PAGE_OFFLINE_FAILED,
+};
+
+struct page_record {
+	struct rb_node		entry;
+	unsigned long long	addr;
+	time_t			start;
+	enum pstate		offlined;
+	unsigned long		count;
+	unsigned long		excess;
+};
+
+struct isolation {
+	char			*name;
+	char			*env;
+	const struct config	*units;
+	unsigned long		val;
+	char			*unit;
+};
+
+void ras_page_account_init(void);
+void ras_record_page_error(unsigned long long addr, unsigned count, time_t time);
+
+#endif