Re: [Xen-devel] [PATCH 2/2] xen: before ballooning hotplugged memory, set frames to invalid

[David Vrabel <david.vrabel@citrix.com>]

On 19/03/15 17:19, Juergen Gross wrote:
> On 03/19/2015 05:18 PM, David Vrabel wrote:
>> On 19/03/15 14:31, Juergen Gross wrote:
>>> Commit 25b884a83d487fd62c3de7ac1ab5549979188482 ("x86/xen: set
>>> regions above the end of RAM as 1:1") introduced a regression.
>>>
>>> To be able to add memory pages which were added via memory hotplug to
>>> a pv domain, the pages must be "invalid" instead of "identity" in the
>>> p2m list before they can be added.
>> [...]
>>> --- a/drivers/xen/balloon.c
>>> +++ b/drivers/xen/balloon.c
>>> @@ -221,15 +221,24 @@ static bool balloon_is_inflated(void)
>>>
>>>   static enum bp_state reserve_additional_memory(long credit)
>>>   {
>>> -    int nid, rc;
>>> +    int nid, rc = 0;
>>>       u64 hotplug_start_paddr;
>>>       unsigned long balloon_hotplug = credit;
>>> +    unsigned long pfn;
>>>
>>>       hotplug_start_paddr = PFN_PHYS(SECTION_ALIGN_UP(max_pfn));
>>>       balloon_hotplug = round_up(balloon_hotplug, PAGES_PER_SECTION);
>>>       nid = memory_add_physaddr_to_nid(hotplug_start_paddr);
>>>
>>> -    rc = add_memory(nid, hotplug_start_paddr, balloon_hotplug <<
>>> PAGE_SHIFT);
>>> +    for (pfn = PFN_DOWN(hotplug_start_paddr);
>>> +         !rc && pfn < PFN_DOWN(hotplug_start_paddr) + balloon_hotplug;
>>> +         pfn++)
>>> +        if (!set_phys_to_machine(pfn, INVALID_P2M_ENTRY))
>>> +            rc = 1;
>>
>>      rc = -ENOMEM;
> 
> I used the value 1 on purpose to be able to identify the case by the
> value printed in the warning below.

Ok.

> 
>>      break;
> 
> Why? !rc is already tested in the for() clause.

I prefer an explicit break.

>>> +
>>> +    if (!rc)
>>> +        rc = add_memory(nid, hotplug_start_paddr,
>>> +                balloon_hotplug << PAGE_SHIFT);
>>
>> Use else here.
> 
> Huh? I want the message to be printed if either set_phys_to_machine()
> or add_memory() failed.

Ok.

David