| Message ID | 20240603033118.76457-1-ioworker0@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] mm/mlock: implement folio_mlock_step() using folio_pte_batch() | expand |
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?
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 >
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 >
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 > >
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>
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>
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,
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(-)