Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining

[Vitaly Kuznetsov <vkuznets@redhat.com>]

David Hildenbrand <david@redhat.com> writes:

> On 07.01.19 14:44, Vitaly Kuznetsov wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
...
>>> On 04.01.19 15:19, Vitaly Kuznetsov wrote:
>>>>  			if (start_pfn > has->start_pfn &&
>>>> -			    !PageReserved(pfn_to_page(start_pfn - 1)))
>>>> +			    online_section_nr(pfn_to_section_nr(start_pfn)))
>>>>  				hv_bring_pgs_online(has, start_pfn, pgs_ol);
>>>>  
>>>>  		}
>>>>
>>>
>>> I wonder if you should use pfn_to_online_page() and check for PageOffline().
>>>
>>> (I guess online_section_nr() should also do the trick)
>> 
>> I'm worried a bit about racing with mm code here as we're not doing
>> mem_hotplug_begin()/done() so I'd slightly prefer keeping
>> online_section_nr() (pfn_to_online_page() also uses it but then it gets
>> to the particular struct page). Moreover, with pfn_to_online_page() we
>> will be looking at some other pfn - because the start_pfn is definitelly
>> offline (pre-patch we were looking at start_pfn-1). Just looking at the
>> whole section seems cleaner.
>
> Fine with me. I guess the section can never be offlined as it still
> contains reserved pages if not fully "fake-onlined" by HV code already.
>
> But we could still consider mem_hotplug_begin()/done() as we could have
> a online section although online_pages() has not completed yet. So we
> could actually touch some "semi onlined section".

Yes, exactly, if we race with section onlining here we may never online
the tail so it will remain 'semi onlined'. I'm going to propose
exporting mem_hotplug_begin()/done() to modules to fix this (in a
separate patch because I anticipate some pushback :-)

>
> Acked-by: David Hildenbrand <david@redhat.com>
>

Thanks!

-- 
Vitaly