[Xen-devel] Re: [PATCH] e820: fix clip_to_limit()

[Xiao Guangrong <xiaoguangrong@xxxxxxxxxxxxxx>]

Hi Keir,

Keir Fraser wrote:
> I think the 'break' is in the wrong place. Actually also I think the case of

Why we can't break the loop if we meet the "large" end address? what am i 
missed?

> successful change_range_type() is also wrong, as i=0 will be skipped on the
> next iteration of the loop.
> 
> Overall I decided that modifying the e820 map inside the iterator loop was
> just bad and confusing, so I've rewritten it in response to your bug
> discovery. Please take a look at xen-unstable:20419 and let me know if you
> see any issues.

Your patch work well, IMHO, double loop is inefficient, we can decrease the
loop counter if we need "memmove" it, like this:

    if ( e820.map[i].addr < limit )
    {
       e820.map[i].size = limit - e820.map[i].addr;
    }
    else
    {
       memmove(&e820.map[i], &e820.map[i+1],
      (e820.nr_map - i - 1) * sizeof(struct e820entry));
       e820.nr_map--;
 +      i--;    
    }

Also in the original code:

   if ( e820_change_range_type(&e820, max(e820.map[i].addr, limit),
                               old_limit, E820_RAM, E820_UNUSABLE) )
   {
        /* Start again now e820 map must have changed. */
       i = 0;
   }

I think we don't need reload loop hear, because e820_change_range_type() not
touch front object(it may merge with e820.map[i+1], but it not hurt us).

Thanks,
Xiao

> 
> On 09/11/2009 20:04, "Xiao Guangrong" <ericxiao.gr@xxxxxxxxx> wrote:
> 
>> In clip_to_limit(), after memmove(&e820.map[i], &e820.map[i+1], ...), the
>> original
>> e820.map[i+1] become current e820.map[i] but the next loop count is i+1, so
>> the original
>> e820.map[i+1] will be skipped
>>
>> Actually, e820 is sorted form low to high by sanitize_e820_map(), so we can
>> simply break
>> the loop if we meet the item which overrun "limit"
>>
>> Signed-off-by: Xiao Guangrong <ericxiao.gr@xxxxxxxxx>
>>
>> diff -r 93bc06dd1161 -r 5e06f2790d93 xen/arch/x86/e820.c
>> --- a/xen/arch/x86/e820.c Tue Nov 10 02:41:59 2009 +0800
>> +++ b/xen/arch/x86/e820.c Tue Nov 10 03:51:08 2009 +0800
>> @@ -389,6 +389,7 @@
>>                       (e820.nr_map - i - 1) * sizeof(struct e820entry));
>>               e820.nr_map--;
>>           }
>> + break;
>>       }
>>
>>       if ( old_limit )
> 
> 
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel