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

[Zi Yan]

On 12 Oct 2018, at 4:00, Anshuman Khandual wrote:

> On 10/10/2018 06:13 PM, Zi Yan wrote:
>> On 10 Oct 2018, at 0:05, Anshuman Khandual wrote:
>>
>>> On 10/09/2018 07:28 PM, Zi Yan wrote:
>>>> cc: Naoya Horiguchi (who proposed to use !_PAGE_PRESENT && !_PAGE_PSE for x86
>>>> PMD migration entry check)
>>>>
>>>> On 8 Oct 2018, at 23:58, 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()
>>>>
>>>> !pmd_present() && pmd_trans_huge() is used to represent THPs under splitting,
>>>
>>> Not really if we just look at code in the conditional blocks.
>>
>> Yeah, I explained it wrong above. Sorry about that.
>>
>> In x86, pmd_present() checks (_PAGE_PRESENT | _PAGE_PROTNONE | _PAGE_PSE),
>> thus, it returns true even if the present bit is cleared but PSE bit is set.
>
> Okay.
>
>> This is done so, because THPs under splitting are regarded as present in the kernel
>> but not present when a hardware page table walker checks it.
>
> Okay.
>
>>
>> For PMD migration entry, which should be regarded as not present, if PSE bit
>> is set, which makes pmd_trans_huge() returns true, like ARM64 does, all
>> PMD migration entries will be regarded as present
>
> Okay to make pmd_present() return false pmd_trans_huge() has to return false
> as well. Is there anything which can be done to get around this problem on
> X86 ? pmd_trans_huge() returning true for a migration entry sounds logical.
> Otherwise we would revert the condition block order to accommodate both the
> implementation for pmd_trans_huge() as suggested by Kirill before or just
> consider this patch forward.
>
> Because I am not really sure yet about the idea of getting pmd_present()
> check into pmd_trans_huge() on arm64 just to make it fit into this semantics
> as suggested by Will. If a PMD is trans huge page or not should not depend on
> whether it is present or not.

In terms of THPs, we have three cases: a present THP, a THP under splitting,
and a THP under migration. pmd_present() and pmd_trans_huge() both return true
for a present THP and a THP under splitting, because they discover _PAGE_PSE bit
is set for both cases, whereas they both return false for a THP under migration.
You want to change them to make pmd_trans_huge() returns true for a THP under migration
instead of false to help ARM64âs support for THP migration.

For x86, this change requires:
1. changing the condition in pmd_trans_huge(), so that it returns true for
PMD migration entries;
2. changing the code, which calls pmd_trans_huge(), to match the new logic.

Another problem I see is that x86âs pmd_present() returns true for a THP under
splitting but ARM64âs pmd_present() returns false for a THP under splitting.
I do not know if there is any correctness issue with this. So I copy Andrea
here, since he made x86âs pmd_present() returns true for a THP under splitting
as an optimization. I want to understand more about it and potentially make
x86 and ARM64 (maybe all other architectures, too) return the same value
for all three cases mentioned above.


Hi Andrea, what is the purpose/benefit of making x86âs pmd_present() returns true
for a THP under splitting? Does it cause problems when ARM64âs pmd_present()
returns false in the same situation?


>>
>> My concern is that if ARM64âs pmd_trans_huge() returns true for migration
>> entries, unlike x86, there might be bugs triggered in the kernel when
>> THP migration is enabled in ARM64.
>
> Right and that is exactly what we are trying to fix with this patch.
>

I am not sure this patch can fix the problem in ARM64, because many other places
in the kernel, pmd_trans_huge() still returns false for a THP under migration.
We may need more comprehensive fixes for ARM64.

Thanks.

â
Best Regards,
Yan Zi

Attachment: signature.asc
Description: OpenPGP digital signature