lkml.org 
[lkml]   [2010]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 01/35] lmb: prepare x86 to use lmb to replace early_res
    From
    Date
    On Thu, 2010-05-13 at 17:19 -0700, Yinghai Lu wrote:
    > 1. expose lmb_debug
    > 2. expose lmb_reserved_init_regions
    > 3. expose lmb_add_region
    > 4. prection for include linux/lmb.h in mm/page_alloc.c and mm/bootmem.c
    > 5. lmb_find_base() should return LMB_ERROR in one failing path.
    > (this one cost me 3 hours !)
    > 6. move LMB_ERROR to lmb.h

    Oh well, let's start somewhere...

    > Signed-off-by: Yinghai Lu <yinghai@kernel.org>
    > ---
    > include/linux/lmb.h | 4 ++++
    > lib/lmb.c | 21 +++++++++------------
    > 2 files changed, 13 insertions(+), 12 deletions(-)
    >
    > diff --git a/include/linux/lmb.h b/include/linux/lmb.h
    > index 6f8c4bd..7987766 100644
    > --- a/include/linux/lmb.h
    > +++ b/include/linux/lmb.h
    > @@ -19,6 +19,7 @@
    > #include <asm/lmb.h>
    >
    > #define INIT_LMB_REGIONS 128
    > +#define LMB_ERROR (~(phys_addr_t)0)

    Ok so this was meant to remain internal. You seem to want to expose a
    whole lot of LMB internals, I suppose for your new arch/x86/lmb.c and I
    really really don't like it.

    If we expose LMB_ERROR then all lmb calls that can fail should return
    that. However, the API calls all return 0 instead. Changing that means
    fixing all callers.

    We can't just have a mix bag of result code in stuff that is exposed.

    If all you need LMB_ERROR is to expose lmb_find_area() and
    lmb_add_region() then make the above __ and export a public variant of
    it that returns 0.

    But that's not the right approach. The right thing to do I believe is to
    instead change LMB to use proper errno.h values.

    For things like lmb_add_region(), return then as a negative int. For
    things that return a phys_addr_t as well with a proper casting macro
    since I -think- we can safely consider that phys addrs in the range
    -PAGE_SIZE..-1 can be error codes. Just like we do for PTR_ERR etc...

    This should be a separate patch btw.

    I'm also not too happy with exposing lmb_add_region(). Why would you
    ever need to expose it ? Just call lmb_reserve() if you want to reserve
    something. lmb_add_region() is an internal function and has no business
    being used outside of the main lmb.c file.

    Also:

    > /* Calculate new doubled size */
    > old_size = type->max * sizeof(struct lmb_region);
    > new_size = old_size << 1;
    > @@ -206,7 +199,7 @@ static int lmb_double_array(struct lmb_type *type)
    > new_array = kmalloc(new_size, GFP_KERNEL);
    > addr = new_array == NULL ? LMB_ERROR : __pa(new_array);
    > } else
    > - addr = lmb_find_base(new_size, sizeof(phys_addr_t), 0, LMB_ALLOC_ACCESSIBLE);
    > + addr = lmb_find_base(new_size, sizeof(struct lmb_region), 0, LMB_ALLOC_ACCESSIBLE);

    Why this change ? Does it need to be aligned to the struct size ? if you
    really want that and have a good justification, make this a separate
    patch and explain why you are doing that in the changeset comment.

    > if (addr == LMB_ERROR) {
    > pr_err("lmb: Failed to double %s array from %ld to %ld entries !\n",
    > lmb_type_name(type), type->max, type->max * 2);
    > @@ -214,6 +207,10 @@ static int lmb_double_array(struct lmb_type *type)
    > }
    > new_array = __va(addr);
    >
    > + if (lmb_debug)
    > + pr_info("lmb: %s array is doubled to %ld at %llx - %llx",
    > + lmb_type_name(type), type->max * 2, (u64)addr, (u64)addr + new_size);
    > +
    > /* Found space, we now need to move the array over before
    > * we add the reserved region since it may be our reserved
    > * array itself that is full.
    > @@ -249,7 +246,7 @@ extern int __weak lmb_memory_can_coalesce(phys_addr_t addr1, phys_addr_t size1,
    > return 1;
    > }

    Cheers,
    Ben.



    \
     
     \ /
      Last update: 2010-05-14 04:17    [W:0.026 / U:90.116 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site