Re: [PATCH] mm/thp: Correctly differentiate between mapped THP and PMD migration entry

From: Anshuman Khandual
Date: Tue Oct 16 2018 - 09:16:56 EST

On 10/15/2018 02:02 PM, Aneesh Kumar K.V wrote:
> On 10/12/18 1:32 PM, Anshuman Khandual wrote:
>> On 10/09/2018 06:48 PM, Will Deacon wrote:
>>> On Tue, Oct 09, 2018 at 04:04:21PM +0300, Kirill A. Shutemov wrote:
>>>> On Tue, Oct 09, 2018 at 09:28:58AM +0530, Anshuman Khandual wrote:
>>>>> A normal mapped THP page at PMD level should be correctly differentiated
>>>>> from a PMD migration entry while walking the page table. A mapped THP would
>>>>> additionally check positive for pmd_present() along with pmd_trans_huge()
>>>>> as compared to a PMD migration entry. This just adds a new conditional test
>>>>> differentiating the two while walking the page table.
>>>>> Fixes: 616b8371539a6 ("mm: thp: enable thp migration in generic path")
>>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>>>>> ---
>>>>> On X86, pmd_trans_huge() and is_pmd_migration_entry() are always mutually
>>>>> exclusive which makes the current conditional block work for both mapped
>>>>> and migration entries. This is not same with arm64 where pmd_trans_huge()
>>>>> returns positive for both mapped and migration entries. Could some one
>>>>> please explain why pmd_trans_huge() has to return false for migration
>>>>> entries which just install swap bits and its still a PMD ?
>>>> I guess it's just a design choice. Any reason why arm64 cannot do the
>>>> same?
>>> Anshuman, would it work to:
>>> #define pmd_trans_huge(pmd)ÂÂÂÂ (pmd_present(pmd) && !(pmd_val(pmd) & PMD_TABLE_BIT))
>> yeah this works but some how does not seem like the right thing to do
>> but can be the very last option.
> There can be other code paths that makes that assumption. I ended up doing the below for pmd_trans_huge on ppc64.

Yeah, did see that in one of the previous proposals.

But the existing semantics does not look right and makes vague assumptions.
Zi Yan has already asked Andrea for his input in this regard on the next
thread. So I guess while being here, its a good idea to revisit existing
semantics and it's assumptions before fixing it in arch specific helpers.

- Anshuman

> /*
> Â* Only returns true for a THP. False for pmd migration entry.
> Â* We also need to return true when we come across a pte that
> Â* in between a thp split. While splitting THP, we mark the pmd
> Â* invalid (pmdp_invalidate()) before we set it with pte page
> Â* address. A pmd_trans_huge() check against a pmd entry during that time
> Â* should return true.
> Â* We should not call this on a hugetlb entry. We should check for HugeTLB
> Â* entry using vma->vm_flags
> Â* The page table walk rule is explained in Documentation/vm/transhuge.rst
> Â*/
> static inline int pmd_trans_huge(pmd_t pmd)
> {
> ÂÂÂÂif (!pmd_present(pmd))
> ÂÂÂÂÂÂÂ return false;
> ÂÂÂÂif (radix_enabled())
> ÂÂÂÂÂÂÂ return radix__pmd_trans_huge(pmd);
> ÂÂÂÂreturn hash__pmd_trans_huge(pmd);
> }
> -aneesh