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

[Franck Bui-Huu <vagabon.xyz@gmail.com>]

Dave Hansen wrote:
> 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);
> 

It depends of the context. Generally, I think you're right but in that
case I prefer the first case. Why ? because "addr + size" is not a
complex expression, it's quite easy to understand what it means. And
secondly the place where "end" is defined made me think that it was
used all along the function but its use was very punctual.

> 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.
> 

It seems that bootmem allocator doesn't get a lot of interest...
Should we stop this work ? 

		Franck