lkml.org 
[lkml]   [2009]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] [13/16] HWPOISON: The high level memory error handler in the VM v5
    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.


    \
     
     \ /
      Last update: 2009-06-04 07:11    [W:0.031 / U:61.744 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site