Re: [PATCH V3 2/5] mm: update ptep_modify_prot_commit to take old pte value as arg

[Christophe LEROY <christophe.leroy@c-s.fr>]

Le 05/12/2018 à 05:06, Aneesh Kumar K.V a écrit :
> On 12/5/18 9:32 AM, Christophe LEROY wrote:
>>
>>
>> Le 05/12/2018 à 04:09, Aneesh Kumar K.V a écrit :
>>> Architectures like ppc64 requires to do a conditional tlb flush based 
>>> on the old
>>> and new value of pte. Enable that by passing old pte value as the arg.
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>>   arch/s390/include/asm/pgtable.h | 3 ++-
>>>   arch/s390/mm/pgtable.c          | 2 +-
>>>   arch/x86/include/asm/paravirt.h | 2 +-
>>>   fs/proc/task_mmu.c              | 8 +++++---
>>>   include/asm-generic/pgtable.h   | 2 +-
>>>   mm/memory.c                     | 8 ++++----
>>>   mm/mprotect.c                   | 6 +++---
>>>   7 files changed, 17 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/pgtable.h 
>>> b/arch/s390/include/asm/pgtable.h
>>> index 5d730199e37b..76dc344edb8c 100644
>>> --- a/arch/s390/include/asm/pgtable.h
>>> +++ b/arch/s390/include/asm/pgtable.h
>>> @@ -1070,7 +1070,8 @@ static inline pte_t ptep_get_and_clear(struct 
>>> mm_struct *mm,
>>>   #define __HAVE_ARCH_PTEP_MODIFY_PROT_TRANSACTION
>>>   pte_t ptep_modify_prot_start(struct vm_area_struct *, unsigned 
>>> long, pte_t *);
>>> -void ptep_modify_prot_commit(struct vm_area_struct *, unsigned long, 
>>> pte_t *, pte_t);
>>> +void ptep_modify_prot_commit(struct vm_area_struct *, unsigned long,
>>> +                 pte_t *, pte_t, pte_t);
>>>   #define __HAVE_ARCH_PTEP_CLEAR_FLUSH
>>>   static inline pte_t ptep_clear_flush(struct vm_area_struct *vma,
>>> diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c
>>> index 29c0a21cd34a..b283b92722cc 100644
>>> --- a/arch/s390/mm/pgtable.c
>>> +++ b/arch/s390/mm/pgtable.c
>>> @@ -322,7 +322,7 @@ pte_t ptep_modify_prot_start(struct 
>>> vm_area_struct *vma, unsigned long addr,
>>>   EXPORT_SYMBOL(ptep_modify_prot_start);
>>>   void ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned 
>>> long addr,
>>> -                 pte_t *ptep, pte_t pte)
>>> +                 pte_t *ptep, pte_t old_pte, pte_t pte)
>>>   {
>>>       pgste_t pgste;
>>>       struct mm_struct *mm = vma->vm_mm;
>>> diff --git a/arch/x86/include/asm/paravirt.h 
>>> b/arch/x86/include/asm/paravirt.h
>>> index 1154f154025d..0d75a4f60500 100644
>>> --- a/arch/x86/include/asm/paravirt.h
>>> +++ b/arch/x86/include/asm/paravirt.h
>>> @@ -429,7 +429,7 @@ static inline pte_t ptep_modify_prot_start(struct 
>>> vm_area_struct *vma, unsigned
>>>   }
>>>   static inline void ptep_modify_prot_commit(struct vm_area_struct 
>>> *vma, unsigned long addr,
>>> -                       pte_t *ptep, pte_t pte)
>>> +                       pte_t *ptep, pte_t old_pte, pte_t pte)
>>>   {
>>>       struct mm_struct *mm = vma->vm_mm;
>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>>> index 9952d7185170..8d62891d38a8 100644
>>> --- a/fs/proc/task_mmu.c
>>> +++ b/fs/proc/task_mmu.c
>>> @@ -940,10 +940,12 @@ static inline void clear_soft_dirty(struct 
>>> vm_area_struct *vma,
>>>       pte_t ptent = *pte;
>>>       if (pte_present(ptent)) {
>>> -        ptent = ptep_modify_prot_start(vma, addr, pte);
>>> -        ptent = pte_wrprotect(ptent);
>>> +        pte_t old_pte;
>>> +
>>> +        old_pte = ptep_modify_prot_start(vma, addr, pte);
>>> +        ptent = pte_wrprotect(old_pte);
>>
>> This change doesn't seem to fit with the commit description. Why write 
>> protecting in addition to clearing dirty ?
>>
>>
> 
> The hunk above use a new variable old_pte. There is no functional change 
> in that hunk.
> 

Oops, sorry, I misread the patch, don't know why.

Christophe