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@xxxxxxxxxxxxxxx> >>>>>> --- >>>>>> 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
