| Message ID | 20191118082003.26240-2-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] mm/memory-failure.c: PageHuge is handled at the beginning of memory_failure | expand |
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?
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
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
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?
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
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 :)
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
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); } /*
hpage is not changed. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- mm/memory-failure.c | 1 - 1 file changed, 1 deletion(-)