[PATCH RFC] mm: arm64: advance pte for contpte_ptep_set_access_flags

[PATCH RFC] mm: arm64: advance pte for contpte_ptep_set_access_flags

[Barry Song]

From: Barry Song <v-songbaohua@oppo.com>

Hi Ryan, David,
it seems contpte_ptep_set_access_flags() has never advanced
pte pfn, and it is setting all entries' pfn to the first
subpage. But I feel quite strange we never have a bug reported.
Am I missing something?

Fixes: 4602e5757bcc ("arm64/mm: wire up PTE_CONT for user mappings")
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
 arch/arm64/mm/contpte.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
index a3edced29ac1..10dcd2641184 100644
--- a/arch/arm64/mm/contpte.c
+++ b/arch/arm64/mm/contpte.c
@@ -421,8 +421,10 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
 		ptep = contpte_align_down(ptep);
 		start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
 
-		for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
+		for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE) {
 			__ptep_set_access_flags(vma, addr, ptep, entry, 0);
+			entry = pte_advance_pfn(entry, 1);
+		}
 
 		if (dirty)
 			__flush_tlb_range(vma, start_addr, addr,
-- 
2.39.3 (Apple Git-146)

Re: [PATCH RFC] mm: arm64: advance pte for contpte_ptep_set_access_flags

[David Hildenbrand]

On 31.08.24 10:35, Barry Song wrote:
> From: Barry Song <v-songbaohua@oppo.com>
> 
> Hi Ryan, David,
> it seems contpte_ptep_set_access_flags() has never advanced
> pte pfn, and it is setting all entries' pfn to the first
> subpage. But I feel quite strange we never have a bug reported.
> Am I missing something?
> 
> Fixes: 4602e5757bcc ("arm64/mm: wire up PTE_CONT for user mappings")
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> ---
>   arch/arm64/mm/contpte.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> index a3edced29ac1..10dcd2641184 100644
> --- a/arch/arm64/mm/contpte.c
> +++ b/arch/arm64/mm/contpte.c
> @@ -421,8 +421,10 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
>   		ptep = contpte_align_down(ptep);
>   		start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
>   
> -		for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
> +		for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE) {
>   			__ptep_set_access_flags(vma, addr, ptep, entry, 0);
> +			entry = pte_advance_pfn(entry, 1);
> +		}
>   
>   		if (dirty)
>   			__flush_tlb_range(vma, start_addr, addr,

Taking a closer look at __ptep_set_access_flags(), there is:

/* only preserve the access flags and write permission *
pte_val(entry) &= PTE_RDONLY | PTE_AF | PTE_WRITE | PTE_DIRTY;

So it looks like it doesn't need the PFN?


OTOH, there is the initial:


if (pte_same(pte, entry))
	return 0;

check that might accelerate things.

So unless I am missing something, this works as expected? (and if the 
pte_same() would frequently be taken with your change would be worthwile 
to optimize)

-- 
Cheers,

David / dhildenb

Re: [PATCH RFC] mm: arm64: advance pte for contpte_ptep_set_access_flags

[Barry Song]

On Sat, Aug 31, 2024 at 9:54 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 31.08.24 10:35, Barry Song wrote:
> > From: Barry Song <v-songbaohua@oppo.com>
> >
> > Hi Ryan, David,
> > it seems contpte_ptep_set_access_flags() has never advanced
> > pte pfn, and it is setting all entries' pfn to the first
> > subpage. But I feel quite strange we never have a bug reported.
> > Am I missing something?
> >
> > Fixes: 4602e5757bcc ("arm64/mm: wire up PTE_CONT for user mappings")
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: David Hildenbrand <david@redhat.com>
> > Cc: Will Deacon <will@kernel.org>
> > Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> > ---
> >   arch/arm64/mm/contpte.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> > index a3edced29ac1..10dcd2641184 100644
> > --- a/arch/arm64/mm/contpte.c
> > +++ b/arch/arm64/mm/contpte.c
> > @@ -421,8 +421,10 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
> >               ptep = contpte_align_down(ptep);
> >               start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
> >
> > -             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
> > +             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE) {
> >                       __ptep_set_access_flags(vma, addr, ptep, entry, 0);
> > +                     entry = pte_advance_pfn(entry, 1);
> > +             }
> >
> >               if (dirty)
> >                       __flush_tlb_range(vma, start_addr, addr,
>
> Taking a closer look at __ptep_set_access_flags(), there is:
>
> /* only preserve the access flags and write permission *
> pte_val(entry) &= PTE_RDONLY | PTE_AF | PTE_WRITE | PTE_DIRTY;
>
> So it looks like it doesn't need the PFN?

right.

>
>
> OTOH, there is the initial:
>
>
> if (pte_same(pte, entry))
>         return 0;
>
> check that might accelerate things.
>
> So unless I am missing something, this works as expected? (and if the
> pte_same() would frequently be taken with your change would be worthwile
> to optimize)


Right. From page 1 to page (nr_pages - 1), we consistently get FALSE
for pte_same().
This seems quite strange. I think we might need to "fix" it, at least
for the sake of code
semantics. on the other hand, if pte_same() is not important, it
should be dropped.

Hi Ryan,
what is your take on this?

>
> --
> Cheers,
>
> David / dhildenb
>

Thanks
Barry

Re: [PATCH RFC] mm: arm64: advance pte for contpte_ptep_set_access_flags

[Will Deacon]

(Adding Ryan, since you're asking him a question!)

On Sat, Aug 31, 2024 at 10:06:40PM +1200, Barry Song wrote:
> On Sat, Aug 31, 2024 at 9:54 PM David Hildenbrand <david@redhat.com> wrote:
> >
> > On 31.08.24 10:35, Barry Song wrote:
> > > From: Barry Song <v-songbaohua@oppo.com>
> > >
> > > Hi Ryan, David,
> > > it seems contpte_ptep_set_access_flags() has never advanced
> > > pte pfn, and it is setting all entries' pfn to the first
> > > subpage. But I feel quite strange we never have a bug reported.
> > > Am I missing something?
> > >
> > > Fixes: 4602e5757bcc ("arm64/mm: wire up PTE_CONT for user mappings")
> > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > Cc: John Hubbard <jhubbard@nvidia.com>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: David Hildenbrand <david@redhat.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> > > ---
> > >   arch/arm64/mm/contpte.c | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> > > index a3edced29ac1..10dcd2641184 100644
> > > --- a/arch/arm64/mm/contpte.c
> > > +++ b/arch/arm64/mm/contpte.c
> > > @@ -421,8 +421,10 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
> > >               ptep = contpte_align_down(ptep);
> > >               start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
> > >
> > > -             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
> > > +             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE) {
> > >                       __ptep_set_access_flags(vma, addr, ptep, entry, 0);
> > > +                     entry = pte_advance_pfn(entry, 1);
> > > +             }
> > >
> > >               if (dirty)
> > >                       __flush_tlb_range(vma, start_addr, addr,
> >
> > Taking a closer look at __ptep_set_access_flags(), there is:
> >
> > /* only preserve the access flags and write permission *
> > pte_val(entry) &= PTE_RDONLY | PTE_AF | PTE_WRITE | PTE_DIRTY;
> >
> > So it looks like it doesn't need the PFN?
> 
> right.
> 
> >
> >
> > OTOH, there is the initial:
> >
> >
> > if (pte_same(pte, entry))
> >         return 0;
> >
> > check that might accelerate things.
> >
> > So unless I am missing something, this works as expected? (and if the
> > pte_same() would frequently be taken with your change would be worthwile
> > to optimize)
> 
> 
> Right. From page 1 to page (nr_pages - 1), we consistently get FALSE
> for pte_same().
> This seems quite strange. I think we might need to "fix" it, at least
> for the sake of code
> semantics. on the other hand, if pte_same() is not important, it
> should be dropped.
> 
> Hi Ryan,
> what is your take on this?
> 
> >
> > --
> > Cheers,
> >
> > David / dhildenb
> >
> 
> Thanks
> Barry

Re: [PATCH RFC] mm: arm64: advance pte for contpte_ptep_set_access_flags

[Ryan Roberts]

On 04/09/2024 16:13, Will Deacon wrote:
> (Adding Ryan, since you're asking him a question!)

Thanks, Will!

Afraid I don't do a good job of monitoring the list; I'm guessing there are
automated ways to filter for mentions of my name so I catch this sort of thing
in future?

> 
> On Sat, Aug 31, 2024 at 10:06:40PM +1200, Barry Song wrote:
>> On Sat, Aug 31, 2024 at 9:54 PM David Hildenbrand <david@redhat.com> wrote:
>>>
>>> On 31.08.24 10:35, Barry Song wrote:
>>>> From: Barry Song <v-songbaohua@oppo.com>
>>>>
>>>> Hi Ryan, David,
>>>> it seems contpte_ptep_set_access_flags() has never advanced
>>>> pte pfn, and it is setting all entries' pfn to the first
>>>> subpage. But I feel quite strange we never have a bug reported.
>>>> Am I missing something?
>>>>
>>>> Fixes: 4602e5757bcc ("arm64/mm: wire up PTE_CONT for user mappings")
>>>> Cc: Ard Biesheuvel <ardb@kernel.org>
>>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: David Hildenbrand <david@redhat.com>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
>>>> ---
>>>>   arch/arm64/mm/contpte.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
>>>> index a3edced29ac1..10dcd2641184 100644
>>>> --- a/arch/arm64/mm/contpte.c
>>>> +++ b/arch/arm64/mm/contpte.c
>>>> @@ -421,8 +421,10 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
>>>>               ptep = contpte_align_down(ptep);
>>>>               start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
>>>>
>>>> -             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
>>>> +             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE) {
>>>>                       __ptep_set_access_flags(vma, addr, ptep, entry, 0);
>>>> +                     entry = pte_advance_pfn(entry, 1);
>>>> +             }
>>>>
>>>>               if (dirty)
>>>>                       __flush_tlb_range(vma, start_addr, addr,
>>>
>>> Taking a closer look at __ptep_set_access_flags(), there is:
>>>
>>> /* only preserve the access flags and write permission *
>>> pte_val(entry) &= PTE_RDONLY | PTE_AF | PTE_WRITE | PTE_DIRTY;
>>>
>>> So it looks like it doesn't need the PFN?

Correct, I don't believe there is a bug here; __ptep_set_access_flags() only
consumes the access flags from entry.

>>
>> right.
>>
>>>
>>>
>>> OTOH, there is the initial:
>>>
>>>
>>> if (pte_same(pte, entry))
>>>         return 0;
>>>
>>> check that might accelerate things.

There is an equivalent check in contpte_ptep_set_access_flags() which is
checking for the whole contpte block and returning early if so. So I don't think
there is a problem here either.

>>>
>>> So unless I am missing something, this works as expected? (and if the
>>> pte_same() would frequently be taken with your change would be worthwile
>>> to optimize)
>>
>>
>> Right. From page 1 to page (nr_pages - 1), we consistently get FALSE
>> for pte_same().
>> This seems quite strange. I think we might need to "fix" it, at least
>> for the sake of code
>> semantics. on the other hand, if pte_same() is not important, it
>> should be dropped.
>>
>> Hi Ryan,
>> what is your take on this?

The code is correct and working as intended, AFAICT. But I accept that this is
not exactly obvious. I'd be happy to Rb your proposed change if you feel it
clarifies things.

Thanks,
Ryan

>>
>>>
>>> --
>>> Cheers,
>>>
>>> David / dhildenb
>>>
>>
>> Thanks
>> Barry

Re: [PATCH RFC] mm: arm64: advance pte for contpte_ptep_set_access_flags

[Barry Song]

On Thu, Sep 5, 2024 at 3:50 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
>
> On 04/09/2024 16:13, Will Deacon wrote:
> > (Adding Ryan, since you're asking him a question!)
>
> Thanks, Will!
>
> Afraid I don't do a good job of monitoring the list; I'm guessing there are
> automated ways to filter for mentions of my name so I catch this sort of thing
> in future?

It's not your fault. I just realized that, for some unknown reason, I forgot to
CC you.

>
> >
> > On Sat, Aug 31, 2024 at 10:06:40PM +1200, Barry Song wrote:
> >> On Sat, Aug 31, 2024 at 9:54 PM David Hildenbrand <david@redhat.com> wrote:
> >>>
> >>> On 31.08.24 10:35, Barry Song wrote:
> >>>> From: Barry Song <v-songbaohua@oppo.com>
> >>>>
> >>>> Hi Ryan, David,
> >>>> it seems contpte_ptep_set_access_flags() has never advanced
> >>>> pte pfn, and it is setting all entries' pfn to the first
> >>>> subpage. But I feel quite strange we never have a bug reported.
> >>>> Am I missing something?
> >>>>
> >>>> Fixes: 4602e5757bcc ("arm64/mm: wire up PTE_CONT for user mappings")
> >>>> Cc: Ard Biesheuvel <ardb@kernel.org>
> >>>> Cc: John Hubbard <jhubbard@nvidia.com>
> >>>> Cc: Mark Rutland <mark.rutland@arm.com>
> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >>>> Cc: David Hildenbrand <david@redhat.com>
> >>>> Cc: Will Deacon <will@kernel.org>
> >>>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> >>>> ---
> >>>>   arch/arm64/mm/contpte.c | 4 +++-
> >>>>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> >>>> index a3edced29ac1..10dcd2641184 100644
> >>>> --- a/arch/arm64/mm/contpte.c
> >>>> +++ b/arch/arm64/mm/contpte.c
> >>>> @@ -421,8 +421,10 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
> >>>>               ptep = contpte_align_down(ptep);
> >>>>               start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
> >>>>
> >>>> -             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
> >>>> +             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE) {
> >>>>                       __ptep_set_access_flags(vma, addr, ptep, entry, 0);
> >>>> +                     entry = pte_advance_pfn(entry, 1);
> >>>> +             }
> >>>>
> >>>>               if (dirty)
> >>>>                       __flush_tlb_range(vma, start_addr, addr,
> >>>
> >>> Taking a closer look at __ptep_set_access_flags(), there is:
> >>>
> >>> /* only preserve the access flags and write permission *
> >>> pte_val(entry) &= PTE_RDONLY | PTE_AF | PTE_WRITE | PTE_DIRTY;
> >>>
> >>> So it looks like it doesn't need the PFN?
>
> Correct, I don't believe there is a bug here; __ptep_set_access_flags() only
> consumes the access flags from entry.
>
> >>
> >> right.
> >>
> >>>
> >>>
> >>> OTOH, there is the initial:
> >>>
> >>>
> >>> if (pte_same(pte, entry))
> >>>         return 0;
> >>>
> >>> check that might accelerate things.
>
> There is an equivalent check in contpte_ptep_set_access_flags() which is
> checking for the whole contpte block and returning early if so. So I don't think
> there is a problem here either.
>
> >>>
> >>> So unless I am missing something, this works as expected? (and if the
> >>> pte_same() would frequently be taken with your change would be worthwile
> >>> to optimize)
> >>
> >>
> >> Right. From page 1 to page (nr_pages - 1), we consistently get FALSE
> >> for pte_same().
> >> This seems quite strange. I think we might need to "fix" it, at least
> >> for the sake of code
> >> semantics. on the other hand, if pte_same() is not important, it
> >> should be dropped.
> >>
> >> Hi Ryan,
> >> what is your take on this?
>
> The code is correct and working as intended, AFAICT. But I accept that this is
> not exactly obvious. I'd be happy to Rb your proposed change if you feel it
> clarifies things.

If this is the case, I'd rather add some comments instead in v2?

diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
index a3edced29ac1..55107d27d3f8 100644
--- a/arch/arm64/mm/contpte.c
+++ b/arch/arm64/mm/contpte.c
@@ -421,6 +421,12 @@ int contpte_ptep_set_access_flags(struct
vm_area_struct *vma,
                ptep = contpte_align_down(ptep);
                start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);

+               /*
+                * We are not advancing entry because __ptep_set_access_flags()
+                * only consumes access flags from entry. And since we
have checked
+                * for the whole contpte block and returned early, pte_same()
+                * within __ptep_set_access_flags() is likely false.
+                */
                for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
                        __ptep_set_access_flags(vma, addr, ptep, entry, 0);
-- 
2.39.3 (Apple Git-146)

Thanks
Barry

Re: [PATCH RFC] mm: arm64: advance pte for contpte_ptep_set_access_flags

[Ryan Roberts]

On 05/09/2024 04:27, Barry Song wrote:
> On Thu, Sep 5, 2024 at 3:50 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>
>> On 04/09/2024 16:13, Will Deacon wrote:
>>> (Adding Ryan, since you're asking him a question!)
>>
>> Thanks, Will!
>>
>> Afraid I don't do a good job of monitoring the list; I'm guessing there are
>> automated ways to filter for mentions of my name so I catch this sort of thing
>> in future?
> 
> It's not your fault. I just realized that, for some unknown reason, I forgot to
> CC you.

No worries. I was just asking if there is a general approach that people take to
monitor mail that they are not explicitly cc'ed on, but I guess that's a bit off
topic.

> 
>>
>>>
>>> On Sat, Aug 31, 2024 at 10:06:40PM +1200, Barry Song wrote:
>>>> On Sat, Aug 31, 2024 at 9:54 PM David Hildenbrand <david@redhat.com> wrote:
>>>>>
>>>>> On 31.08.24 10:35, Barry Song wrote:
>>>>>> From: Barry Song <v-songbaohua@oppo.com>
>>>>>>
>>>>>> Hi Ryan, David,
>>>>>> it seems contpte_ptep_set_access_flags() has never advanced
>>>>>> pte pfn, and it is setting all entries' pfn to the first
>>>>>> subpage. But I feel quite strange we never have a bug reported.
>>>>>> Am I missing something?
>>>>>>
>>>>>> Fixes: 4602e5757bcc ("arm64/mm: wire up PTE_CONT for user mappings")
>>>>>> Cc: Ard Biesheuvel <ardb@kernel.org>
>>>>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>> Cc: David Hildenbrand <david@redhat.com>
>>>>>> Cc: Will Deacon <will@kernel.org>
>>>>>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
>>>>>> ---
>>>>>>   arch/arm64/mm/contpte.c | 4 +++-
>>>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
>>>>>> index a3edced29ac1..10dcd2641184 100644
>>>>>> --- a/arch/arm64/mm/contpte.c
>>>>>> +++ b/arch/arm64/mm/contpte.c
>>>>>> @@ -421,8 +421,10 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
>>>>>>               ptep = contpte_align_down(ptep);
>>>>>>               start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
>>>>>>
>>>>>> -             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
>>>>>> +             for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE) {
>>>>>>                       __ptep_set_access_flags(vma, addr, ptep, entry, 0);
>>>>>> +                     entry = pte_advance_pfn(entry, 1);
>>>>>> +             }
>>>>>>
>>>>>>               if (dirty)
>>>>>>                       __flush_tlb_range(vma, start_addr, addr,
>>>>>
>>>>> Taking a closer look at __ptep_set_access_flags(), there is:
>>>>>
>>>>> /* only preserve the access flags and write permission *
>>>>> pte_val(entry) &= PTE_RDONLY | PTE_AF | PTE_WRITE | PTE_DIRTY;
>>>>>
>>>>> So it looks like it doesn't need the PFN?
>>
>> Correct, I don't believe there is a bug here; __ptep_set_access_flags() only
>> consumes the access flags from entry.
>>
>>>>
>>>> right.
>>>>
>>>>>
>>>>>
>>>>> OTOH, there is the initial:
>>>>>
>>>>>
>>>>> if (pte_same(pte, entry))
>>>>>         return 0;
>>>>>
>>>>> check that might accelerate things.
>>
>> There is an equivalent check in contpte_ptep_set_access_flags() which is
>> checking for the whole contpte block and returning early if so. So I don't think
>> there is a problem here either.
>>
>>>>>
>>>>> So unless I am missing something, this works as expected? (and if the
>>>>> pte_same() would frequently be taken with your change would be worthwile
>>>>> to optimize)
>>>>
>>>>
>>>> Right. From page 1 to page (nr_pages - 1), we consistently get FALSE
>>>> for pte_same().
>>>> This seems quite strange. I think we might need to "fix" it, at least
>>>> for the sake of code
>>>> semantics. on the other hand, if pte_same() is not important, it
>>>> should be dropped.
>>>>
>>>> Hi Ryan,
>>>> what is your take on this?
>>
>> The code is correct and working as intended, AFAICT. But I accept that this is
>> not exactly obvious. I'd be happy to Rb your proposed change if you feel it
>> clarifies things.
> 
> If this is the case, I'd rather add some comments instead in v2?
> 
> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> index a3edced29ac1..55107d27d3f8 100644
> --- a/arch/arm64/mm/contpte.c
> +++ b/arch/arm64/mm/contpte.c
> @@ -421,6 +421,12 @@ int contpte_ptep_set_access_flags(struct
> vm_area_struct *vma,
>                 ptep = contpte_align_down(ptep);
>                 start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
> 
> +               /*
> +                * We are not advancing entry because __ptep_set_access_flags()
> +                * only consumes access flags from entry. And since we
> have checked
> +                * for the whole contpte block and returned early, pte_same()
> +                * within __ptep_set_access_flags() is likely false.
> +                */
>                 for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
>                         __ptep_set_access_flags(vma, addr, ptep, entry, 0);

LGTM:

Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>

Re: [PATCH RFC] mm: arm64: advance pte for contpte_ptep_set_access_flags

[David Hildenbrand]

On 05.09.24 09:20, Ryan Roberts wrote:
> On 05/09/2024 04:27, Barry Song wrote:
>> On Thu, Sep 5, 2024 at 3:50 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>
>>> On 04/09/2024 16:13, Will Deacon wrote:
>>>> (Adding Ryan, since you're asking him a question!)
>>>
>>> Thanks, Will!
>>>
>>> Afraid I don't do a good job of monitoring the list; I'm guessing there are
>>> automated ways to filter for mentions of my name so I catch this sort of thing
>>> in future?
>>
>> It's not your fault. I just realized that, for some unknown reason, I forgot to
>> CC you.
> 
> No worries. I was just asking if there is a general approach that people take to
> monitor mail that they are not explicitly cc'ed on, but I guess that's a bit off
> topic.

I do something slightly different: I filter mails based on keywords. For 
example, whenever a patch contains page_mapcount(), folio_mapcount(), 
folio_mapped(), ... it gets put into a separate "mapcount" folder :) 
Same regarding file names.

For you, it would probably make sense to filter for contpte or sth like 
that :)

[...]

>>>
>>> The code is correct and working as intended, AFAICT. But I accept that this is
>>> not exactly obvious. I'd be happy to Rb your proposed change if you feel it
>>> clarifies things.
>>
>> If this is the case, I'd rather add some comments instead in v2?
>>
>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
>> index a3edced29ac1..55107d27d3f8 100644
>> --- a/arch/arm64/mm/contpte.c
>> +++ b/arch/arm64/mm/contpte.c
>> @@ -421,6 +421,12 @@ int contpte_ptep_set_access_flags(struct
>> vm_area_struct *vma,
>>                  ptep = contpte_align_down(ptep);
>>                  start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
>>
>> +               /*
>> +                * We are not advancing entry because __ptep_set_access_flags()
>> +                * only consumes access flags from entry. And since we
>> have checked
>> +                * for the whole contpte block and returned early, pte_same()
>> +                * within __ptep_set_access_flags() is likely false.
>> +                */
>>                  for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
>>                          __ptep_set_access_flags(vma, addr, ptep, entry, 0);
> 
> LGTM:
> 
> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>
> 

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb