* [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure
@ 2019-11-18 8:20 Wei Yang
2019-11-18 8:20 ` [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage Wei Yang
2019-11-19 12:23 ` [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure David Hildenbrand
0 siblings, 2 replies; 13+ messages in thread
From: Wei Yang @ 2019-11-18 8:20 UTC (permalink / raw)
To: n-horiguchi, akpm; +Cc: linux-mm, linux-kernel, Wei Yang
PageHuge is handled by memory_failure_hugetlb(), so this case could be
removed.
Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
mm/memory-failure.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 3151c87dff73..392ac277b17d 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1359,10 +1359,7 @@ int memory_failure(unsigned long pfn, int flags)
* page_remove_rmap() in try_to_unmap_one(). So to determine page status
* correctly, we save a copy of the page flags at this time.
*/
- if (PageHuge(p))
- page_flags = hpage->flags;
- else
- page_flags = p->flags;
+ page_flags = p->flags;
/*
* unpoison always clear PG_hwpoison inside page lock
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage
2019-11-18 8:20 [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure Wei Yang
@ 2019-11-18 8:20 ` Wei Yang
2019-11-20 15:07 ` David Hildenbrand
2019-11-19 12:23 ` [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure David Hildenbrand
1 sibling, 1 reply; 13+ messages in thread
From: Wei Yang @ 2019-11-18 8:20 UTC (permalink / raw)
To: n-horiguchi, akpm; +Cc: linux-mm, linux-kernel, Wei Yang
hpage is not changed.
Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
mm/memory-failure.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 392ac277b17d..9784f4339ae7 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
}
unlock_page(p);
VM_BUG_ON_PAGE(!page_count(p), p);
- hpage = compound_head(p);
}
/*
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure
2019-11-18 8:20 [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure Wei Yang
2019-11-18 8:20 ` [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage Wei Yang
@ 2019-11-19 12:23 ` David Hildenbrand
2019-11-20 0:46 ` Wei Yang
1 sibling, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2019-11-19 12:23 UTC (permalink / raw)
To: Wei Yang, n-horiguchi, akpm; +Cc: linux-mm, linux-kernel
On 18.11.19 09:20, Wei Yang wrote:
> PageHuge is handled by memory_failure_hugetlb(), so this case could be
> removed.
>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
> mm/memory-failure.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 3151c87dff73..392ac277b17d 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1359,10 +1359,7 @@ int memory_failure(unsigned long pfn, int flags)
> * page_remove_rmap() in try_to_unmap_one(). So to determine page status
> * correctly, we save a copy of the page flags at this time.
> */
> - if (PageHuge(p))
> - page_flags = hpage->flags;
> - else
> - page_flags = p->flags;
> + page_flags = p->flags;
>
> /*
> * unpoison always clear PG_hwpoison inside page lock
>
I somewhat miss a proper explanation why this is safe to do. We access
page flags here, so why is it safe to refer to the ones of the sub-page?
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure
2019-11-19 12:23 ` [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure David Hildenbrand
@ 2019-11-20 0:46 ` Wei Yang
2019-11-20 15:04 ` David Hildenbrand
0 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2019-11-20 0:46 UTC (permalink / raw)
To: David Hildenbrand; +Cc: Wei Yang, n-horiguchi, akpm, linux-mm, linux-kernel
On Tue, Nov 19, 2019 at 01:23:54PM +0100, David Hildenbrand wrote:
>On 18.11.19 09:20, Wei Yang wrote:
>> PageHuge is handled by memory_failure_hugetlb(), so this case could be
>> removed.
>>
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>> mm/memory-failure.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 3151c87dff73..392ac277b17d 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1359,10 +1359,7 @@ int memory_failure(unsigned long pfn, int flags)
>> * page_remove_rmap() in try_to_unmap_one(). So to determine page status
>> * correctly, we save a copy of the page flags at this time.
>> */
>> - if (PageHuge(p))
>> - page_flags = hpage->flags;
>> - else
>> - page_flags = p->flags;
>> + page_flags = p->flags;
>> /*
>> * unpoison always clear PG_hwpoison inside page lock
>>
>
>I somewhat miss a proper explanation why this is safe to do. We access page
>flags here, so why is it safe to refer to the ones of the sub-page?
>
Hi, David
I think your comment is on this line:
page_flags = p->flags;
Maybe we need to use this:
page_flags = hpage->flags;
And use hpage in the following or even the whole function?
While one thing interesting is not all "compound page" is PageCompound. For
some sub-page, we can't get the correct head. This means we may just check on
the sub-page.
>--
>
>Thanks,
>
>David / dhildenb
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure
2019-11-20 0:46 ` Wei Yang
@ 2019-11-20 15:04 ` David Hildenbrand
2019-11-21 1:07 ` Wei Yang
0 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2019-11-20 15:04 UTC (permalink / raw)
To: Wei Yang
Cc: n-horiguchi, akpm, linux-mm, linux-kernel, James Morse, Naoya Horiguchi
On 20.11.19 01:46, Wei Yang wrote:
> On Tue, Nov 19, 2019 at 01:23:54PM +0100, David Hildenbrand wrote:
>> On 18.11.19 09:20, Wei Yang wrote:
>>> PageHuge is handled by memory_failure_hugetlb(), so this case could be
>>> removed.
>>>
>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>> ---
>>> mm/memory-failure.c | 5 +----
>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>> index 3151c87dff73..392ac277b17d 100644
>>> --- a/mm/memory-failure.c
>>> +++ b/mm/memory-failure.c
>>> @@ -1359,10 +1359,7 @@ int memory_failure(unsigned long pfn, int flags)
>>> * page_remove_rmap() in try_to_unmap_one(). So to determine page status
>>> * correctly, we save a copy of the page flags at this time.
>>> */
>>> - if (PageHuge(p))
>>> - page_flags = hpage->flags;
>>> - else
>>> - page_flags = p->flags;
>>> + page_flags = p->flags;
>>> /*
>>> * unpoison always clear PG_hwpoison inside page lock
>>>
>>
>> I somewhat miss a proper explanation why this is safe to do. We access page
>> flags here, so why is it safe to refer to the ones of the sub-page?
>>
>
> Hi, David
>
> I think your comment is on this line:
>
> page_flags = p->flags;
>
> Maybe we need to use this:
>
> page_flags = hpage->flags;
>
> And use hpage in the following or even the whole function?
>
> While one thing interesting is not all "compound page" is PageCompound. For
> some sub-page, we can't get the correct head. This means we may just check on
> the sub-page.
Oh wait, I think I missed the check right at the beginning of this
function, sorry :/
Sooo, memory_failure_hugetlb() was introduced by
commit 761ad8d7c7b5485bb66fd5bccb58a891fe784544
Author: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Mon Jul 10 15:47:47 2017 -0700
mm: hwpoison: introduce memory_failure_hugetlb()
and essentially ripped out all PageHuge() checks *except* this check.
This check was introduced in
commit 7258ae5c5a2ce2f5969e8b18b881be40ab55433d
Author: James Morse <james.morse@arm.com>
Date: Fri Jun 16 14:02:29 2017 -0700
mm/memory-failure.c: use compound_head() flags for huge pages
Maybe that was just a merge oddity as both commits are only ~1month
apart. IOW, I think Naoya's patch forgot to rip it out.
Can we make this clear in the patch description like "This is dead code
that cannot be reached after commit 761ad8d7c7b5 ("mm: hwpoison:
introduce memory_failure_hugetlb()")"
I assume Andrew can fix up when applying
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage
2019-11-18 8:20 ` [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage Wei Yang
@ 2019-11-20 15:07 ` David Hildenbrand
2019-11-21 1:05 ` Wei Yang
2019-12-02 22:28 ` Wei Yang
0 siblings, 2 replies; 13+ messages in thread
From: David Hildenbrand @ 2019-11-20 15:07 UTC (permalink / raw)
To: Wei Yang, n-horiguchi, akpm; +Cc: linux-mm, linux-kernel
On 18.11.19 09:20, Wei Yang wrote:
> hpage is not changed.
>
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
> mm/memory-failure.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 392ac277b17d..9784f4339ae7 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
> }
> unlock_page(p);
> VM_BUG_ON_PAGE(!page_count(p), p);
> - hpage = compound_head(p);
> }
>
> /*
>
I am *absolutely* no transparent huge page expert (sorry :) ), but won't
the split_huge_page(p) eventually split the compound page, such that
compound_head(p) will return something else after that call?
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage
2019-11-20 15:07 ` David Hildenbrand
@ 2019-11-21 1:05 ` Wei Yang
2019-12-02 22:28 ` Wei Yang
1 sibling, 0 replies; 13+ messages in thread
From: Wei Yang @ 2019-11-21 1:05 UTC (permalink / raw)
To: David Hildenbrand; +Cc: Wei Yang, n-horiguchi, akpm, linux-mm, linux-kernel
On Wed, Nov 20, 2019 at 04:07:38PM +0100, David Hildenbrand wrote:
>On 18.11.19 09:20, Wei Yang wrote:
>> hpage is not changed.
>>
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>> mm/memory-failure.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 392ac277b17d..9784f4339ae7 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
>> }
>> unlock_page(p);
>> VM_BUG_ON_PAGE(!page_count(p), p);
>> - hpage = compound_head(p);
>> }
>> /*
>>
>
>I am *absolutely* no transparent huge page expert (sorry :) ), but won't the
>split_huge_page(p) eventually split the compound page, such that
>compound_head(p) will return something else after that call?
>
ok, you may get some point. I lost some concentration here.
Thanks
>--
>
>Thanks,
>
>David / dhildenb
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure
2019-11-20 15:04 ` David Hildenbrand
@ 2019-11-21 1:07 ` Wei Yang
0 siblings, 0 replies; 13+ messages in thread
From: Wei Yang @ 2019-11-21 1:07 UTC (permalink / raw)
To: David Hildenbrand
Cc: Wei Yang, n-horiguchi, akpm, linux-mm, linux-kernel, James Morse
On Wed, Nov 20, 2019 at 04:04:44PM +0100, David Hildenbrand wrote:
>On 20.11.19 01:46, Wei Yang wrote:
>> On Tue, Nov 19, 2019 at 01:23:54PM +0100, David Hildenbrand wrote:
>> > On 18.11.19 09:20, Wei Yang wrote:
>> > > PageHuge is handled by memory_failure_hugetlb(), so this case could be
>> > > removed.
>> > >
>> > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> > > ---
>> > > mm/memory-failure.c | 5 +----
>> > > 1 file changed, 1 insertion(+), 4 deletions(-)
>> > >
>> > > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> > > index 3151c87dff73..392ac277b17d 100644
>> > > --- a/mm/memory-failure.c
>> > > +++ b/mm/memory-failure.c
>> > > @@ -1359,10 +1359,7 @@ int memory_failure(unsigned long pfn, int flags)
>> > > * page_remove_rmap() in try_to_unmap_one(). So to determine page status
>> > > * correctly, we save a copy of the page flags at this time.
>> > > */
>> > > - if (PageHuge(p))
>> > > - page_flags = hpage->flags;
>> > > - else
>> > > - page_flags = p->flags;
>> > > + page_flags = p->flags;
>> > > /*
>> > > * unpoison always clear PG_hwpoison inside page lock
>> > >
>> >
>> > I somewhat miss a proper explanation why this is safe to do. We access page
>> > flags here, so why is it safe to refer to the ones of the sub-page?
>> >
>>
>> Hi, David
>>
>> I think your comment is on this line:
>>
>> page_flags = p->flags;
>>
>> Maybe we need to use this:
>>
>> page_flags = hpage->flags;
>>
>> And use hpage in the following or even the whole function?
>>
>> While one thing interesting is not all "compound page" is PageCompound. For
>> some sub-page, we can't get the correct head. This means we may just check on
>> the sub-page.
>
>Oh wait, I think I missed the check right at the beginning of this function,
>sorry :/
>
>Sooo, memory_failure_hugetlb() was introduced by
>
>commit 761ad8d7c7b5485bb66fd5bccb58a891fe784544
>Author: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>Date: Mon Jul 10 15:47:47 2017 -0700
>
> mm: hwpoison: introduce memory_failure_hugetlb()
>
>and essentially ripped out all PageHuge() checks *except* this check. This
>check was introduced in
>
>commit 7258ae5c5a2ce2f5969e8b18b881be40ab55433d
>Author: James Morse <james.morse@arm.com>
>Date: Fri Jun 16 14:02:29 2017 -0700
>
> mm/memory-failure.c: use compound_head() flags for huge pages
>
>
>Maybe that was just a merge oddity as both commits are only ~1month apart.
>IOW, I think Naoya's patch forgot to rip it out.
>
>
>Can we make this clear in the patch description like "This is dead code that
>cannot be reached after commit 761ad8d7c7b5 ("mm: hwpoison: introduce
>memory_failure_hugetlb()")"
>
>I assume Andrew can fix up when applying
>
That's very helpful. Thanks for your time sincerely.
>Reviewed-by: David Hildenbrand <david@redhat.com>
>
>--
>
>Thanks,
>
>David / dhildenb
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage
2019-11-20 15:07 ` David Hildenbrand
2019-11-21 1:05 ` Wei Yang
@ 2019-12-02 22:28 ` Wei Yang
2019-12-05 15:06 ` David Hildenbrand
1 sibling, 1 reply; 13+ messages in thread
From: Wei Yang @ 2019-12-02 22:28 UTC (permalink / raw)
To: David Hildenbrand; +Cc: Wei Yang, n-horiguchi, akpm, linux-mm, linux-kernel
On Wed, Nov 20, 2019 at 04:07:38PM +0100, David Hildenbrand wrote:
>On 18.11.19 09:20, Wei Yang wrote:
>> hpage is not changed.
>>
>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>> ---
>> mm/memory-failure.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 392ac277b17d..9784f4339ae7 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
>> }
>> unlock_page(p);
>> VM_BUG_ON_PAGE(!page_count(p), p);
>> - hpage = compound_head(p);
>> }
>> /*
>>
>
>I am *absolutely* no transparent huge page expert (sorry :) ), but won't the
>split_huge_page(p) eventually split the compound page, such that
>compound_head(p) will return something else after that call?
>
Hi, David
Took sometime to look into the code and re-think about it. Found maybe we can
simplify this in another way.
First, code touches here means split_huge_page() succeeds and "p" is now a PTE
page. So compound_head(p) == p.
Then let's look at who will use hpage in the following function. There are two
uses in current upstream:
* page_flags calculation
* hwpoison_user_mappings()
The first one would be removed in next patch since PageHuge is handled at the
beginning.
And in the second place, comment says if split succeeds, hpage points to page
"p".
After all, we don't need to re-calculate hpage after split, and just replace
hpage in hwpoison_user_mappings() with p is enough.
>--
>
>Thanks,
>
>David / dhildenb
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage
2019-12-02 22:28 ` Wei Yang
@ 2019-12-05 15:06 ` David Hildenbrand
2019-12-06 1:48 ` Wei Yang
0 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2019-12-05 15:06 UTC (permalink / raw)
To: Wei Yang; +Cc: Wei Yang, n-horiguchi, akpm, linux-mm, linux-kernel
On 02.12.19 23:28, Wei Yang wrote:
> On Wed, Nov 20, 2019 at 04:07:38PM +0100, David Hildenbrand wrote:
>> On 18.11.19 09:20, Wei Yang wrote:
>>> hpage is not changed.
>>>
>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>> ---
>>> mm/memory-failure.c | 1 -
>>> 1 file changed, 1 deletion(-)
>>>
>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>> index 392ac277b17d..9784f4339ae7 100644
>>> --- a/mm/memory-failure.c
>>> +++ b/mm/memory-failure.c
>>> @@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
>>> }
>>> unlock_page(p);
>>> VM_BUG_ON_PAGE(!page_count(p), p);
>>> - hpage = compound_head(p);
>>> }
>>> /*
>>>
>>
>> I am *absolutely* no transparent huge page expert (sorry :) ), but won't the
>> split_huge_page(p) eventually split the compound page, such that
>> compound_head(p) will return something else after that call?
>>
>
> Hi, David
>
> Took sometime to look into the code and re-think about it. Found maybe we can
> simplify this in another way.
>
> First, code touches here means split_huge_page() succeeds and "p" is now a PTE
> page. So compound_head(p) == p.
While this would also be my intuition, I can't state that this is
guaranteed to be the case (IOW, I did not check the code/documentation) :)
>
> Then let's look at who will use hpage in the following function. There are two
> uses in current upstream:
>
> * page_flags calculation
> * hwpoison_user_mappings()
>
> The first one would be removed in next patch since PageHuge is handled at the
> beginning.
>
> And in the second place, comment says if split succeeds, hpage points to page
> "p".
>
> After all, we don't need to re-calculate hpage after split, and just replace
> hpage in hwpoison_user_mappings() with p is enough.
That assumption would only be true in case all compound pages at this
point are transparent huge pages, no? AFAIK that is not necessarily
true. Or am I missing something?
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage
2019-12-05 15:06 ` David Hildenbrand
@ 2019-12-06 1:48 ` Wei Yang
2020-01-08 12:20 ` David Hildenbrand
0 siblings, 1 reply; 13+ messages in thread
From: Wei Yang @ 2019-12-06 1:48 UTC (permalink / raw)
To: David Hildenbrand
Cc: Wei Yang, Wei Yang, n-horiguchi, akpm, linux-mm, linux-kernel
On Thu, Dec 05, 2019 at 04:06:20PM +0100, David Hildenbrand wrote:
>On 02.12.19 23:28, Wei Yang wrote:
>> On Wed, Nov 20, 2019 at 04:07:38PM +0100, David Hildenbrand wrote:
>>> On 18.11.19 09:20, Wei Yang wrote:
>>>> hpage is not changed.
>>>>
>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>>> ---
>>>> mm/memory-failure.c | 1 -
>>>> 1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>> index 392ac277b17d..9784f4339ae7 100644
>>>> --- a/mm/memory-failure.c
>>>> +++ b/mm/memory-failure.c
>>>> @@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
>>>> }
>>>> unlock_page(p);
>>>> VM_BUG_ON_PAGE(!page_count(p), p);
>>>> - hpage = compound_head(p);
>>>> }
>>>> /*
>>>>
>>>
>>> I am *absolutely* no transparent huge page expert (sorry :) ), but won't the
>>> split_huge_page(p) eventually split the compound page, such that
>>> compound_head(p) will return something else after that call?
>>>
>>
>> Hi, David
>>
>> Took sometime to look into the code and re-think about it. Found maybe we can
>> simplify this in another way.
>>
>> First, code touches here means split_huge_page() succeeds and "p" is now a PTE
>> page. So compound_head(p) == p.
>
>While this would also be my intuition, I can't state that this is
>guaranteed to be the case (IOW, I did not check the code/documentation) :)
>
If my understanding is correct, split_huge_page() succeeds the THP would be
tear down to normal page.
>>
>> Then let's look at who will use hpage in the following function. There are two
>> uses in current upstream:
>>
>> * page_flags calculation
>> * hwpoison_user_mappings()
>>
>> The first one would be removed in next patch since PageHuge is handled at the
>> beginning.
>>
>> And in the second place, comment says if split succeeds, hpage points to page
>> "p".
>>
>> After all, we don't need to re-calculate hpage after split, and just replace
>> hpage in hwpoison_user_mappings() with p is enough.
>
>That assumption would only be true in case all compound pages at this
>point are transparent huge pages, no? AFAIK that is not necessarily
>true. Or am I missing something?
>
Function hwpoison_user_mappings() just handle user space mapping. If my
understanding is correct, we just have three type of pages would be used in
user space mapping:
* normal page
* THP
* hugetlb
Since THP would be split or already returned and hugetlb is handled in another
branch, this means for other pages hwpoison_user_mappings() would just return
true.
>
>--
>Thanks,
>
>David / dhildenb
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage
2019-12-06 1:48 ` Wei Yang
@ 2020-01-08 12:20 ` David Hildenbrand
2020-01-09 1:58 ` Wei Yang
0 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand @ 2020-01-08 12:20 UTC (permalink / raw)
To: Wei Yang; +Cc: Wei Yang, n-horiguchi, akpm, linux-mm, linux-kernel
On 06.12.19 02:48, Wei Yang wrote:
> On Thu, Dec 05, 2019 at 04:06:20PM +0100, David Hildenbrand wrote:
>> On 02.12.19 23:28, Wei Yang wrote:
>>> On Wed, Nov 20, 2019 at 04:07:38PM +0100, David Hildenbrand wrote:
>>>> On 18.11.19 09:20, Wei Yang wrote:
>>>>> hpage is not changed.
>>>>>
>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>>>> ---
>>>>> mm/memory-failure.c | 1 -
>>>>> 1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>>> index 392ac277b17d..9784f4339ae7 100644
>>>>> --- a/mm/memory-failure.c
>>>>> +++ b/mm/memory-failure.c
>>>>> @@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
>>>>> }
>>>>> unlock_page(p);
>>>>> VM_BUG_ON_PAGE(!page_count(p), p);
>>>>> - hpage = compound_head(p);
>>>>> }
>>>>> /*
>>>>>
>>>>
>>>> I am *absolutely* no transparent huge page expert (sorry :) ), but won't the
>>>> split_huge_page(p) eventually split the compound page, such that
>>>> compound_head(p) will return something else after that call?
>>>>
>>>
>>> Hi, David
>>>
>>> Took sometime to look into the code and re-think about it. Found maybe we can
>>> simplify this in another way.
>>>
>>> First, code touches here means split_huge_page() succeeds and "p" is now a PTE
>>> page. So compound_head(p) == p.
>>
>> While this would also be my intuition, I can't state that this is
>> guaranteed to be the case (IOW, I did not check the code/documentation) :)
>>
>
> If my understanding is correct, split_huge_page() succeeds the THP would be
> tear down to normal page.
>
>>>
>>> Then let's look at who will use hpage in the following function. There are two
>>> uses in current upstream:
>>>
>>> * page_flags calculation
>>> * hwpoison_user_mappings()
>>>
>>> The first one would be removed in next patch since PageHuge is handled at the
>>> beginning.
>>>
>>> And in the second place, comment says if split succeeds, hpage points to page
>>> "p".
>>>
>>> After all, we don't need to re-calculate hpage after split, and just replace
>>> hpage in hwpoison_user_mappings() with p is enough.
>>
>> That assumption would only be true in case all compound pages at this
>> point are transparent huge pages, no? AFAIK that is not necessarily
>> true. Or am I missing something?
>>
>
> Function hwpoison_user_mappings() just handle user space mapping. If my
> understanding is correct, we just have three type of pages would be used in
> user space mapping:
>
> * normal page
> * THP
> * hugetlb
>
> Since THP would be split or already returned and hugetlb is handled in another
> branch, this means for other pages hwpoison_user_mappings() would just return
> true.
>
Sorry for the late reply :)
While I think you are correct, I am not sure if the change you are
suggesting is a) future proof and b) worth it. IOW, the recalculation
after the split makes it clear that something changed and that the
compound page does no longer exist. I might be wrong of course and this
cleanup makes perfect sense :)
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage
2020-01-08 12:20 ` David Hildenbrand
@ 2020-01-09 1:58 ` Wei Yang
0 siblings, 0 replies; 13+ messages in thread
From: Wei Yang @ 2020-01-09 1:58 UTC (permalink / raw)
To: David Hildenbrand
Cc: Wei Yang, Wei Yang, n-horiguchi, akpm, linux-mm, linux-kernel
On Wed, Jan 08, 2020 at 01:20:44PM +0100, David Hildenbrand wrote:
>On 06.12.19 02:48, Wei Yang wrote:
>> On Thu, Dec 05, 2019 at 04:06:20PM +0100, David Hildenbrand wrote:
>>> On 02.12.19 23:28, Wei Yang wrote:
>>>> On Wed, Nov 20, 2019 at 04:07:38PM +0100, David Hildenbrand wrote:
>>>>> On 18.11.19 09:20, Wei Yang wrote:
>>>>>> hpage is not changed.
>>>>>>
>>>>>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
>>>>>> ---
>>>>>> mm/memory-failure.c | 1 -
>>>>>> 1 file changed, 1 deletion(-)
>>>>>>
>>>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>>>> index 392ac277b17d..9784f4339ae7 100644
>>>>>> --- a/mm/memory-failure.c
>>>>>> +++ b/mm/memory-failure.c
>>>>>> @@ -1319,7 +1319,6 @@ int memory_failure(unsigned long pfn, int flags)
>>>>>> }
>>>>>> unlock_page(p);
>>>>>> VM_BUG_ON_PAGE(!page_count(p), p);
>>>>>> - hpage = compound_head(p);
>>>>>> }
>>>>>> /*
>>>>>>
>>>>>
>>>>> I am *absolutely* no transparent huge page expert (sorry :) ), but won't the
>>>>> split_huge_page(p) eventually split the compound page, such that
>>>>> compound_head(p) will return something else after that call?
>>>>>
>>>>
>>>> Hi, David
>>>>
>>>> Took sometime to look into the code and re-think about it. Found maybe we can
>>>> simplify this in another way.
>>>>
>>>> First, code touches here means split_huge_page() succeeds and "p" is now a PTE
>>>> page. So compound_head(p) == p.
>>>
>>> While this would also be my intuition, I can't state that this is
>>> guaranteed to be the case (IOW, I did not check the code/documentation) :)
>>>
>>
>> If my understanding is correct, split_huge_page() succeeds the THP would be
>> tear down to normal page.
>>
>>>>
>>>> Then let's look at who will use hpage in the following function. There are two
>>>> uses in current upstream:
>>>>
>>>> * page_flags calculation
>>>> * hwpoison_user_mappings()
>>>>
>>>> The first one would be removed in next patch since PageHuge is handled at the
>>>> beginning.
>>>>
>>>> And in the second place, comment says if split succeeds, hpage points to page
>>>> "p".
>>>>
>>>> After all, we don't need to re-calculate hpage after split, and just replace
>>>> hpage in hwpoison_user_mappings() with p is enough.
>>>
>>> That assumption would only be true in case all compound pages at this
>>> point are transparent huge pages, no? AFAIK that is not necessarily
>>> true. Or am I missing something?
>>>
>>
>> Function hwpoison_user_mappings() just handle user space mapping. If my
>> understanding is correct, we just have three type of pages would be used in
>> user space mapping:
>>
>> * normal page
>> * THP
>> * hugetlb
>>
>> Since THP would be split or already returned and hugetlb is handled in another
>> branch, this means for other pages hwpoison_user_mappings() would just return
>> true.
>>
>
>Sorry for the late reply :)
>
>While I think you are correct, I am not sure if the change you are
>suggesting is a) future proof and b) worth it. IOW, the recalculation
>after the split makes it clear that something changed and that the
>compound page does no longer exist. I might be wrong of course and this
>cleanup makes perfect sense :)
>
Yep, you are welcome.
I would think about the whole picture again.
>
>--
>Thanks,
>
>David / dhildenb
--
Wei Yang
Help you, Help me
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-01-09 1:58 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-18 8:20 [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure Wei Yang
2019-11-18 8:20 ` [PATCH 2/2] mm/memory-failure.c: not necessary to recalculate hpage Wei Yang
2019-11-20 15:07 ` David Hildenbrand
2019-11-21 1:05 ` Wei Yang
2019-12-02 22:28 ` Wei Yang
2019-12-05 15:06 ` David Hildenbrand
2019-12-06 1:48 ` Wei Yang
2020-01-08 12:20 ` David Hildenbrand
2020-01-09 1:58 ` Wei Yang
2019-11-19 12:23 ` [PATCH 1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure David Hildenbrand
2019-11-20 0:46 ` Wei Yang
2019-11-20 15:04 ` David Hildenbrand
2019-11-21 1:07 ` Wei Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).