All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
@ 2019-01-04 14:19 Vitaly Kuznetsov
  2019-01-07 10:21 ` David Hildenbrand
  0 siblings, 1 reply; 11+ messages in thread
From: Vitaly Kuznetsov @ 2019-01-04 14:19 UTC (permalink / raw)
  To: devel
  Cc: Sasha Levin, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	linux-kernel, Dexuan Cui

Hyper-V memory hotplug protocol has 2M granularity and in Linux x86 we use
128M. To deal with it we implement partial section onlining by registering
custom page onlining callback (hv_online_page()). Later, when more memory
arrives we try to online the 'tail' (see hv_bring_pgs_online()).

It was found that in some cases this 'tail' onlining causes issues:

 BUG: Bad page state in process kworker/0:2  pfn:109e3a
 page:ffffe08344278e80 count:0 mapcount:1 mapping:0000000000000000 index:0x0
 flags: 0xfffff80000000()
 raw: 000fffff80000000 dead000000000100 dead000000000200 0000000000000000
 raw: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
 page dumped because: nonzero mapcount
 ...
 Workqueue: events hot_add_req [hv_balloon]
 Call Trace:
  dump_stack+0x5c/0x80
  bad_page.cold.112+0x7f/0xb2
  free_pcppages_bulk+0x4b8/0x690
  free_unref_page+0x54/0x70
  hv_page_online_one+0x5c/0x80 [hv_balloon]
  hot_add_req.cold.24+0x182/0x835 [hv_balloon]
  ...

Turns out that we now have deferred struct page initialization for memory
hotplug so e.g. memory_block_action() in drivers/base/memory.c does
pages_correctly_probed() check and in that check it avoids inspecting
struct pages and checks sections instead. But in Hyper-V balloon driver we
do PageReserved(pfn_to_page()) check and this is now wrong.

Switch to checking online_section_nr() instead.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/hv_balloon.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 5301fef16c31..7c6349a50ef1 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -888,12 +888,14 @@ static unsigned long handle_pg_range(unsigned long pg_start,
 			pfn_cnt -= pgs_ol;
 			/*
 			 * Check if the corresponding memory block is already
-			 * online by checking its last previously backed page.
-			 * In case it is we need to bring rest (which was not
-			 * backed previously) online too.
+			 * online. It is possible to observe struct pages still
+			 * being uninitialized here so check section instead.
+			 * In case the section is online we need to bring the
+			 * rest of pfns (which were not backed previously)
+			 * online too.
 			 */
 			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);
 
 		}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
  2019-01-04 14:19 [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining Vitaly Kuznetsov
@ 2019-01-07 10:21 ` David Hildenbrand
  2019-01-07 13:44   ` Vitaly Kuznetsov
  0 siblings, 1 reply; 11+ messages in thread
From: David Hildenbrand @ 2019-01-07 10:21 UTC (permalink / raw)
  To: Vitaly Kuznetsov, devel
  Cc: Sasha Levin, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	linux-kernel, Dexuan Cui

On 04.01.19 15:19, Vitaly Kuznetsov wrote:
> Hyper-V memory hotplug protocol has 2M granularity and in Linux x86 we use
> 128M. To deal with it we implement partial section onlining by registering
> custom page onlining callback (hv_online_page()). Later, when more memory
> arrives we try to online the 'tail' (see hv_bring_pgs_online()).
> 
> It was found that in some cases this 'tail' onlining causes issues:
> 
>  BUG: Bad page state in process kworker/0:2  pfn:109e3a
>  page:ffffe08344278e80 count:0 mapcount:1 mapping:0000000000000000 index:0x0
>  flags: 0xfffff80000000()
>  raw: 000fffff80000000 dead000000000100 dead000000000200 0000000000000000
>  raw: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>  page dumped because: nonzero mapcount
>  ...
>  Workqueue: events hot_add_req [hv_balloon]
>  Call Trace:
>   dump_stack+0x5c/0x80
>   bad_page.cold.112+0x7f/0xb2
>   free_pcppages_bulk+0x4b8/0x690
>   free_unref_page+0x54/0x70
>   hv_page_online_one+0x5c/0x80 [hv_balloon]
>   hot_add_req.cold.24+0x182/0x835 [hv_balloon]
>   ...
> 
> Turns out that we now have deferred struct page initialization for memory
> hotplug so e.g. memory_block_action() in drivers/base/memory.c does
> pages_correctly_probed() check and in that check it avoids inspecting
> struct pages and checks sections instead. But in Hyper-V balloon driver we
> do PageReserved(pfn_to_page()) check and this is now wrong.
> 
> Switch to checking online_section_nr() instead.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  drivers/hv/hv_balloon.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 5301fef16c31..7c6349a50ef1 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -888,12 +888,14 @@ static unsigned long handle_pg_range(unsigned long pg_start,
>  			pfn_cnt -= pgs_ol;
>  			/*
>  			 * Check if the corresponding memory block is already
> -			 * online by checking its last previously backed page.
> -			 * In case it is we need to bring rest (which was not
> -			 * backed previously) online too.
> +			 * online. It is possible to observe struct pages still
> +			 * being uninitialized here so check section instead.
> +			 * In case the section is online we need to bring the
> +			 * rest of pfns (which were not backed previously)
> +			 * online too.
>  			 */
>  			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)

-- 

Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
  2019-01-07 10:21 ` David Hildenbrand
@ 2019-01-07 13:44   ` Vitaly Kuznetsov
  2019-01-07 17:56     ` Sasha Levin
  2019-01-08  9:15     ` David Hildenbrand
  0 siblings, 2 replies; 11+ messages in thread
From: Vitaly Kuznetsov @ 2019-01-07 13:44 UTC (permalink / raw)
  To: David Hildenbrand, devel
  Cc: Sasha Levin, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	linux-kernel, Dexuan Cui

David Hildenbrand <david@redhat.com> writes:

> On 04.01.19 15:19, Vitaly Kuznetsov wrote:
>> Hyper-V memory hotplug protocol has 2M granularity and in Linux x86 we use
>> 128M. To deal with it we implement partial section onlining by registering
>> custom page onlining callback (hv_online_page()). Later, when more memory
>> arrives we try to online the 'tail' (see hv_bring_pgs_online()).
>> 
>> It was found that in some cases this 'tail' onlining causes issues:
>> 
>>  BUG: Bad page state in process kworker/0:2  pfn:109e3a
>>  page:ffffe08344278e80 count:0 mapcount:1 mapping:0000000000000000 index:0x0
>>  flags: 0xfffff80000000()
>>  raw: 000fffff80000000 dead000000000100 dead000000000200 0000000000000000
>>  raw: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>  page dumped because: nonzero mapcount
>>  ...
>>  Workqueue: events hot_add_req [hv_balloon]
>>  Call Trace:
>>   dump_stack+0x5c/0x80
>>   bad_page.cold.112+0x7f/0xb2
>>   free_pcppages_bulk+0x4b8/0x690
>>   free_unref_page+0x54/0x70
>>   hv_page_online_one+0x5c/0x80 [hv_balloon]
>>   hot_add_req.cold.24+0x182/0x835 [hv_balloon]
>>   ...
>> 
>> Turns out that we now have deferred struct page initialization for memory
>> hotplug so e.g. memory_block_action() in drivers/base/memory.c does
>> pages_correctly_probed() check and in that check it avoids inspecting
>> struct pages and checks sections instead. But in Hyper-V balloon driver we
>> do PageReserved(pfn_to_page()) check and this is now wrong.
>> 
>> Switch to checking online_section_nr() instead.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  drivers/hv/hv_balloon.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
>> index 5301fef16c31..7c6349a50ef1 100644
>> --- a/drivers/hv/hv_balloon.c
>> +++ b/drivers/hv/hv_balloon.c
>> @@ -888,12 +888,14 @@ static unsigned long handle_pg_range(unsigned long pg_start,
>>  			pfn_cnt -= pgs_ol;
>>  			/*
>>  			 * Check if the corresponding memory block is already
>> -			 * online by checking its last previously backed page.
>> -			 * In case it is we need to bring rest (which was not
>> -			 * backed previously) online too.
>> +			 * online. It is possible to observe struct pages still
>> +			 * being uninitialized here so check section instead.
>> +			 * In case the section is online we need to bring the
>> +			 * rest of pfns (which were not backed previously)
>> +			 * online too.
>>  			 */
>>  			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.

P.S. I still think about bringing mem_hotplug_begin()/done() to
hv_balloon but that's going to be a separate discussion, here I want to
have a small fix backportable to stable.

Thanks,

-- 
Vitaly

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
  2019-01-07 13:44   ` Vitaly Kuznetsov
@ 2019-01-07 17:56     ` Sasha Levin
  2019-01-07 18:38       ` Vitaly Kuznetsov
  2019-01-08  9:15     ` David Hildenbrand
  1 sibling, 1 reply; 11+ messages in thread
From: Sasha Levin @ 2019-01-07 17:56 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: David Hildenbrand, devel, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, linux-kernel, Dexuan Cui

On Mon, Jan 07, 2019 at 02:44:30PM +0100, Vitaly Kuznetsov wrote:
>David Hildenbrand <david@redhat.com> writes:
>> On 04.01.19 15:19, Vitaly Kuznetsov wrote:
>>> Hyper-V memory hotplug protocol has 2M granularity and in Linux x86 we use
>>> 128M. To deal with it we implement partial section onlining by registering
>>> custom page onlining callback (hv_online_page()). Later, when more memory
>>> arrives we try to online the 'tail' (see hv_bring_pgs_online()).
>>>
>>> It was found that in some cases this 'tail' onlining causes issues:
>>>
>>>  BUG: Bad page state in process kworker/0:2  pfn:109e3a
>>>  page:ffffe08344278e80 count:0 mapcount:1 mapping:0000000000000000 index:0x0
>>>  flags: 0xfffff80000000()
>>>  raw: 000fffff80000000 dead000000000100 dead000000000200 0000000000000000
>>>  raw: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>>  page dumped because: nonzero mapcount
>>>  ...
>>>  Workqueue: events hot_add_req [hv_balloon]
>>>  Call Trace:
>>>   dump_stack+0x5c/0x80
>>>   bad_page.cold.112+0x7f/0xb2
>>>   free_pcppages_bulk+0x4b8/0x690
>>>   free_unref_page+0x54/0x70
>>>   hv_page_online_one+0x5c/0x80 [hv_balloon]
>>>   hot_add_req.cold.24+0x182/0x835 [hv_balloon]
>>>   ...
>>>
>>> Turns out that we now have deferred struct page initialization for memory
>>> hotplug so e.g. memory_block_action() in drivers/base/memory.c does
>>> pages_correctly_probed() check and in that check it avoids inspecting
>>> struct pages and checks sections instead. But in Hyper-V balloon driver we
>>> do PageReserved(pfn_to_page()) check and this is now wrong.
>>>
>>> Switch to checking online_section_nr() instead.
>>>
>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>> ---
>>>  drivers/hv/hv_balloon.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
>>> index 5301fef16c31..7c6349a50ef1 100644
>>> --- a/drivers/hv/hv_balloon.c
>>> +++ b/drivers/hv/hv_balloon.c
>>> @@ -888,12 +888,14 @@ static unsigned long handle_pg_range(unsigned long pg_start,
>>>  			pfn_cnt -= pgs_ol;
>>>  			/*
>>>  			 * Check if the corresponding memory block is already
>>> -			 * online by checking its last previously backed page.
>>> -			 * In case it is we need to bring rest (which was not
>>> -			 * backed previously) online too.
>>> +			 * online. It is possible to observe struct pages still
>>> +			 * being uninitialized here so check section instead.
>>> +			 * In case the section is online we need to bring the
>>> +			 * rest of pfns (which were not backed previously)
>>> +			 * online too.
>>>  			 */
>>>  			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.
>
>P.S. I still think about bringing mem_hotplug_begin()/done() to
>hv_balloon but that's going to be a separate discussion, here I want to
>have a small fix backportable to stable.

This should probably be marked for stable then :)

--
Thanks,
Sasha

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
  2019-01-07 17:56     ` Sasha Levin
@ 2019-01-07 18:38       ` Vitaly Kuznetsov
  2019-01-08  6:41         ` Greg KH
                           ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Vitaly Kuznetsov @ 2019-01-07 18:38 UTC (permalink / raw)
  To: Sasha Levin
  Cc: David Hildenbrand, devel, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, linux-kernel, Dexuan Cui

Sasha Levin <sashal@kernel.org> writes:

> On Mon, Jan 07, 2019 at 02:44:30PM +0100, Vitaly Kuznetsov wrote:
>>P.S. I still think about bringing mem_hotplug_begin()/done() to
>>hv_balloon but that's going to be a separate discussion, here I want to
>>have a small fix backportable to stable.
>
> This should probably be marked for stable then :)
>

Yes, please :-)

I didn't test old kernels but this seems to be an old issue,
e.g. DEFERRED_STRUCT_PAGE_INIT appeared in 4.2 - not sure if memory
hotplug code was using it back then but, oh well, it's all history now.

(I remember Greg disliked when people were tagging patches for stable@
themselves, he prefered maintainers deciding if the particular commit
deserves stable@ or not - but as you have a tree now we may as well have
different rules :-)

-- 
Vitaly

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
  2019-01-07 18:38       ` Vitaly Kuznetsov
@ 2019-01-08  6:41         ` Greg KH
  2019-01-08  9:47         ` Dan Carpenter
  2019-01-08 21:23         ` Sasha Levin
  2 siblings, 0 replies; 11+ messages in thread
From: Greg KH @ 2019-01-08  6:41 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Sasha Levin, Stephen Hemminger, David Hildenbrand, Haiyang Zhang,
	linux-kernel, devel

On Mon, Jan 07, 2019 at 07:38:20PM +0100, Vitaly Kuznetsov wrote:
> (I remember Greg disliked when people were tagging patches for stable@
> themselves, he prefered maintainers deciding if the particular commit
> deserves stable@ or not - but as you have a tree now we may as well have
> different rules :-)

I don't recall ever saying I disliked that, unless people were tagging
stuff that were obviously not stable material.  Which, given the history
of this codebase, was probably the case :)

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
  2019-01-07 13:44   ` Vitaly Kuznetsov
  2019-01-07 17:56     ` Sasha Levin
@ 2019-01-08  9:15     ` David Hildenbrand
  2019-01-08  9:42       ` Vitaly Kuznetsov
  1 sibling, 1 reply; 11+ messages in thread
From: David Hildenbrand @ 2019-01-08  9:15 UTC (permalink / raw)
  To: Vitaly Kuznetsov, devel
  Cc: Sasha Levin, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	linux-kernel, Dexuan Cui

On 07.01.19 14:44, Vitaly Kuznetsov wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 04.01.19 15:19, Vitaly Kuznetsov wrote:
>>> Hyper-V memory hotplug protocol has 2M granularity and in Linux x86 we use
>>> 128M. To deal with it we implement partial section onlining by registering
>>> custom page onlining callback (hv_online_page()). Later, when more memory
>>> arrives we try to online the 'tail' (see hv_bring_pgs_online()).
>>>
>>> It was found that in some cases this 'tail' onlining causes issues:
>>>
>>>  BUG: Bad page state in process kworker/0:2  pfn:109e3a
>>>  page:ffffe08344278e80 count:0 mapcount:1 mapping:0000000000000000 index:0x0
>>>  flags: 0xfffff80000000()
>>>  raw: 000fffff80000000 dead000000000100 dead000000000200 0000000000000000
>>>  raw: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>>  page dumped because: nonzero mapcount
>>>  ...
>>>  Workqueue: events hot_add_req [hv_balloon]
>>>  Call Trace:
>>>   dump_stack+0x5c/0x80
>>>   bad_page.cold.112+0x7f/0xb2
>>>   free_pcppages_bulk+0x4b8/0x690
>>>   free_unref_page+0x54/0x70
>>>   hv_page_online_one+0x5c/0x80 [hv_balloon]
>>>   hot_add_req.cold.24+0x182/0x835 [hv_balloon]
>>>   ...
>>>
>>> Turns out that we now have deferred struct page initialization for memory
>>> hotplug so e.g. memory_block_action() in drivers/base/memory.c does
>>> pages_correctly_probed() check and in that check it avoids inspecting
>>> struct pages and checks sections instead. But in Hyper-V balloon driver we
>>> do PageReserved(pfn_to_page()) check and this is now wrong.
>>>
>>> Switch to checking online_section_nr() instead.
>>>
>>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>> ---
>>>  drivers/hv/hv_balloon.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
>>> index 5301fef16c31..7c6349a50ef1 100644
>>> --- a/drivers/hv/hv_balloon.c
>>> +++ b/drivers/hv/hv_balloon.c
>>> @@ -888,12 +888,14 @@ static unsigned long handle_pg_range(unsigned long pg_start,
>>>  			pfn_cnt -= pgs_ol;
>>>  			/*
>>>  			 * Check if the corresponding memory block is already
>>> -			 * online by checking its last previously backed page.
>>> -			 * In case it is we need to bring rest (which was not
>>> -			 * backed previously) online too.
>>> +			 * online. It is possible to observe struct pages still
>>> +			 * being uninitialized here so check section instead.
>>> +			 * In case the section is online we need to bring the
>>> +			 * rest of pfns (which were not backed previously)
>>> +			 * online too.
>>>  			 */
>>>  			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".

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

> 
> P.S. I still think about bringing mem_hotplug_begin()/done() to
> hv_balloon but that's going to be a separate discussion, here I want to
> have a small fix backportable to stable.
> 
> Thanks,
> 


-- 

Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
  2019-01-08  9:15     ` David Hildenbrand
@ 2019-01-08  9:42       ` Vitaly Kuznetsov
  0 siblings, 0 replies; 11+ messages in thread
From: Vitaly Kuznetsov @ 2019-01-08  9:42 UTC (permalink / raw)
  To: David Hildenbrand, devel
  Cc: Sasha Levin, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	linux-kernel, Dexuan Cui

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
  2019-01-07 18:38       ` Vitaly Kuznetsov
  2019-01-08  6:41         ` Greg KH
@ 2019-01-08  9:47         ` Dan Carpenter
  2019-01-08 14:34           ` Vitaly Kuznetsov
  2019-01-08 21:23         ` Sasha Levin
  2 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2019-01-08  9:47 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Sasha Levin, Stephen Hemminger, David Hildenbrand, Haiyang Zhang,
	linux-kernel, devel

On Mon, Jan 07, 2019 at 07:38:20PM +0100, Vitaly Kuznetsov wrote:
> (I remember Greg disliked when people were tagging patches for stable@
> themselves, he prefered maintainers deciding if the particular commit
> deserves stable@ or not - but as you have a tree now we may as well have
> different rules :-)

You're thinking of Dave Miller.  So far as I know he's the only
maintainer who handles stable that way.  It's a lot of work to do the
things Dave does.  #understatement

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
  2019-01-08  9:47         ` Dan Carpenter
@ 2019-01-08 14:34           ` Vitaly Kuznetsov
  0 siblings, 0 replies; 11+ messages in thread
From: Vitaly Kuznetsov @ 2019-01-08 14:34 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Sasha Levin, Stephen Hemminger, David Hildenbrand, Haiyang Zhang,
	linux-kernel, devel

Dan Carpenter <dan.carpenter@oracle.com> writes:

> On Mon, Jan 07, 2019 at 07:38:20PM +0100, Vitaly Kuznetsov wrote:
>> (I remember Greg disliked when people were tagging patches for stable@
>> themselves, he prefered maintainers deciding if the particular commit
>> deserves stable@ or not - but as you have a tree now we may as well have
>> different rules :-)
>
> You're thinking of Dave Miller.  So far as I know he's the only
> maintainer who handles stable that way.

This may actually be the case, you think "one of the the mighty Linux
overlords" but your fingers type "Greg" instread. Sorry for the
confusion :-)

-- 
Vitaly

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining
  2019-01-07 18:38       ` Vitaly Kuznetsov
  2019-01-08  6:41         ` Greg KH
  2019-01-08  9:47         ` Dan Carpenter
@ 2019-01-08 21:23         ` Sasha Levin
  2 siblings, 0 replies; 11+ messages in thread
From: Sasha Levin @ 2019-01-08 21:23 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: David Hildenbrand, devel, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, linux-kernel, Dexuan Cui

On Mon, Jan 07, 2019 at 07:38:20PM +0100, Vitaly Kuznetsov wrote:
>Sasha Levin <sashal@kernel.org> writes:
>
>> On Mon, Jan 07, 2019 at 02:44:30PM +0100, Vitaly Kuznetsov wrote:
>>>P.S. I still think about bringing mem_hotplug_begin()/done() to
>>>hv_balloon but that's going to be a separate discussion, here I want to
>>>have a small fix backportable to stable.
>>
>> This should probably be marked for stable then :)
>>
>
>Yes, please :-)

Queued up for hyperv-fixes with a -stable tag, thank you!

--
Thanks,
Sasha

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-01-08 21:23 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-04 14:19 [PATCH] hv_balloon: avoid touching uninitialized struct page during tail onlining Vitaly Kuznetsov
2019-01-07 10:21 ` David Hildenbrand
2019-01-07 13:44   ` Vitaly Kuznetsov
2019-01-07 17:56     ` Sasha Levin
2019-01-07 18:38       ` Vitaly Kuznetsov
2019-01-08  6:41         ` Greg KH
2019-01-08  9:47         ` Dan Carpenter
2019-01-08 14:34           ` Vitaly Kuznetsov
2019-01-08 21:23         ` Sasha Levin
2019-01-08  9:15     ` David Hildenbrand
2019-01-08  9:42       ` Vitaly Kuznetsov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.