Re: [Xen-devel] [PATCH] xen/p2m: fix extra memory regions accounting

["Roger Pau Monné" <roger.pau@citrix.com>]

El 04/09/15 a les 9.47, Juergen Gross ha escrit:
> On 09/04/2015 09:37 AM, Roger Pau Monné wrote:
>> Hello,
>>
>> El 04/09/15 a les 7.07, Juergen Gross ha escrit:
>>> Could you try the attached patch? It should do the job. It is booting
>>> fine on my laptop, but I think you should try it on the machine with
>>> the memory ranges not at page boundary.
>>>
>>>
>>> Juergen
>>>
>>>
>>> extramem.patch
>>>
>>>
>>> commit 3d0f8aa4d1b4c9c16a81902a197b5d6a77e182a0
>>> Author: Juergen Gross <jgross@suse.com>
>>> Date:   Thu Sep 3 17:44:04 2015 +0200
>>>
>>>      xen: switch extra memory accounting to use pfns
>>>
>>>      Instead of using physical addresses for accounting of extra memory
>>>      areas available for ballooning switch to pfns as this is much less
>>>      error prone regarding partial pages.
>>
>> Thanks for taking care of this! I've tested it on the box that has
>> non-page aligned memory ranges and it works fine, only a couple of
>> comments below.
>>
>>>      Reported-by: Roger Pau Monné <roger.pau@citrix.com>
>>>      Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
>>> index 7a5d566..aa58bc4 100644
>>> --- a/arch/x86/xen/setup.c
>>> +++ b/arch/x86/xen/setup.c
>>> @@ -90,62 +90,65 @@ static void __init xen_parse_512gb(void)
>>>       xen_512gb_limit = val;
>>>   }
>>>
>>> -static void __init xen_add_extra_mem(phys_addr_t start, phys_addr_t
>>> size)
>>> +static void __init xen_add_extra_mem(unsigned long start_pfn,
>>> +                     unsigned long n_pfns)
>>
>> Not very important, but for type consistency this should probably be
>> xen_pfn_t instead of unsigned long here and below.
> 
> All of the p2m code is using unsigned long for pfns. I wouldn't mind
> changing this to use xen_pfn_t instead, but this should be done in a
> separate patch. I'll put it on my list.
> 
>>
>>>   {
>>>       int i;
>>>
>>>       for (i = 0; i < XEN_EXTRA_MEM_MAX_REGIONS; i++) {
>>>           /* Add new region. */
>>> -        if (xen_extra_mem[i].size == 0) {
>>> -            xen_extra_mem[i].start = start;
>>> -            xen_extra_mem[i].size  = size;
>>> +        if (xen_extra_mem[i].n_pfns == 0) {
>>> +            xen_extra_mem[i].start_pfn = start_pfn;
>>> +            xen_extra_mem[i].n_pfns = n_pfns;
>>>               break;
>>>           }
>>>           /* Append to existing region. */
>>> -        if (xen_extra_mem[i].start + xen_extra_mem[i].size == start) {
>>> -            xen_extra_mem[i].size += size;
>>> +        if (xen_extra_mem[i].start_pfn + xen_extra_mem[i].n_pfns ==
>>> +            start_pfn) {
>>> +            xen_extra_mem[i].n_pfns += n_pfns;
>>>               break;
>>>           }
>>
>> I also noticed this with the original code, why isn't there a case to
>> prepend to an existing region:
>>
>> if (start_pfn + n_pfns == xen_extra_mem[i].start_pfn) {
>>      xen_extra_mem[i].n_pfns += n_pfns;
>>      xen_extra_mem[i].start_pfn = start_pfn;
>> }
> 
> Processing of memory is done from low to high addresses. This case
> should never happen. And even if it does, the only downside from
> not handling this scenario is wasting an additional table entry.

Right, this case would only be useful for xen_del_extra_mem, and as you
say, the worst that can happen is that we end up using an extra slot.

>>
>>>       }
>>>       if (i == XEN_EXTRA_MEM_MAX_REGIONS)
>>>           printk(KERN_WARNING "Warning: not enough extra memory
>>> regions\n");
>>>
>>> -    memblock_reserve(start, size);
>>> +    memblock_reserve(PFN_PHYS(start_pfn), PFN_PHYS(n_pfns));
>>>   }
>>
>> [...]
>>
>>> @@ -831,9 +833,11 @@ char * __init xen_memory_setup(void)
>>>                   chunk_size = min(size, mem_end - addr);
>>>               } else if (extra_pages) {
>>>                   chunk_size = min(size, PFN_PHYS(extra_pages));
>>> -                extra_pages -= PFN_DOWN(chunk_size);
>>> -                xen_add_extra_mem(addr, chunk_size);
>>> -                xen_max_p2m_pfn = PFN_DOWN(addr + chunk_size);
>>> +                pfn_s = PFN_UP(addr);
>>> +                n_pfns = PFN_DOWN(addr + chunk_size) - pfn_s;
>>
>> Should xen_add_extra_mem check for empty ranges and bail out early, or
>> should the caller make sure it doesn't try to add empty ranges?
>>
>> IMHO it's easier and cleaner to add the check to xen_add_extra_mem.
> 
> This isn't critical at all. Adding an empty range is a nop, as a table
> entry is regarded to be not used when n_pfns is 0.

Yes, it's just so we can bail out earlier instead of iterating over the
whole xen_extra_mem for empty ranges. IMHO, I would add a check to
xen_add_extra_mem so we don't waste cycles.

Roger.