Linux Virtualization - Apr 2016 - [PATCH v3 01/16] mm: use put_page to free page instead of putback_lru_page

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

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
>

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