Re: [PATCH 03/16] debug_vm_pgtable/set_pte: Don't use set_pte_at to update an existing pte entry

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

On 8/12/20 2:42 PM, Anshuman Khandual wrote:
> 
> 
> On 08/12/2020 12:03 PM, Aneesh Kumar K.V wrote:
>> set_pte_at() should not be used to set a pte entry at locations that
>> already holds a valid pte entry. Architectures like ppc64 don't do TLB
>> invalidate in set_pte_at() and hence expect it to be used to set locations
>> that are not a valid PTE.
> 
> Even though set_pte_at() is not really a arch page table helper and
> very much arch specific, I just wonder why this deviation on ppc64
> as compared to other platforms. Detecting such semantics variation
> across platforms is an objective of this test.

Not sure what you mean by set_pte_at is not a page table helper. Generic 
kernel use that helper to set a pte entry.

Now w.r.t ppc64 behavior this was discussed multiple times. I guess 
Linux kernel always used set_pte_at on a none pte entry. We had some 
exceptions in the recent past. But all fixed when noticed.


383321ab8578dfe3bbcc0bc5604c0f8ae08a5c98
mm/hugetlb/migration: use set_huge_pte_at instead of set_pte_at

cee216a696b2004017a5ecb583366093d90b1568
mm/autonuma: don't use set_pte_at when updating protnone ptes

56eecdb912b536a4fa97fb5bfe5a940a54d79be6
mm: Use ptep/pmdp_set_numa() for updating _PAGE_NUMA bit

Yes. Having a testcase like this help

> 
> As small nit.
> 
> Please follow the existing subject format for all patches in here.
> It will improve readability and also help understand these changes
> better, later on.
> 
> mm/debug_vm_pgtable: <Specify changes to an individual test>
> 
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   mm/debug_vm_pgtable.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
>> index 4c32063a8acf..02a7c20aa4a2 100644
>> --- a/mm/debug_vm_pgtable.c
>> +++ b/mm/debug_vm_pgtable.c
>> @@ -81,8 +81,6 @@ static void __init pte_advanced_tests(struct mm_struct *mm,
>>   	pte = ptep_get(ptep);
>>   	WARN_ON(pte_write(pte));
>>   
>> -	pte = pfn_pte(pfn, prot);
>> -	set_pte_at(mm, vaddr, ptep, pte);
>>   	ptep_get_and_clear(mm, vaddr, ptep);
>>   	pte = ptep_get(ptep);
>>   	WARN_ON(!pte_none(pte));
> 
> This makes sense. But could you please fold this code stanza with
> the previous one in order to imply that 'ptep' did have some valid
> entry before being cleared and checked against pte_none().
> 

will do that

>> @@ -97,12 +95,14 @@ static void __init pte_advanced_tests(struct mm_struct *mm,
>>   	pte = ptep_get(ptep);
>>   	WARN_ON(!(pte_write(pte) && pte_dirty(pte)));
>>   
>> -	pte = pfn_pte(pfn, prot);
>> -	set_pte_at(mm, vaddr, ptep, pte);
>>   	ptep_get_and_clear_full(mm, vaddr, ptep, 1);
>>   	pte = ptep_get(ptep);
>>   	WARN_ON(!pte_none(pte));
> 
> Same, please fold back.
> 

ok


>> +	/*
>> +	 * We should clear pte before we do set_pte_at
>> +	 */
>> +	pte = ptep_get_and_clear(mm, vaddr, ptep);
>>   	pte = pte_mkyoung(pte);
>>   	set_pte_at(mm, vaddr, ptep, pte);
>>   	ptep_test_and_clear_young(vma, vaddr, ptep);
>>
> 
> The comment above should also explain details that are mentioned
> in the commit message i.e how platforms such as ppc64 expects a
> clear pte entry for set_pte_at() to work.
> 

I don't think it is specific to ppc64. There is nothing specific to 
ppc64 architecture in there. It is an optimization used in kernel to 
help architecture avoid TLB flush. I will update the comment as below


/* We should clear pte before we do set_pte_at so that set_pte_at don't 
find a valid pte at ptep *?

is that good?

-aneesh