Messages in this thread | | | Subject | Re: [PATCH 6/7] bootmem: use pfn/page conversion macros | From | Dave Hansen <> | Date | Tue, 27 Jun 2006 12:26:37 -0700 |
| |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |