Re: [PATCH] [13/16] HWPOISON: The high level memory error handler in the VM v5

Andi Kleen
2009-06-04T05:06:43Z

On Thu, Jun 04, 2009 at 11:24:41AM +0800, Wu Fengguang wrote:
> On Thu, Jun 04, 2009 at 02:46:47AM +0800, Andi Kleen wrote:
>
> [snip]
>
> This patch is full of this style error (the old version didn't have
> this problem though):

I don't see that here. At least nothing new compared to old.

>
> ERROR: code indent should use tabs where possible

It's checkpath clean for me, except for a few > 80 lines on printks,
one list_for_each_entry_safe (which I think checkpatch is wrong on) and
the meminfo comma error which I also think checkpath.pl is wrong on too.

> > + page_cache_release(p);
> > +
> > + /*
> > + * Now truncate the page in the page cache. This is really
> > + * more like a "temporary hole punch"
> > + * Don't do this for block devices when someone else
> > + * has a reference, because it could be file system metadata
> > + * and that's not safe to truncate.
> > + */
> > + mapping = page_mapping(p);
> > + if (mapping && S_ISBLK(mapping->host->i_mode) && page_count(p) > 1) {
>
> Shall use (page_count > 2) to count for the page cache reference.

I think the page cache reference got dropped in

if (!isolate_lru_page(p))
page_cache_release(p);

So it should be only one if there are no other users

> Or can we base the test on busy buffers instead of page count? Nick?

At least the S_ISBLK test is the best one I came up with. I'm not
saying it's the absolutely best.

> > + SetPageError(p);
> > + /* TBD: print more information about the file. */
> > + if (mapping) {
> > + /*
> > + * IO error will be reported by write(), fsync(), etc.
> > + * who check the mapping.
>
> btw, here are some side notes on EIO.
>
> close() *may* also report it. NFS will sync file on close.

I think the comment is already too verbose, sure there are other
details too that it doesn't describe. It's not trying to be a
full reference on linux error reporting. So I prefer to not
add more cases.

> > + * at the wrong time.
> > + *
> > + * So right now we assume that the application DTRT on
>
> DTRT = do the return value test?

Do The Right Thing

> > +};
> > +
> > +static void action_result(unsigned long pfn, char *msg, int ret)
>
> rename 'ret' to 'action'?

But's not an action (as in a page state handler), it's a return value?
(RECOVERED, FAILED etc.) I can name it result.

> > + * need this to decide if we should kill or just drop the page.
> > + */
> > + mapping = page_mapping(p);
> > + if (!PageDirty(p) && !PageAnon(p) && !PageSwapBacked(p) &&
>
> !PageAnon(p) could be removed: the below non-zero mapping check will
> do the work implicitly.

You mean !page_mapped? Ok.

> > + kill = 0;
> > + printk(KERN_INFO
> > + "MCE %#lx: corrupted page was clean: dropped without side effects\n",
> > + pfn);
> > + ttu |= TTU_IGNORE_HWPOISON;
>
> Why not put the two assignment lines together? :)

Ok. But that was your patch @)

> > + * Try a few times (RED-PEN better strategy?)
> > + */
> > + for (i = 0; i < N_UNMAP_TRIES; i++) {
> > + ret = try_to_unmap(p, ttu);
> > + if (ret == SWAP_SUCCESS)
> > + break;
> > + pr_debug("MCE %#lx: try_to_unmap retry needed %d\n", pfn, ret);
>
> Can we make it a printk? This is a serious accident.

I think it can actually happen due to races, e.g. when a remap
is currently in process.

> > + */
> > + hwpoison_user_mappings(p, pfn, trapno);
> > +
> > + /*
> > + * Torn down by someone else?
> > + */
> > + if (PageLRU(p) && !PageSwapCache(p) && p->mapping == NULL) {
> > + action_result(pfn, "already unmapped LRU", IGNORED);
>
> "NULL mapping LRU" or "already truncated page"?
> At least page_mapped != page_mapping.

It's "already truncated" now.

> > @@ -1311,6 +1311,20 @@
> > .mode = 0644,
> > .proc_handler = &scan_unevictable_handler,
> > },
> > +#ifdef CONFIG_MEMORY_FAILURE
> > + {
> > + .ctl_name = CTL_UNNUMBERED,
> > + .procname = "memory_failure_early_kill",
> > + .data = &sysctl_memory_failure_early_kill,
> > + .maxlen = sizeof(vm_highmem_is_dirtyable),
>
> s/vm_highmem_is_dirtyable/sysctl_memory_failure_early_kill/

Fixed thanks.

> > * Documentation/sysctl/ctl_unnumbered.txt
> > Index: linux/fs/proc/meminfo.c
> > ===================================================================
> > --- linux.orig/fs/proc/meminfo.c 2009-06-03 19:37:38.000000000 +0200
> > +++ linux/fs/proc/meminfo.c 2009-06-03 20:13:43.000000000 +0200
> > @@ -95,7 +95,11 @@
> > "Committed_AS: %8lu kB\n"
> > "VmallocTotal: %8lu kB\n"
> > "VmallocUsed: %8lu kB\n"
> > - "VmallocChunk: %8lu kB\n",
> > + "VmallocChunk: %8lu kB\n"
> > +#ifdef CONFIG_MEMORY_FAILURE
> > + "BadPages: %8lu kB\n"
>
> "HWPoison:" or something like that?
> People is more likely to misinterpret "BadPages".

I'll name it HardwareCorrupted. That makes it too long, but it's hopefully
clearer.

> > vmi.used >> 10,
> > vmi.largest_chunk >> 10
> > +#ifdef CONFIG_MEMORY_FAILURE
> > + ,atomic_long_read(&mce_bad_pages) << (PAGE_SHIFT - 10)
>
> ERROR: space required after that ','

That's one of the cases where checkpatch.pl is stupid. The lone comma
with a space looks absolutely ridiculous to me. I refuse to do ridiculous
things things just for checkpatch.pl deficiencies.

> > Enable the KSM kernel module to allow page sharing of equal pages
> > among different tasks.
> >
> > +config MEMORY_FAILURE
> > + bool
> > +
>
> Do we have code to automatically enable/disable CONFIG_MEMORY_FAILURE
> based on hardware capability?

Yes the architecture can enable it. There's also another patch
which always enables it for testing.

> > +
> > +Control how to kill processes when uncorrected memory error (typically
> > +a 2bit error in a memory module) is detected in the background by hardware.
> > +
> > +1: Kill all processes that have the corrupted page mapped as soon as the
> > +corruption is detected.
> > +
> > +0: Only unmap the page from all processes and only kill a process
> > +who tries to access it.
>
> Note that
> - no process will be killed if the page data is clean and can be
> safely reloaded from disk
> - pages in swap cache is always late killed.

I clarified that

Thanks,
-Andi

--
ak@linux.intel.com -- Speaking for myself only.