Re: [PATCH v10 06/25] mm: make pte_unmap_same compatible with SPF

[Laurent Dufour]

On 10/05/2018 18:15, vinayak menon wrote:
> On Tue, Apr 17, 2018 at 8:03 PM, Laurent Dufour
> <ldufour@xxxxxxxxxxxxxxxxxx> wrote:
>> pte_unmap_same() is making the assumption that the page table are still
>> around because the mmap_sem is held.
>> This is no more the case when running a speculative page fault and
>> additional check must be made to ensure that the final page table are still
>> there.
>>
>> This is now done by calling pte_spinlock() to check for the VMA's
>> consistency while locking for the page tables.
>>
>> This is requiring passing a vm_fault structure to pte_unmap_same() which is
>> containing all the needed parameters.
>>
>> As pte_spinlock() may fail in the case of a speculative page fault, if the
>> VMA has been touched in our back, pte_unmap_same() should now return 3
>> cases :
>>         1. pte are the same (0)
>>         2. pte are different (VM_FAULT_PTNOTSAME)
>>         3. a VMA's changes has been detected (VM_FAULT_RETRY)
>>
>> The case 2 is handled by the introduction of a new VM_FAULT flag named
>> VM_FAULT_PTNOTSAME which is then trapped in cow_user_page().
>> If VM_FAULT_RETRY is returned, it is passed up to the callers to retry the
>> page fault while holding the mmap_sem.
>>
>> Acked-by: David Rientjes <rientjes@xxxxxxxxxx>
>> Signed-off-by: Laurent Dufour <ldufour@xxxxxxxxxxxxxxxxxx>
>> ---
>>  include/linux/mm.h |  1 +
>>  mm/memory.c        | 39 ++++++++++++++++++++++++++++-----------
>>  2 files changed, 29 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 4d1aff80669c..714da99d77a3 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1208,6 +1208,7 @@ static inline void clear_page_pfmemalloc(struct page *page)
>>  #define VM_FAULT_NEEDDSYNC  0x2000     /* ->fault did not modify page tables
>>                                          * and needs fsync() to complete (for
>>                                          * synchronous page faults in DAX) */
>> +#define VM_FAULT_PTNOTSAME 0x4000      /* Page table entries have changed */
> 
> 
> This has to be added to VM_FAULT_RESULT_TRACE ?

Indeed there is no chance that the macro VM_FAULT_RESULT_TRACE would have to
translate that code to a string since VM_FAULT_PTNOTSAME is currently only
returned by pte_unmap_same() and then converted by its only caller
do_swap_page() to return 0. So VM_FAULT_PTNOTSAME is not expected to be seen
outside of these services which are never using VM_FAULT_RESULT_TRACE().

This being said, this may be a good idea to add it in the case of future
potential usage.