[PATCH 2/2] mm/hwpoison: fix refcount of THP head page in no-injection case

[PATCH 2/2] mm/hwpoison: fix refcount of THP head page in no-injection case

[Wanpeng Li]

Hwpoison injection takes a refcount of target page and another refcount
of head page of THP if the target page is the tail page of a THP. However,
current code doesn't release the refcount of head page if the THP is not 
supported to be injected wrt hwpoison filter. 

Fix it by reducing the refcount of head page if the target page is the tail 
page of a THP and it is not supported to be injected.

Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 mm/hwpoison-inject.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/mm/hwpoison-inject.c b/mm/hwpoison-inject.c
index 5015679..c343a45 100644
--- a/mm/hwpoison-inject.c
+++ b/mm/hwpoison-inject.c
@@ -56,6 +56,8 @@ inject:
 	return memory_failure(pfn, 18, MF_COUNT_INCREASED);
 put_out:
 	put_page(p);
+	if (p != hpage)
+		put_page(hpage);
 	return 0;
 }
 
-- 
1.7.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in no-injection case

[Naoya Horiguchi]

On Mon, Aug 10, 2015 at 02:32:31PM +0800, Wanpeng Li wrote:
> Hwpoison injection takes a refcount of target page and another refcount
> of head page of THP if the target page is the tail page of a THP. However,
> current code doesn't release the refcount of head page if the THP is not 
> supported to be injected wrt hwpoison filter. 
> 
> Fix it by reducing the refcount of head page if the target page is the tail 
> page of a THP and it is not supported to be injected.
> 
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
>  mm/hwpoison-inject.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/hwpoison-inject.c b/mm/hwpoison-inject.c
> index 5015679..c343a45 100644
> --- a/mm/hwpoison-inject.c
> +++ b/mm/hwpoison-inject.c
> @@ -56,6 +56,8 @@ inject:
>  	return memory_failure(pfn, 18, MF_COUNT_INCREASED);
>  put_out:
>  	put_page(p);
> +	if (p != hpage)
> +		put_page(hpage);

Yes, we need this when we inject to a thp tail page and "goto put_out" is
called. But it seems that this code can be called also when injecting error
to a hugetlb tail page and hwpoison_filter() returns non-zero, which is not
expected. Unfortunately simply doing like below

+	if (!PageHuge(p) && p != hpage)
+		put_page(hpage);

doesn't work, because exisiting put_page(p) can release refcount of hugetlb
tail page, while get_hwpoison_page() takes refcount of hugetlb head page.

So I feel that we need put_hwpoison_page() to properly release the refcount
taken by memory error handlers.
I'll post some patch(es) to address this problem this week.

Thanks,
Naoya Horiguchi
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in no-injection case

[Wanpeng Li]

On 8/10/15 4:35 PM, Naoya Horiguchi wrote:
> On Mon, Aug 10, 2015 at 02:32:31PM +0800, Wanpeng Li wrote:
>> Hwpoison injection takes a refcount of target page and another refcount
>> of head page of THP if the target page is the tail page of a THP. However,
>> current code doesn't release the refcount of head page if the THP is not
>> supported to be injected wrt hwpoison filter.
>>
>> Fix it by reducing the refcount of head page if the target page is the tail
>> page of a THP and it is not supported to be injected.
>>
>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> ---
>>   mm/hwpoison-inject.c |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/hwpoison-inject.c b/mm/hwpoison-inject.c
>> index 5015679..c343a45 100644
>> --- a/mm/hwpoison-inject.c
>> +++ b/mm/hwpoison-inject.c
>> @@ -56,6 +56,8 @@ inject:
>>   	return memory_failure(pfn, 18, MF_COUNT_INCREASED);
>>   put_out:
>>   	put_page(p);
>> +	if (p != hpage)
>> +		put_page(hpage);
> Yes, we need this when we inject to a thp tail page and "goto put_out" is
> called. But it seems that this code can be called also when injecting error
> to a hugetlb tail page and hwpoison_filter() returns non-zero, which is not
> expected. Unfortunately simply doing like below
>
> +	if (!PageHuge(p) && p != hpage)
> +		put_page(hpage);
>
> doesn't work, because exisiting put_page(p) can release refcount of hugetlb
> tail page, while get_hwpoison_page() takes refcount of hugetlb head page.
>
> So I feel that we need put_hwpoison_page() to properly release the refcount
> taken by memory error handlers.

Good point. I think I will continue to do it and will post it out soon. :)

Regards,
Wanpeng Li

> I'll post some patch(es) to address this problem this week.
>
> Thanks,
> Naoya Horiguchi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in no-injection case

[Naoya Horiguchi]

On Mon, Aug 10, 2015 at 04:54:39PM +0800, Wanpeng Li wrote:
> On 8/10/15 4:35 PM, Naoya Horiguchi wrote:
> >On Mon, Aug 10, 2015 at 02:32:31PM +0800, Wanpeng Li wrote:
> >>Hwpoison injection takes a refcount of target page and another refcount
> >>of head page of THP if the target page is the tail page of a THP. However,
> >>current code doesn't release the refcount of head page if the THP is not
> >>supported to be injected wrt hwpoison filter.
> >>
> >>Fix it by reducing the refcount of head page if the target page is the tail
> >>page of a THP and it is not supported to be injected.
> >>
> >>Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> >>---
> >>  mm/hwpoison-inject.c |    2 ++
> >>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/mm/hwpoison-inject.c b/mm/hwpoison-inject.c
> >>index 5015679..c343a45 100644
> >>--- a/mm/hwpoison-inject.c
> >>+++ b/mm/hwpoison-inject.c
> >>@@ -56,6 +56,8 @@ inject:
> >>  	return memory_failure(pfn, 18, MF_COUNT_INCREASED);
> >>  put_out:
> >>  	put_page(p);
> >>+	if (p != hpage)
> >>+		put_page(hpage);
> >Yes, we need this when we inject to a thp tail page and "goto put_out" is
> >called. But it seems that this code can be called also when injecting error
> >to a hugetlb tail page and hwpoison_filter() returns non-zero, which is not
> >expected. Unfortunately simply doing like below
> >
> >+	if (!PageHuge(p) && p != hpage)
> >+		put_page(hpage);
> >
> >doesn't work, because exisiting put_page(p) can release refcount of hugetlb
> >tail page, while get_hwpoison_page() takes refcount of hugetlb head page.
> >
> >So I feel that we need put_hwpoison_page() to properly release the refcount
> >taken by memory error handlers.
> 
> Good point. I think I will continue to do it and will post it out soon. :)

Great, thank you :)

Naoya
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in no-injection case

[Wanpeng Li]

On 8/10/15 4:54 PM, Wanpeng Li wrote:
>
>
> On 8/10/15 4:35 PM, Naoya Horiguchi wrote:
>> On Mon, Aug 10, 2015 at 02:32:31PM +0800, Wanpeng Li wrote:
>>> Hwpoison injection takes a refcount of target page and another refcount
>>> of head page of THP if the target page is the tail page of a THP. 
>>> However,
>>> current code doesn't release the refcount of head page if the THP is 
>>> not
>>> supported to be injected wrt hwpoison filter.
>>>
>>> Fix it by reducing the refcount of head page if the target page is 
>>> the tail
>>> page of a THP and it is not supported to be injected.
>>>
>>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>>> ---
>>>   mm/hwpoison-inject.c |    2 ++
>>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/mm/hwpoison-inject.c b/mm/hwpoison-inject.c
>>> index 5015679..c343a45 100644
>>> --- a/mm/hwpoison-inject.c
>>> +++ b/mm/hwpoison-inject.c
>>> @@ -56,6 +56,8 @@ inject:
>>>       return memory_failure(pfn, 18, MF_COUNT_INCREASED);
>>>   put_out:
>>>       put_page(p);
>>> +    if (p != hpage)
>>> +        put_page(hpage);
>> Yes, we need this when we inject to a thp tail page and "goto 
>> put_out" is
>> called. But it seems that this code can be called also when injecting 
>> error
>> to a hugetlb tail page and hwpoison_filter() returns non-zero, which 
>> is not
>> expected. Unfortunately simply doing like below
>>
>> +    if (!PageHuge(p) && p != hpage)
>> +        put_page(hpage);
>>
>> doesn't work, because exisiting put_page(p) can release refcount of 
>> hugetlb
>> tail page, while get_hwpoison_page() takes refcount of hugetlb head 
>> page.
>>
>> So I feel that we need put_hwpoison_page() to properly release the 
>> refcount
>> taken by memory error handlers.
>
> Good point. I think I will continue to do it and will post it out 
> soon. :)

How about something like this:

+void put_hwpoison_page(struct page *page)
+{
+       struct page *head = compound_head(page);
+
+       if (PageHuge(head))
+               goto put_out;
+
+       if (PageTransHuge(head))
+               if (page != head)
+                       put_page(head);
+
+put_out:
+       put_page(page);
+       return;
+}
+

Any comments are welcome, I can update the patch by myself. :)

Regards,
Wanpeng Li

>
> Regards,
> Wanpeng Li
>
>> I'll post some patch(es) to address this problem this week.
>>
>> Thanks,
>> Naoya Horiguchi
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in no-injection case

[Naoya Horiguchi]

On Mon, Aug 10, 2015 at 05:06:25PM +0800, Wanpeng Li wrote:
...
> >>>diff --git a/mm/hwpoison-inject.c b/mm/hwpoison-inject.c
> >>>index 5015679..c343a45 100644
> >>>--- a/mm/hwpoison-inject.c
> >>>+++ b/mm/hwpoison-inject.c
> >>>@@ -56,6 +56,8 @@ inject:
> >>>      return memory_failure(pfn, 18, MF_COUNT_INCREASED);
> >>>  put_out:
> >>>      put_page(p);
> >>>+    if (p != hpage)
> >>>+        put_page(hpage);
> >>Yes, we need this when we inject to a thp tail page and "goto put_out"
> >>is
> >>called. But it seems that this code can be called also when injecting
> >>error
> >>to a hugetlb tail page and hwpoison_filter() returns non-zero, which is
> >>not
> >>expected. Unfortunately simply doing like below
> >>
> >>+    if (!PageHuge(p) && p != hpage)
> >>+        put_page(hpage);
> >>
> >>doesn't work, because exisiting put_page(p) can release refcount of
> >>hugetlb
> >>tail page, while get_hwpoison_page() takes refcount of hugetlb head
> >>page.
> >>
> >>So I feel that we need put_hwpoison_page() to properly release the
> >>refcount
> >>taken by memory error handlers.
> >
> >Good point. I think I will continue to do it and will post it out soon. :)
> 
> How about something like this:
> 
> +void put_hwpoison_page(struct page *page)
> +{
> +       struct page *head = compound_head(page);
> +
> +       if (PageHuge(head))
> +               goto put_out;
> +
> +       if (PageTransHuge(head))
> +               if (page != head)
> +                       put_page(head);
> +
> +put_out:
> +       put_page(page);
> +       return;
> +}
> +

Looks good.

> Any comments are welcome, I can update the patch by myself. :)

Most of callsites of put_page() in memory_failure(), soft_offline_page(),
and unpoison_page() can be replaced with put_hwpoison_page().

Thanks,
Naoya Horiguchi
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in no-injection case

[Wanpeng Li]

On 8/10/15 5:20 PM, Naoya Horiguchi wrote:
> On Mon, Aug 10, 2015 at 05:06:25PM +0800, Wanpeng Li wrote:
> ...
>>>>> diff --git a/mm/hwpoison-inject.c b/mm/hwpoison-inject.c
>>>>> index 5015679..c343a45 100644
>>>>> --- a/mm/hwpoison-inject.c
>>>>> +++ b/mm/hwpoison-inject.c
>>>>> @@ -56,6 +56,8 @@ inject:
>>>>>      return memory_failure(pfn, 18, MF_COUNT_INCREASED);
>>>>>  put_out:
>>>>>      put_page(p);
>>>>> +    if (p != hpage)
>>>>> +        put_page(hpage);
>>>> Yes, we need this when we inject to a thp tail page and "goto put_out"
>>>> is
>>>> called. But it seems that this code can be called also when injecting
>>>> error
>>>> to a hugetlb tail page and hwpoison_filter() returns non-zero, which is
>>>> not
>>>> expected. Unfortunately simply doing like below
>>>>
>>>> +    if (!PageHuge(p) && p != hpage)
>>>> +        put_page(hpage);
>>>>
>>>> doesn't work, because exisiting put_page(p) can release refcount of
>>>> hugetlb
>>>> tail page, while get_hwpoison_page() takes refcount of hugetlb head
>>>> page.
>>>>
>>>> So I feel that we need put_hwpoison_page() to properly release the
>>>> refcount
>>>> taken by memory error handlers.
>>> Good point. I think I will continue to do it and will post it out soon. :)
>> How about something like this:
>>
>> +void put_hwpoison_page(struct page *page)
>> +{
>> +       struct page *head = compound_head(page);
>> +
>> +       if (PageHuge(head))
>> +               goto put_out;
>> +
>> +       if (PageTransHuge(head))
>> +               if (page != head)
>> +                       put_page(head);
>> +
>> +put_out:
>> +       put_page(page);
>> +       return;
>> +}
>> +
> Looks good.
>
>> Any comments are welcome, I can update the patch by myself. :)
> Most of callsites of put_page() in memory_failure(), soft_offline_page(),
> and unpoison_page() can be replaced with put_hwpoison_page().

Cool, thanks for your pointing out. I will do it soon. :)

Regards,
Wanpeng Li

>
> Thanks,
> Naoya Horiguchi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>