2024-06-03 03:31:56

by Lance Yang

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

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 <[email protected]>
---
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

2024-06-03 03:37:06

by Matthew Wilcox

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

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?

2024-06-03 04:13:47

by Lance Yang

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

Hi Matthew,

Thanks for taking time to review!

On Mon, Jun 3, 2024 at 11:36 AM Matthew Wilcox <[email protected]> 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

>

2024-06-03 04:15:00

by Barry Song

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

On Mon, Jun 3, 2024 at 3:31 PM Lance Yang <[email protected]> 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 <[email protected]>
> ---
> 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
>

2024-06-03 04:27:35

by Lance Yang

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

Hi Barry,

Thanks for taking time to review!

On Mon, Jun 3, 2024 at 12:14 PM Barry Song <[email protected]> wrote:
>
> On Mon, Jun 3, 2024 at 3:31 PM Lance Yang <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >

2024-06-03 08:59:19

by Baolin Wang

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

On 2024/6/3 12:14, Barry Song wrote:
> On Mon, Jun 3, 2024 at 3:31 PM Lance Yang <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>

2024-06-03 09:04:16

by David Hildenbrand

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

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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>

Acked-by: David Hildenbrand <[email protected]>

--
Cheers,

David / dhildenb