Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1370324 > unrolled thread

Re: [PATCH v3 01/16] mm: use put_page to free page instead of putback_lru_page

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 01/16] mm: use put_page to free page instead of  putback_lru_page Minchan Kim <minchan@kernel.org> - 2016-04-04 03:40 +0200
    Re: [PATCH v3 01/16] mm: use put_page to free page instead of  putback_lru_page Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2016-04-04 06:50 +0200
      Re: [PATCH v3 01/16] mm: use put_page to free page instead of  putback_lru_page Vlastimil Babka <vbabka@suse.cz> - 2016-04-04 16:50 +0200
        Re: [PATCH v3 01/16] mm: use put_page to free page instead of  putback_lru_page Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2016-04-05 04:00 +0200
          Re: [PATCH v3 01/16] mm: use put_page to free page instead of  putback_lru_page Vlastimil Babka <vbabka@suse.cz> - 2016-04-05 10:30 +0200
            Re: [PATCH v3 01/16] mm: use put_page to free page instead of  putback_lru_page Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2016-04-06 03:00 +0200
              Re: [PATCH v3 01/16] mm: use put_page to free page instead of  putback_lru_page Vlastimil Babka <vbabka@suse.cz> - 2016-04-06 10:00 +0200

#1370324 — Re: [PATCH v3 01/16] mm: use put_page to free page instead of putback_lru_page

On Fri, Apr 01, 2016 at 02:58:21PM +0200, Vlastimil Babka wrote:
> On 03/30/2016 09:12 AM, Minchan Kim wrote:
> >Procedure of page migration is as follows:
> >
> >First of all, it should isolate a page from LRU and try to
> >migrate the page. If it is successful, it releases the page
> >for freeing. Otherwise, it should put the page back to LRU
> >list.
> >
> >For LRU pages, we have used putback_lru_page for both freeing
> >and putback to LRU list. It's okay because put_page is aware of
> >LRU list so if it releases last refcount of the page, it removes
> >the page from LRU list. However, It makes unnecessary operations
> >(e.g., lru_cache_add, pagevec and flags operations. It would be
> >not significant but no worth to do) and harder to support new
> >non-lru page migration because put_page isn't aware of non-lru
> >page's data structure.
> >
> >To solve the problem, we can add new hook in put_page with
> >PageMovable flags check but it can increase overhead in
> >hot path and needs new locking scheme to stabilize the flag check
> >with put_page.
> >
> >So, this patch cleans it up to divide two semantic(ie, put and putback).
> >If migration is successful, use put_page instead of putback_lru_page and
> >use putback_lru_page only on failure. That makes code more readable
> >and doesn't add overhead in put_page.
> >
> >Comment from Vlastimil
> >"Yeah, and compaction (perhaps also other migration users) has to drain
> >the lru pvec... Getting rid of this stuff is worth even by itself."
> >
> >Cc: Mel Gorman <mgorman@suse.de>
> >Cc: Hugh Dickins <hughd@google.com>
> >Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> >Acked-by: Vlastimil Babka <vbabka@suse.cz>
> >Signed-off-by: Minchan Kim <minchan@kernel.org>
> 
> [...]
> 
> >@@ -974,28 +986,28 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
> >  		list_del(&page->lru);
> >  		dec_zone_page_state(page, NR_ISOLATED_ANON +
> >  				page_is_file_cache(page));
> >-		/* Soft-offlined page shouldn't go through lru cache list */
> >+	}
> >+
> >+	/*
> >+	 * If migration is successful, drop the reference grabbed during
> >+	 * isolation. Otherwise, restore the page to LRU list unless we
> >+	 * want to retry.
> >+	 */
> >+	if (rc == MIGRATEPAGE_SUCCESS) {
> >+		put_page(page);
> >  		if (reason == MR_MEMORY_FAILURE) {
> >-			put_page(page);
> >  			if (!test_set_page_hwpoison(page))
> >  				num_poisoned_pages_inc();
> >-		} else
> >+		}
> 
> Hmm, I didn't notice it previously, or it's due to rebasing, but it
> seems that you restricted the memory failure handling (i.e. setting
> hwpoison) to MIGRATE_SUCCESS, while previously it was done for all
> non-EAGAIN results. I think that goes against the intention of
> hwpoison, which is IIRC to catch and kill the poor process that
> still uses the page?

That's why I Cc'ed Naoya Horiguchi to catch things I might make
mistake.

Thanks for catching it, Vlastimil.
It was my mistake. But in this chance, I looked over hwpoison code and
I saw other places which increases num_poisoned_pages are successful
migration, already freed page and successful invalidated page.
IOW, they are already successful isolated page so I guess it should
increase the count when only successful migration is done?
And when I read memory_failure, it bails out without killing if it
encounters HWPoisoned page so I think it's not for catching and
kill the poor proces.

> 
> Also (but not your fault) the put_page() preceding
> test_set_page_hwpoison(page)) IMHO deserves a comment saying which
> pin we are releasing and which one we still have (hopefully? if I
> read description of da1b13ccfbebe right) otherwise it looks like
> doing something with a page that we just potentially freed.

Yes, while I read the code, I had same question. I think the releasing
refcount is for get_any_page.

Naoya, could you answer above two questions?

Thanks.

> 
> >+	} else {
> >+		if (rc != -EAGAIN)
> >  			putback_lru_page(page);
> >+		if (put_new_page)
> >+			put_new_page(newpage, private);
> >+		else
> >+			put_page(newpage);
> >  	}
> >
> >-	/*
> >-	 * If migration was not successful and there's a freeing callback, use
> >-	 * it.  Otherwise, putback_lru_page() will drop the reference grabbed
> >-	 * during isolation.
> >-	 */
> >-	if (put_new_page)
> >-		put_new_page(newpage, private);
> >-	else if (unlikely(__is_movable_balloon_page(newpage))) {
> >-		/* drop our reference, page already in the balloon */
> >-		put_page(newpage);
> >-	} else
> >-		putback_lru_page(newpage);
> >-
> >  	if (result) {
> >  		if (rc)
> >  			*result = rc;
> >
> 

[toc] | [next] | [standalone]

#1370343

On Mon, Apr 04, 2016 at 10:39:17AM +0900, Minchan Kim wrote:
> On Fri, Apr 01, 2016 at 02:58:21PM +0200, Vlastimil Babka wrote:
> > On 03/30/2016 09:12 AM, Minchan Kim wrote:
> > >Procedure of page migration is as follows:
> > >
> > >First of all, it should isolate a page from LRU and try to
> > >migrate the page. If it is successful, it releases the page
> > >for freeing. Otherwise, it should put the page back to LRU
> > >list.
> > >
> > >For LRU pages, we have used putback_lru_page for both freeing
> > >and putback to LRU list. It's okay because put_page is aware of
> > >LRU list so if it releases last refcount of the page, it removes
> > >the page from LRU list. However, It makes unnecessary operations
> > >(e.g., lru_cache_add, pagevec and flags operations. It would be
> > >not significant but no worth to do) and harder to support new
> > >non-lru page migration because put_page isn't aware of non-lru
> > >page's data structure.
> > >
> > >To solve the problem, we can add new hook in put_page with
> > >PageMovable flags check but it can increase overhead in
> > >hot path and needs new locking scheme to stabilize the flag check
> > >with put_page.
> > >
> > >So, this patch cleans it up to divide two semantic(ie, put and putback).
> > >If migration is successful, use put_page instead of putback_lru_page and
> > >use putback_lru_page only on failure. That makes code more readable
> > >and doesn't add overhead in put_page.
> > >
> > >Comment from Vlastimil
> > >"Yeah, and compaction (perhaps also other migration users) has to drain
> > >the lru pvec... Getting rid of this stuff is worth even by itself."
> > >
> > >Cc: Mel Gorman <mgorman@suse.de>
> > >Cc: Hugh Dickins <hughd@google.com>
> > >Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > >Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > >Signed-off-by: Minchan Kim <minchan@kernel.org>
> > 
> > [...]
> > 
> > >@@ -974,28 +986,28 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page,
> > >  		list_del(&page->lru);
> > >  		dec_zone_page_state(page, NR_ISOLATED_ANON +
> > >  				page_is_file_cache(page));
> > >-		/* Soft-offlined page shouldn't go through lru cache list */
> > >+	}
> > >+
> > >+	/*
> > >+	 * If migration is successful, drop the reference grabbed during
> > >+	 * isolation. Otherwise, restore the page to LRU list unless we
> > >+	 * want to retry.
> > >+	 */
> > >+	if (rc == MIGRATEPAGE_SUCCESS) {
> > >+		put_page(page);
> > >  		if (reason == MR_MEMORY_FAILURE) {
> > >-			put_page(page);
> > >  			if (!test_set_page_hwpoison(page))
> > >  				num_poisoned_pages_inc();
> > >-		} else
> > >+		}
> > 
> > Hmm, I didn't notice it previously, or it's due to rebasing, but it
> > seems that you restricted the memory failure handling (i.e. setting
> > hwpoison) to MIGRATE_SUCCESS, while previously it was done for all
> > non-EAGAIN results. I think that goes against the intention of
> > hwpoison, which is IIRC to catch and kill the poor process that
> > still uses the page?
> 
> That's why I Cc'ed Naoya Horiguchi to catch things I might make
> mistake.
> 
> Thanks for catching it, Vlastimil.
> It was my mistake. But in this chance, I looked over hwpoison code and
> I saw other places which increases num_poisoned_pages are successful
> migration, already freed page and successful invalidated page.
> IOW, they are already successful isolated page so I guess it should
> increase the count when only successful migration is done?

Yes, that's right. When exiting with migration's failure, we shouldn't call
test_set_page_hwpoison or num_poisoned_pages_inc, so current code checking
(rc != -EAGAIN) is simply incorrect. Your change fixes the bug in memory
error handling. Great!

> And when I read memory_failure, it bails out without killing if it
> encounters HWPoisoned page so I think it's not for catching and
> kill the poor proces.
>
> > 
> > Also (but not your fault) the put_page() preceding
> > test_set_page_hwpoison(page)) IMHO deserves a comment saying which
> > pin we are releasing and which one we still have (hopefully? if I
> > read description of da1b13ccfbebe right) otherwise it looks like
> > doing something with a page that we just potentially freed.
>
> Yes, while I read the code, I had same question. I think the releasing
> refcount is for get_any_page.

As the other callers of page migration do, soft_offline_page expects the
migration source page to be freed at this put_page() (no pin remains.)
The refcount released here is from isolate_lru_page() in __soft_offline_page().
(the pin by get_any_page is released by put_hwpoison_page just after it.)

.. yes, doing something just after freeing page looks weird, but that's
how PageHWPoison flag works. IOW, many other page flags are maintained
only during one "allocate-free" life span, but PageHWPoison still does
its job beyond it.

As for commenting, this put_page() is called in any MIGRATEPAGE_SUCCESS
case (regardless of callers), so what we can say here is "we free the
source page here, bypassing LRU list" or something?

Thanks,
Naoya Horiguchi

[toc] | [prev] | [next] | [standalone]

#1370675

On 04/04/2016 06:45 AM, Naoya Horiguchi wrote:
> On Mon, Apr 04, 2016 at 10:39:17AM +0900, Minchan Kim wrote:
>> Thanks for catching it, Vlastimil.
>> It was my mistake. But in this chance, I looked over hwpoison code and
>> I saw other places which increases num_poisoned_pages are successful
>> migration, already freed page and successful invalidated page.
>> IOW, they are already successful isolated page so I guess it should
>> increase the count when only successful migration is done?
> 
> Yes, that's right. When exiting with migration's failure, we shouldn't call
> test_set_page_hwpoison or num_poisoned_pages_inc, so current code checking
> (rc != -EAGAIN) is simply incorrect. Your change fixes the bug in memory
> error handling. Great!

Ah, I see, soft onlining works differently than I thought.

>> And when I read memory_failure, it bails out without killing if it
>> encounters HWPoisoned page so I think it's not for catching and
>> kill the poor proces.
>>
>>>
>>> Also (but not your fault) the put_page() preceding
>>> test_set_page_hwpoison(page)) IMHO deserves a comment saying which
>>> pin we are releasing and which one we still have (hopefully? if I
>>> read description of da1b13ccfbebe right) otherwise it looks like
>>> doing something with a page that we just potentially freed.
>>
>> Yes, while I read the code, I had same question. I think the releasing
>> refcount is for get_any_page.
> 
> As the other callers of page migration do, soft_offline_page expects the
> migration source page to be freed at this put_page() (no pin remains.)
> The refcount released here is from isolate_lru_page() in __soft_offline_page().
> (the pin by get_any_page is released by put_hwpoison_page just after it.)
> 
> .. yes, doing something just after freeing page looks weird, but that's
> how PageHWPoison flag works. IOW, many other page flags are maintained
> only during one "allocate-free" life span, but PageHWPoison still does
> its job beyond it.

But what prevents the page from being allocated again between put_page()
and test_set_page_hwpoison()? In that case we would be marking page
poisoned while still in use, which is the same as marking it while still
in use after a failed migration?

(Also, which part prevents pages with PageHWPoison to be allocated
again, anyway? I can't find it and test_set_page_hwpoison() doesn't
remove from buddy freelists).

Thanks.

> As for commenting, this put_page() is called in any MIGRATEPAGE_SUCCESS
> case (regardless of callers), so what we can say here is "we free the
> source page here, bypassing LRU list" or something?
> 
> Thanks,
> Naoya Horiguchi
> 

[toc] | [prev] | [next] | [standalone]

#1371117

On Mon, Apr 04, 2016 at 04:46:31PM +0200, Vlastimil Babka wrote:
> On 04/04/2016 06:45 AM, Naoya Horiguchi wrote:
> > On Mon, Apr 04, 2016 at 10:39:17AM +0900, Minchan Kim wrote:
...
> >>>
> >>> Also (but not your fault) the put_page() preceding
> >>> test_set_page_hwpoison(page)) IMHO deserves a comment saying which
> >>> pin we are releasing and which one we still have (hopefully? if I
> >>> read description of da1b13ccfbebe right) otherwise it looks like
> >>> doing something with a page that we just potentially freed.
> >>
> >> Yes, while I read the code, I had same question. I think the releasing
> >> refcount is for get_any_page.
> > 
> > As the other callers of page migration do, soft_offline_page expects the
> > migration source page to be freed at this put_page() (no pin remains.)
> > The refcount released here is from isolate_lru_page() in __soft_offline_page().
> > (the pin by get_any_page is released by put_hwpoison_page just after it.)
> > 
> > .. yes, doing something just after freeing page looks weird, but that's
> > how PageHWPoison flag works. IOW, many other page flags are maintained
> > only during one "allocate-free" life span, but PageHWPoison still does
> > its job beyond it.
> 
> But what prevents the page from being allocated again between put_page()
> and test_set_page_hwpoison()? In that case we would be marking page
> poisoned while still in use, which is the same as marking it while still
> in use after a failed migration?

Actually nothing prevents that race. But I think that the result of the race
is that the error page can be reused for allocation, which results in killing
processes at page fault time. Soft offline is kind of mild/precautious thing
(for correctable errors that don't require immediate handling), so killing
processes looks to me an overkill. And marking hwpoison means that we can no
longer do retry from userspace.

And another practical thing is the race with unpoison_memory() as described
in commit da1b13ccfbebe. unpoison_memory() properly works only for properly
poisoned pages, so doing unpoison for in-use hwpoisoned pages is fragile.
That's why I'd like to avoid setting PageHWPoison for in-use pages if possible.

> (Also, which part prevents pages with PageHWPoison to be allocated
> again, anyway? I can't find it and test_set_page_hwpoison() doesn't
> remove from buddy freelists).

check_new_page() in mm/page_alloc.c should prevent reallocation of PageHWPoison.
As you pointed out, memory error handler doens't remove it from buddy freelists.


BTW, it might be a bit off-topic, but recently I felt that check_new_page()
might be improvable, because when check_new_page() returns 1, the whole buddy
block (not only the bad page) seems to be leaked from buddy freelist.
For example, if thp (order 9) is requested, and PageHWPoison (or any other
types of bad pages) is found in an order 9 block, all 512 page are discarded.
Unpoison can't bring it back to buddy.
So, some code to split buddy block including bad page (and recovering code from
unpoison) might be helpful, although that's another story ...

Thanks,
Naoya Horiguchi

[toc] | [prev] | [next] | [standalone]

#1371315

On 04/05/2016 03:54 AM, Naoya Horiguchi wrote:
> On Mon, Apr 04, 2016 at 04:46:31PM +0200, Vlastimil Babka wrote:
>> On 04/04/2016 06:45 AM, Naoya Horiguchi wrote:
>>> On Mon, Apr 04, 2016 at 10:39:17AM +0900, Minchan Kim wrote:
> ...
>>>>>
>>>>> Also (but not your fault) the put_page() preceding
>>>>> test_set_page_hwpoison(page)) IMHO deserves a comment saying which
>>>>> pin we are releasing and which one we still have (hopefully? if I
>>>>> read description of da1b13ccfbebe right) otherwise it looks like
>>>>> doing something with a page that we just potentially freed.
>>>>
>>>> Yes, while I read the code, I had same question. I think the releasing
>>>> refcount is for get_any_page.
>>>
>>> As the other callers of page migration do, soft_offline_page expects the
>>> migration source page to be freed at this put_page() (no pin remains.)
>>> The refcount released here is from isolate_lru_page() in __soft_offline_page().
>>> (the pin by get_any_page is released by put_hwpoison_page just after it.)
>>>
>>> .. yes, doing something just after freeing page looks weird, but that's
>>> how PageHWPoison flag works. IOW, many other page flags are maintained
>>> only during one "allocate-free" life span, but PageHWPoison still does
>>> its job beyond it.
>>
>> But what prevents the page from being allocated again between put_page()
>> and test_set_page_hwpoison()? In that case we would be marking page
>> poisoned while still in use, which is the same as marking it while still
>> in use after a failed migration?
> 
> Actually nothing prevents that race. But I think that the result of the race
> is that the error page can be reused for allocation, which results in killing
> processes at page fault time. Soft offline is kind of mild/precautious thing
> (for correctable errors that don't require immediate handling), so killing
> processes looks to me an overkill. And marking hwpoison means that we can no
> longer do retry from userspace.

So you agree that this race is a bug? It may turn a soft-offline attempt
into a killed process. In that case we should fix it the same as we are
fixing the failed migration case. Maybe it will be just enough to switch
the test_set_page_hwpoison() and put_page() calls?

> And another practical thing is the race with unpoison_memory() as described
> in commit da1b13ccfbebe. unpoison_memory() properly works only for properly
> poisoned pages, so doing unpoison for in-use hwpoisoned pages is fragile.
> That's why I'd like to avoid setting PageHWPoison for in-use pages if possible.
> 
>> (Also, which part prevents pages with PageHWPoison to be allocated
>> again, anyway? I can't find it and test_set_page_hwpoison() doesn't
>> remove from buddy freelists).
> 
> check_new_page() in mm/page_alloc.c should prevent reallocation of PageHWPoison.
> As you pointed out, memory error handler doens't remove it from buddy freelists.

Oh, I see. It's using __PG_HWPOISON wrapper, so I didn't notice it when
searching. In any case that results in a bad_page() warning, right? Is
it desirable for a soft-offlined page? If we didn't free poisoned pages
to buddy system, they wouldn't trigger this warning.

> BTW, it might be a bit off-topic, but recently I felt that check_new_page()
> might be improvable, because when check_new_page() returns 1, the whole buddy
> block (not only the bad page) seems to be leaked from buddy freelist.
> For example, if thp (order 9) is requested, and PageHWPoison (or any other
> types of bad pages) is found in an order 9 block, all 512 page are discarded.
> Unpoison can't bring it back to buddy.
> So, some code to split buddy block including bad page (and recovering code from
> unpoison) might be helpful, although that's another story ...

Hm sounds like another argument for not freeing the page to buddy lists
in the first place. Maybe a hook in free_pages_check()?

> Thanks,
> Naoya Horiguchi
> 

[toc] | [prev] | [next] | [standalone]

#1372111

On Tue, Apr 05, 2016 at 10:20:50AM +0200, Vlastimil Babka wrote:
> On 04/05/2016 03:54 AM, Naoya Horiguchi wrote:
> > On Mon, Apr 04, 2016 at 04:46:31PM +0200, Vlastimil Babka wrote:
> >> On 04/04/2016 06:45 AM, Naoya Horiguchi wrote:
> >>> On Mon, Apr 04, 2016 at 10:39:17AM +0900, Minchan Kim wrote:
> > ...
> >>>>>
> >>>>> Also (but not your fault) the put_page() preceding
> >>>>> test_set_page_hwpoison(page)) IMHO deserves a comment saying which
> >>>>> pin we are releasing and which one we still have (hopefully? if I
> >>>>> read description of da1b13ccfbebe right) otherwise it looks like
> >>>>> doing something with a page that we just potentially freed.
> >>>>
> >>>> Yes, while I read the code, I had same question. I think the releasing
> >>>> refcount is for get_any_page.
> >>>
> >>> As the other callers of page migration do, soft_offline_page expects the
> >>> migration source page to be freed at this put_page() (no pin remains.)
> >>> The refcount released here is from isolate_lru_page() in __soft_offline_page().
> >>> (the pin by get_any_page is released by put_hwpoison_page just after it.)
> >>>
> >>> .. yes, doing something just after freeing page looks weird, but that's
> >>> how PageHWPoison flag works. IOW, many other page flags are maintained
> >>> only during one "allocate-free" life span, but PageHWPoison still does
> >>> its job beyond it.
> >>
> >> But what prevents the page from being allocated again between put_page()
> >> and test_set_page_hwpoison()? In that case we would be marking page
> >> poisoned while still in use, which is the same as marking it while still
> >> in use after a failed migration?
> > 
> > Actually nothing prevents that race. But I think that the result of the race
> > is that the error page can be reused for allocation, which results in killing
> > processes at page fault time. Soft offline is kind of mild/precautious thing
> > (for correctable errors that don't require immediate handling), so killing
> > processes looks to me an overkill. And marking hwpoison means that we can no
> > longer do retry from userspace.
> 
> So you agree that this race is a bug? It may turn a soft-offline attempt
> into a killed process. In that case we should fix it the same as we are
> fixing the failed migration case.

I agree, it's a bug, although rare and non-critical.

> Maybe it will be just enough to switch
> the test_set_page_hwpoison() and put_page() calls?

Unfortunately that restores the other race with unpoison (described below.)
Sorry for my bad/unclear statements, these races seems exclusive and a compatible
solution is not found, so I prioritized fixing the latter one by comparing
severity (the latter causes kernel crash,) which led to the current code.

> > And another practical thing is the race with unpoison_memory() as described
> > in commit da1b13ccfbebe. unpoison_memory() properly works only for properly
> > poisoned pages, so doing unpoison for in-use hwpoisoned pages is fragile.
> > That's why I'd like to avoid setting PageHWPoison for in-use pages if possible.
> > 
> >> (Also, which part prevents pages with PageHWPoison to be allocated
> >> again, anyway? I can't find it and test_set_page_hwpoison() doesn't
> >> remove from buddy freelists).
> > 
> > check_new_page() in mm/page_alloc.c should prevent reallocation of PageHWPoison.
> > As you pointed out, memory error handler doens't remove it from buddy freelists.
> 
> Oh, I see. It's using __PG_HWPOISON wrapper, so I didn't notice it when
> searching. In any case that results in a bad_page() warning, right? Is
> it desirable for a soft-offlined page?

That's right, and the bad_page warning might be too strong for soft offlining.
We can't tell which of memory_failure/soft_offline_page a PageHWPoison came
from, but users can find other lines in dmesg which should tell that.
And memory error events can hit buddy pages directly, in that case we still
need the check in check_new_page().

> If we didn't free poisoned pages
> to buddy system, they wouldn't trigger this warning.

Actually, we didn't free at commit add05cecef80 ("mm: soft-offline: don't free
target page in successful page migration"), but that's was reverted in
commit f4c18e6f7b5b ("mm: check __PG_HWPOISON separately from PAGE_FLAGS_CHECK_AT_*").
Now I start thinking the revert was a bad decision, so I'll dig this problem again.

> > BTW, it might be a bit off-topic, but recently I felt that check_new_page()
> > might be improvable, because when check_new_page() returns 1, the whole buddy
> > block (not only the bad page) seems to be leaked from buddy freelist.
> > For example, if thp (order 9) is requested, and PageHWPoison (or any other
> > types of bad pages) is found in an order 9 block, all 512 page are discarded.
> > Unpoison can't bring it back to buddy.
> > So, some code to split buddy block including bad page (and recovering code from
> > unpoison) might be helpful, although that's another story ...
> 
> Hm sounds like another argument for not freeing the page to buddy lists
> in the first place. Maybe a hook in free_pages_check()?

Sounds a good idea. I'll try it, too.

Thanks,
Naoya Horiguchi

[toc] | [prev] | [next] | [standalone]

#1372267

On 04/06/2016 02:54 AM, Naoya Horiguchi wrote:
> On Tue, Apr 05, 2016 at 10:20:50AM +0200, Vlastimil Babka wrote:
>>
>> So you agree that this race is a bug? It may turn a soft-offline attempt
>> into a killed process. In that case we should fix it the same as we are
>> fixing the failed migration case.
> 
> I agree, it's a bug, although rare and non-critical.
> 
>> Maybe it will be just enough to switch
>> the test_set_page_hwpoison() and put_page() calls?
> 
> Unfortunately that restores the other race with unpoison (described below.)
> Sorry for my bad/unclear statements, these races seems exclusive and a compatible
> solution is not found, so I prioritized fixing the latter one by comparing
> severity (the latter causes kernel crash,) which led to the current code.

Ah, I see. However unpoison is a functionality just for stress-testing,
and not expected to be used in production, right? So it's somewhat
unfortunate trade-off with danger of soft-offlining killing an unrelated
process.

>>> And another practical thing is the race with unpoison_memory() as described
>>> in commit da1b13ccfbebe. unpoison_memory() properly works only for properly
>>> poisoned pages, so doing unpoison for in-use hwpoisoned pages is fragile.
>>> That's why I'd like to avoid setting PageHWPoison for in-use pages if possible.
>>>
>>>> (Also, which part prevents pages with PageHWPoison to be allocated
>>>> again, anyway? I can't find it and test_set_page_hwpoison() doesn't
>>>> remove from buddy freelists).
>>>
>>> check_new_page() in mm/page_alloc.c should prevent reallocation of PageHWPoison.
>>> As you pointed out, memory error handler doens't remove it from buddy freelists.
>>
>> Oh, I see. It's using __PG_HWPOISON wrapper, so I didn't notice it when
>> searching. In any case that results in a bad_page() warning, right? Is
>> it desirable for a soft-offlined page?
> 
> That's right, and the bad_page warning might be too strong for soft offlining.
> We can't tell which of memory_failure/soft_offline_page a PageHWPoison came
> from, but users can find other lines in dmesg which should tell that.
> And memory error events can hit buddy pages directly, in that case we still
> need the check in check_new_page().

Ah, ok.

>> If we didn't free poisoned pages
>> to buddy system, they wouldn't trigger this warning.
> 
> Actually, we didn't free at commit add05cecef80 ("mm: soft-offline: don't free
> target page in successful page migration"), but that's was reverted in
> commit f4c18e6f7b5b ("mm: check __PG_HWPOISON separately from PAGE_FLAGS_CHECK_AT_*").
> Now I start thinking the revert was a bad decision, so I'll dig this problem again.

Good.

>>> BTW, it might be a bit off-topic, but recently I felt that check_new_page()
>>> might be improvable, because when check_new_page() returns 1, the whole buddy
>>> block (not only the bad page) seems to be leaked from buddy freelist.
>>> For example, if thp (order 9) is requested, and PageHWPoison (or any other
>>> types of bad pages) is found in an order 9 block, all 512 page are discarded.
>>> Unpoison can't bring it back to buddy.
>>> So, some code to split buddy block including bad page (and recovering code from
>>> unpoison) might be helpful, although that's another story ...
>>
>> Hm sounds like another argument for not freeing the page to buddy lists
>> in the first place. Maybe a hook in free_pages_check()?
> 
> Sounds a good idea. I'll try it, too.

So what I think could hopefully work is to replace the put_page() after
migration with a hwpoison-specific construct that does something like:

if (put_page_testzero(page))
     if (test_set_page_hwpoison()) ...
     __put_page()

With some more thought about what other parts of put_page() apply - how
to handle compound pages and zone-device pages.

That should hopefully be the safest course. When put_page_testzero()
succeeds, there should be no other (current of near-future) users of the
page, and we can still do whatever we need before releasing to
__put_page(). I.e. set the HWPoison flag, and maybe combine this with
modification to free_pages_check() to divert it from becoming a buddy page.

It should be even safer than the current "put_page();
test_set_page_hwpoison();" approach in that we are currently not
guaranteed that the put_page() is indeed releasing the last pin, but we
set HWPoison in any case. Although we have just migrated the page away,
there might be a pfn scanner holding its pin and checking the page.
Hopefully no such scanner has a path that would break on HWPoison flag,
but I don't know. By not setting the HWpoison when we don't succeed
put_page_testzero(), we are safer. It's true the page might stay
unpoisoned due to a temporary pin, but the process data was migrated
away which is the important part, and userspace can retry anyway?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web