lkml.org 
[lkml]   [2008]   [Feb]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] [5/8] Fix logic error in 64bit memory hotadd

    * Andi Kleen <ak@suse.de> wrote:

    > The memory hotadd code assumes that the pud always exists already, but
    > that might be not true. Allocate it if it isn't there.

    ok, this seems an like an ancient memory-hotplug bug. Does anyone even
    use memory hotplug currently? Did you find this bug via review, or did
    it trigger in practice?

    Also, your fix, while it solves a real bug we want to fix, is not quite
    right for upstream integration yet. I can see 3 immediate problems with
    it:

    > + if (!pud_present(*pud)) {
    > + pud = (pud_t *)get_zeroed_page(GFP_ATOMIC);

    the GFP_ATOMIC here can fail.

    The proper solution is to instead extend init_memory_mapping() with a
    gfp_t parameter and pass in GFP_ATOMIC from the early init code (where
    we must not schedule and where GFP_ATOMIC will succeed anyway), but do a
    GFP_KERNEL from arch_add_memory().

    > + if (!pud)
    > + panic("Out of memory");

    the panic() here is very rude to the user in the hotplug usecase.

    The proper solution is to extend init_memory_mapping() with a return
    value, and to check in the caller. arch_add_memory() obviously does not
    want to panic(), it wants to return -ENOMEM to mm/memory_hotplug.c.

    and a small style nit while changing this code:

    > + } else
    > pud = alloc_low_page(&pud_phys);

    please add curly braces to the 'else' branch too. (we generally prefer
    symmetrical curly braces) Thanks,

    Ingo


    \
     
     \ /
      Last update: 2008-02-11 13:49    [from the cache]
    ©2003-2014 Jasper Spaans. hosted at Digital Ocean