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

Groups > linux.kernel > #1203845 > unrolled thread

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

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

  [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in no-injection case Wanpeng Li <wanpeng.li@hotmail.com> - 2015-08-10 09:00 +0200
    Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in  no-injection case Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2015-08-10 10:50 +0200
      Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in  no-injection case Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2015-08-10 11:00 +0200
      Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in  no-injection case Wanpeng Li <wanpeng.li@hotmail.com> - 2015-08-10 11:00 +0200
        Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in  no-injection case Wanpeng Li <wanpeng.li@hotmail.com> - 2015-08-10 11:10 +0200
          Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in  no-injection case Wanpeng Li <wanpeng.li@hotmail.com> - 2015-08-10 11:30 +0200
          Re: [PATCH 2/2] mm/hwpoison: fix refcount of THP head page in  no-injection case Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> - 2015-08-10 11:30 +0200

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web