Re: [PATCH 6/7] bootmem: use pfn/page conversion macros

[Dave Hansen]

On Tue, 2006-06-27 at 14:54 +0200, Franck Bui-Huu wrote:
>  static void __init free_bootmem_core(bootmem_data_t *bdata, unsigned
> long addr,
>                                      unsigned long size)
>  {
> +       unsigned long sidx, eidx;
>         unsigned long i;
> -       unsigned long start;
> +
>         /*
>          * round down end of usable mem, partially free pages are
>          * considered reserved.
>          */
> -       unsigned long sidx;
> -       unsigned long eidx = (addr + size -
> bdata->node_boot_start)/PAGE_SIZE;
> -       unsigned long end = (addr + size)/PAGE_SIZE;
> -
>         BUG_ON(!size);
> -       BUG_ON(end > bdata->node_low_pfn);
> +       BUG_ON(PFN_DOWN(addr + size) > bdata->node_low_pfn);

In general, I like these kinds of conversions.  But, in this case, I
think it makes the code harder to read.  Those intermediate variables
are really nice and I think they make the code much more readable.  

Do you really prefer:

       BUG_ON(PFN_DOWN(addr + size) > bdata->node_low_pfn)

over

       BUG_ON(end > bdata->node_low_pfn);

Also, these do a bit more than just conversions to using the pfn/page
macros.  With this much churn, it is more than possible that bugs can
creep in.  How about a bit more restrictive conversion to the PFN_
macros, first?

Oh, and if you're going to chew through it later, feel free to make
things like sidx into decent variable names. ;)

Is everybody else OK with this code churn?  It doesn't appear that there
is too much in -mm pending in this area.

-- Dave

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/