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

David Vrabel
2015-03-19T17:40:15Z

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