[PATCH 1/1] mm/mlock: implement folio_mlock_step() using folio_pte_batch()

[PATCH 1/1] mm/mlock: implement folio_mlock_step() using folio_pte_batch()

[Lance Yang]

Let's make folio_mlock_step() simply a wrapper around folio_pte_batch(),
which will greatly reduce the cost of ptep_get() when scanning a range of
contptes.

Signed-off-by: Lance Yang <ioworker0@gmail.com>
---
 mm/mlock.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/mm/mlock.c b/mm/mlock.c
index 30b51cdea89d..1ae6232d38cf 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -307,26 +307,15 @@ void munlock_folio(struct folio *folio)
 static inline unsigned int folio_mlock_step(struct folio *folio,
 		pte_t *pte, unsigned long addr, unsigned long end)
 {
-	unsigned int count, i, nr = folio_nr_pages(folio);
-	unsigned long pfn = folio_pfn(folio);
-	pte_t ptent = ptep_get(pte);
-
-	if (!folio_test_large(folio))
+	if (likely(!folio_test_large(folio)))
 		return 1;
 
-	count = pfn + nr - pte_pfn(ptent);
-	count = min_t(unsigned int, count, (end - addr) >> PAGE_SHIFT);
-
-	for (i = 0; i < count; i++, pte++) {
-		pte_t entry = ptep_get(pte);
-
-		if (!pte_present(entry))
-			break;
-		if (pte_pfn(entry) - pfn >= nr)
-			break;
-	}
+	const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
+	int max_nr = (end - addr) / PAGE_SIZE;
+	pte_t ptent = ptep_get(pte);
 
-	return i;
+	return folio_pte_batch(folio, addr, pte, ptent, max_nr, fpb_flags, NULL,
+			       NULL, NULL);
 }
 
 static inline bool allow_mlock_munlock(struct folio *folio,
-- 
2.33.1

Re: [PATCH 1/1] mm/mlock: implement folio_mlock_step() using folio_pte_batch()

[Matthew Wilcox]

On Mon, Jun 03, 2024 at 11:31:17AM +0800, Lance Yang wrote:
>  {
> -	unsigned int count, i, nr = folio_nr_pages(folio);
> -	unsigned long pfn = folio_pfn(folio);
> -	pte_t ptent = ptep_get(pte);

Please don't move type declarations later in the function.  Just because
you can doesn't mean you should.

> -	if (!folio_test_large(folio))
> +	if (likely(!folio_test_large(folio)))
>  		return 1;

How likely is this now?  How likely will it be in two years time?
Does this actually make any difference in either code generation or
performance?

Re: [PATCH 1/1] mm/mlock: implement folio_mlock_step() using folio_pte_batch()

[Lance Yang]

Hi Matthew,

Thanks for taking time to review!

On Mon, Jun 3, 2024 at 11:36 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Jun 03, 2024 at 11:31:17AM +0800, Lance Yang wrote:
> >  {
> > -     unsigned int count, i, nr = folio_nr_pages(folio);
> > -     unsigned long pfn = folio_pfn(folio);
> > -     pte_t ptent = ptep_get(pte);
>
> Please don't move type declarations later in the function.  Just because
> you can doesn't mean you should.

Thanks for pointing this out, I'll adjust as you suggested.

>
> > -     if (!folio_test_large(folio))
> > +     if (likely(!folio_test_large(folio)))
> >               return 1;
>
> How likely is this now?  How likely will it be in two years time?
> Does this actually make any difference in either code generation or
> performance?

IMO, this hint could impact code generation and performance :)
But it seems that 'likely' is not necessary here. I'll remove it.

Thanks again for your time!
Lance

>

Re: [PATCH 1/1] mm/mlock: implement folio_mlock_step() using folio_pte_batch()

[Barry Song]

On Mon, Jun 3, 2024 at 3:31 PM Lance Yang <ioworker0@gmail.com> wrote:
>
> Let's make folio_mlock_step() simply a wrapper around folio_pte_batch(),
> which will greatly reduce the cost of ptep_get() when scanning a range of
> contptes.
>
> Signed-off-by: Lance Yang <ioworker0@gmail.com>
> ---
>  mm/mlock.c | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 30b51cdea89d..1ae6232d38cf 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -307,26 +307,15 @@ void munlock_folio(struct folio *folio)
>  static inline unsigned int folio_mlock_step(struct folio *folio,
>                 pte_t *pte, unsigned long addr, unsigned long end)
>  {
> -       unsigned int count, i, nr = folio_nr_pages(folio);
> -       unsigned long pfn = folio_pfn(folio);
> -       pte_t ptent = ptep_get(pte);
> -
> -       if (!folio_test_large(folio))
> +       if (likely(!folio_test_large(folio)))
>                 return 1;
>
> -       count = pfn + nr - pte_pfn(ptent);
> -       count = min_t(unsigned int, count, (end - addr) >> PAGE_SHIFT);
> -
> -       for (i = 0; i < count; i++, pte++) {
> -               pte_t entry = ptep_get(pte);
> -
> -               if (!pte_present(entry))
> -                       break;
> -               if (pte_pfn(entry) - pfn >= nr)
> -                       break;
> -       }
> +       const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> +       int max_nr = (end - addr) / PAGE_SIZE;
> +       pte_t ptent = ptep_get(pte);
>
> -       return i;
> +       return folio_pte_batch(folio, addr, pte, ptent, max_nr, fpb_flags, NULL,
> +                              NULL, NULL);
>  }

what about a minimum change as below?
index 30b51cdea89d..e8b98f84fbd2 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -307,26 +307,15 @@ void munlock_folio(struct folio *folio)
 static inline unsigned int folio_mlock_step(struct folio *folio,
                pte_t *pte, unsigned long addr, unsigned long end)
 {
-       unsigned int count, i, nr = folio_nr_pages(folio);
-       unsigned long pfn = folio_pfn(folio);
+       unsigned int count = (end - addr) >> PAGE_SHIFT;
        pte_t ptent = ptep_get(pte);
+       const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;

        if (!folio_test_large(folio))
                return 1;

-       count = pfn + nr - pte_pfn(ptent);
-       count = min_t(unsigned int, count, (end - addr) >> PAGE_SHIFT);
-
-       for (i = 0; i < count; i++, pte++) {
-               pte_t entry = ptep_get(pte);
-
-               if (!pte_present(entry))
-                       break;
-               if (pte_pfn(entry) - pfn >= nr)
-                       break;
-       }
-
-       return i;
+       return folio_pte_batch(folio, addr, pte, ptent, count, fpb_flags, NULL,
+                              NULL, NULL);
 }



>
>  static inline bool allow_mlock_munlock(struct folio *folio,
> --
> 2.33.1
>

Re: [PATCH 1/1] mm/mlock: implement folio_mlock_step() using folio_pte_batch()

[Lance Yang]

Hi Barry,

Thanks for taking time to review!

On Mon, Jun 3, 2024 at 12:14 PM Barry Song <21cnbao@gmail.com> wrote:
>
> On Mon, Jun 3, 2024 at 3:31 PM Lance Yang <ioworker0@gmail.com> wrote:
> >
> > Let's make folio_mlock_step() simply a wrapper around folio_pte_batch(),
> > which will greatly reduce the cost of ptep_get() when scanning a range of
> > contptes.
> >
> > Signed-off-by: Lance Yang <ioworker0@gmail.com>
> > ---
> >  mm/mlock.c | 23 ++++++-----------------
> >  1 file changed, 6 insertions(+), 17 deletions(-)
> >
> > diff --git a/mm/mlock.c b/mm/mlock.c
> > index 30b51cdea89d..1ae6232d38cf 100644
> > --- a/mm/mlock.c
> > +++ b/mm/mlock.c
> > @@ -307,26 +307,15 @@ void munlock_folio(struct folio *folio)
> >  static inline unsigned int folio_mlock_step(struct folio *folio,
> >                 pte_t *pte, unsigned long addr, unsigned long end)
> >  {
> > -       unsigned int count, i, nr = folio_nr_pages(folio);
> > -       unsigned long pfn = folio_pfn(folio);
> > -       pte_t ptent = ptep_get(pte);
> > -
> > -       if (!folio_test_large(folio))
> > +       if (likely(!folio_test_large(folio)))
> >                 return 1;
> >
> > -       count = pfn + nr - pte_pfn(ptent);
> > -       count = min_t(unsigned int, count, (end - addr) >> PAGE_SHIFT);
> > -
> > -       for (i = 0; i < count; i++, pte++) {
> > -               pte_t entry = ptep_get(pte);
> > -
> > -               if (!pte_present(entry))
> > -                       break;
> > -               if (pte_pfn(entry) - pfn >= nr)
> > -                       break;
> > -       }
> > +       const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> > +       int max_nr = (end - addr) / PAGE_SIZE;
> > +       pte_t ptent = ptep_get(pte);
> >
> > -       return i;
> > +       return folio_pte_batch(folio, addr, pte, ptent, max_nr, fpb_flags, NULL,
> > +                              NULL, NULL);
> >  }
>
> what about a minimum change as below?

Nice, that makes sense to me ;)
I'll adjust as you suggested.

Thanks again for your time!
Lance

> index 30b51cdea89d..e8b98f84fbd2 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -307,26 +307,15 @@ void munlock_folio(struct folio *folio)
>  static inline unsigned int folio_mlock_step(struct folio *folio,
>                 pte_t *pte, unsigned long addr, unsigned long end)
>  {
> -       unsigned int count, i, nr = folio_nr_pages(folio);
> -       unsigned long pfn = folio_pfn(folio);
> +       unsigned int count = (end - addr) >> PAGE_SHIFT;
>         pte_t ptent = ptep_get(pte);
> +       const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>
>         if (!folio_test_large(folio))
>                 return 1;
>
> -       count = pfn + nr - pte_pfn(ptent);
> -       count = min_t(unsigned int, count, (end - addr) >> PAGE_SHIFT);
> -
> -       for (i = 0; i < count; i++, pte++) {
> -               pte_t entry = ptep_get(pte);
> -
> -               if (!pte_present(entry))
> -                       break;
> -               if (pte_pfn(entry) - pfn >= nr)
> -                       break;
> -       }
> -
> -       return i;
> +       return folio_pte_batch(folio, addr, pte, ptent, count, fpb_flags, NULL,
> +                              NULL, NULL);
>  }
>
>
>
> >
> >  static inline bool allow_mlock_munlock(struct folio *folio,
> > --
> > 2.33.1
> >

Re: [PATCH 1/1] mm/mlock: implement folio_mlock_step() using folio_pte_batch()

[Baolin Wang]

On 2024/6/3 12:14, Barry Song wrote:
> On Mon, Jun 3, 2024 at 3:31 PM Lance Yang <ioworker0@gmail.com> wrote:
>>
>> Let's make folio_mlock_step() simply a wrapper around folio_pte_batch(),
>> which will greatly reduce the cost of ptep_get() when scanning a range of
>> contptes.
>>
>> Signed-off-by: Lance Yang <ioworker0@gmail.com>
>> ---
>>   mm/mlock.c | 23 ++++++-----------------
>>   1 file changed, 6 insertions(+), 17 deletions(-)
>>
>> diff --git a/mm/mlock.c b/mm/mlock.c
>> index 30b51cdea89d..1ae6232d38cf 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -307,26 +307,15 @@ void munlock_folio(struct folio *folio)
>>   static inline unsigned int folio_mlock_step(struct folio *folio,
>>                  pte_t *pte, unsigned long addr, unsigned long end)
>>   {
>> -       unsigned int count, i, nr = folio_nr_pages(folio);
>> -       unsigned long pfn = folio_pfn(folio);
>> -       pte_t ptent = ptep_get(pte);
>> -
>> -       if (!folio_test_large(folio))
>> +       if (likely(!folio_test_large(folio)))
>>                  return 1;
>>
>> -       count = pfn + nr - pte_pfn(ptent);
>> -       count = min_t(unsigned int, count, (end - addr) >> PAGE_SHIFT);
>> -
>> -       for (i = 0; i < count; i++, pte++) {
>> -               pte_t entry = ptep_get(pte);
>> -
>> -               if (!pte_present(entry))
>> -                       break;
>> -               if (pte_pfn(entry) - pfn >= nr)
>> -                       break;
>> -       }
>> +       const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>> +       int max_nr = (end - addr) / PAGE_SIZE;
>> +       pte_t ptent = ptep_get(pte);
>>
>> -       return i;
>> +       return folio_pte_batch(folio, addr, pte, ptent, max_nr, fpb_flags, NULL,
>> +                              NULL, NULL);
>>   }
> 
> what about a minimum change as below?
> index 30b51cdea89d..e8b98f84fbd2 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -307,26 +307,15 @@ void munlock_folio(struct folio *folio)
>   static inline unsigned int folio_mlock_step(struct folio *folio,
>                  pte_t *pte, unsigned long addr, unsigned long end)
>   {
> -       unsigned int count, i, nr = folio_nr_pages(folio);
> -       unsigned long pfn = folio_pfn(folio);
> +       unsigned int count = (end - addr) >> PAGE_SHIFT;
>          pte_t ptent = ptep_get(pte);
> +       const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> 
>          if (!folio_test_large(folio))
>                  return 1;
> 
> -       count = pfn + nr - pte_pfn(ptent);
> -       count = min_t(unsigned int, count, (end - addr) >> PAGE_SHIFT);
> -
> -       for (i = 0; i < count; i++, pte++) {
> -               pte_t entry = ptep_get(pte);
> -
> -               if (!pte_present(entry))
> -                       break;
> -               if (pte_pfn(entry) - pfn >= nr)
> -                       break;
> -       }
> -
> -       return i;
> +       return folio_pte_batch(folio, addr, pte, ptent, count, fpb_flags, NULL,
> +                              NULL, NULL);
>   }

LGTM.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>

Re: [PATCH 1/1] mm/mlock: implement folio_mlock_step() using folio_pte_batch()

[David Hildenbrand]

On 03.06.24 10:58, Baolin Wang wrote:
> 
> 
> On 2024/6/3 12:14, Barry Song wrote:
>> On Mon, Jun 3, 2024 at 3:31 PM Lance Yang <ioworker0@gmail.com> wrote:
>>>
>>> Let's make folio_mlock_step() simply a wrapper around folio_pte_batch(),
>>> which will greatly reduce the cost of ptep_get() when scanning a range of
>>> contptes.
>>>
>>> Signed-off-by: Lance Yang <ioworker0@gmail.com>
>>> ---
>>>    mm/mlock.c | 23 ++++++-----------------
>>>    1 file changed, 6 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>> index 30b51cdea89d..1ae6232d38cf 100644
>>> --- a/mm/mlock.c
>>> +++ b/mm/mlock.c
>>> @@ -307,26 +307,15 @@ void munlock_folio(struct folio *folio)
>>>    static inline unsigned int folio_mlock_step(struct folio *folio,
>>>                   pte_t *pte, unsigned long addr, unsigned long end)
>>>    {
>>> -       unsigned int count, i, nr = folio_nr_pages(folio);
>>> -       unsigned long pfn = folio_pfn(folio);
>>> -       pte_t ptent = ptep_get(pte);
>>> -
>>> -       if (!folio_test_large(folio))
>>> +       if (likely(!folio_test_large(folio)))
>>>                   return 1;
>>>
>>> -       count = pfn + nr - pte_pfn(ptent);
>>> -       count = min_t(unsigned int, count, (end - addr) >> PAGE_SHIFT);
>>> -
>>> -       for (i = 0; i < count; i++, pte++) {
>>> -               pte_t entry = ptep_get(pte);
>>> -
>>> -               if (!pte_present(entry))
>>> -                       break;
>>> -               if (pte_pfn(entry) - pfn >= nr)
>>> -                       break;
>>> -       }
>>> +       const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>> +       int max_nr = (end - addr) / PAGE_SIZE;
>>> +       pte_t ptent = ptep_get(pte);
>>>
>>> -       return i;
>>> +       return folio_pte_batch(folio, addr, pte, ptent, max_nr, fpb_flags, NULL,
>>> +                              NULL, NULL);
>>>    }
>>
>> what about a minimum change as below?
>> index 30b51cdea89d..e8b98f84fbd2 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -307,26 +307,15 @@ void munlock_folio(struct folio *folio)
>>    static inline unsigned int folio_mlock_step(struct folio *folio,
>>                   pte_t *pte, unsigned long addr, unsigned long end)
>>    {
>> -       unsigned int count, i, nr = folio_nr_pages(folio);
>> -       unsigned long pfn = folio_pfn(folio);
>> +       unsigned int count = (end - addr) >> PAGE_SHIFT;
>>           pte_t ptent = ptep_get(pte);
>> +       const fpb_t fpb_flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>
>>           if (!folio_test_large(folio))
>>                   return 1;
>>
>> -       count = pfn + nr - pte_pfn(ptent);
>> -       count = min_t(unsigned int, count, (end - addr) >> PAGE_SHIFT);
>> -
>> -       for (i = 0; i < count; i++, pte++) {
>> -               pte_t entry = ptep_get(pte);
>> -
>> -               if (!pte_present(entry))
>> -                       break;
>> -               if (pte_pfn(entry) - pfn >= nr)
>> -                       break;
>> -       }
>> -
>> -       return i;
>> +       return folio_pte_batch(folio, addr, pte, ptent, count, fpb_flags, NULL,
>> +                              NULL, NULL);
>>    }
> 
> LGTM.
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> 

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

-- 
Cheers,

David / dhildenb