| Message ID | 20221227122714.161224-3-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: convert page_idle/damon to use folios | expand |
Hi Kefeng, Could we use 'mm/damon:' prefix instead of 'mm: damon:' for the patch subjects? On Tue, 27 Dec 2022 20:27:09 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > Introduce damon_get_folio(), and the temporary wrapper function > damon_get_page(), which help us to convert damon related function > to use folios, and it will be droped once the conversion is completed. > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > mm/damon/ops-common.c | 14 ++++++++------ > mm/damon/ops-common.h | 8 +++++++- > 2 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c > index 75409601f934..ff38a19aa92e 100644 > --- a/mm/damon/ops-common.c > +++ b/mm/damon/ops-common.c > @@ -16,21 +16,23 @@ > * Get an online page for a pfn if it's in the LRU list. Otherwise, returns > * NULL. > * > - * The body of this function is stolen from the 'page_idle_get_page()'. We > + * The body of this function is stolen from the 'page_idle_get_folio()'. We > * steal rather than reuse it because the code is quite simple. > */ > -struct page *damon_get_page(unsigned long pfn) > +struct folio *damon_get_folio(unsigned long pfn) > { > struct page *page = pfn_to_online_page(pfn); > + struct folio *folio; > > if (!page || !PageLRU(page) || !get_page_unless_zero(page)) > return NULL; > > - if (unlikely(!PageLRU(page))) { > - put_page(page); > - page = NULL; > + folio = page_folio(page); > + if (unlikely(!folio_test_lru(folio))) { > + folio_put(folio); > + folio = NULL; > } I think Matthew's comment for 'page_idle_get_folio()'[1] could applied here? [1] https://lore.kernel.org/damon/Y6s2HPjrON2Sx%2Fgr@casper.infradead.org/ > - return page; > + return folio; > } > > void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) > diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h > index 8d82d3722204..4ee607813981 100644 > --- a/mm/damon/ops-common.h > +++ b/mm/damon/ops-common.h > @@ -7,7 +7,13 @@ > > #include <linux/damon.h> > > -struct page *damon_get_page(unsigned long pfn); > +struct folio *damon_get_folio(unsigned long pfn); > +static inline struct page *damon_get_page(unsigned long pfn) > +{ > + struct folio *folio = damon_get_folio(pfn); > + > + return &folio->page; I think we should check if folio is NULL before dereferencing it? > +} > > void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr); > void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr); > -- > 2.35.3 >
On Tue, Dec 27, 2022 at 07:42:57PM +0000, SeongJae Park wrote: > > +static inline struct page *damon_get_page(unsigned long pfn) > > +{ > > + struct folio *folio = damon_get_folio(pfn); > > + > > + return &folio->page; > > I think we should check if folio is NULL before dereferencing it? &folio->page does not dereeference folio.
On Tue, 27 Dec 2022 19:49:56 +0000 Matthew Wilcox <willy@infradead.org> wrote: > On Tue, Dec 27, 2022 at 07:42:57PM +0000, SeongJae Park wrote: > > > +static inline struct page *damon_get_page(unsigned long pfn) > > > +{ > > > + struct folio *folio = damon_get_folio(pfn); > > > + > > > + return &folio->page; > > > > I think we should check if folio is NULL before dereferencing it? > > &folio->page does not dereeference folio. Ah, ok. And this is safe because ->page is at the beginning of folio, right? Kefeng, could we add a comment explaining it, for people having bad eyes like me? Thanks, SJ
On 2022/12/28 3:42, SeongJae Park wrote: > Hi Kefeng, > > Could we use 'mm/damon:' prefix instead of 'mm: damon:' for the patch subjects? Sure. > > On Tue, 27 Dec 2022 20:27:09 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> Introduce damon_get_folio(), and the temporary wrapper function >> damon_get_page(), which help us to convert damon related function >> to use folios, and it will be droped once the conversion is completed. >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> mm/damon/ops-common.c | 14 ++++++++------ >> mm/damon/ops-common.h | 8 +++++++- >> 2 files changed, 15 insertions(+), 7 deletions(-) >> >> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c >> index 75409601f934..ff38a19aa92e 100644 >> --- a/mm/damon/ops-common.c >> +++ b/mm/damon/ops-common.c >> @@ -16,21 +16,23 @@ >> * Get an online page for a pfn if it's in the LRU list. Otherwise, returns >> * NULL. >> * >> - * The body of this function is stolen from the 'page_idle_get_page()'. We >> + * The body of this function is stolen from the 'page_idle_get_folio()'. We >> * steal rather than reuse it because the code is quite simple. >> */ >> -struct page *damon_get_page(unsigned long pfn) >> +struct folio *damon_get_folio(unsigned long pfn) >> { >> struct page *page = pfn_to_online_page(pfn); >> + struct folio *folio; >> >> if (!page || !PageLRU(page) || !get_page_unless_zero(page)) >> return NULL; >> >> - if (unlikely(!PageLRU(page))) { >> - put_page(page); >> - page = NULL; >> + folio = page_folio(page); >> + if (unlikely(!folio_test_lru(folio))) { >> + folio_put(folio); >> + folio = NULL; >> } > > I think Matthew's comment for 'page_idle_get_folio()'[1] could applied here? > Will change too. > > [1] https://lore.kernel.org/damon/Y6s2HPjrON2Sx%2Fgr@casper.infradead.org/ > >> - return page; >> + return folio; >> } >> >> void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) >> diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h >> index 8d82d3722204..4ee607813981 100644 >> --- a/mm/damon/ops-common.h >> +++ b/mm/damon/ops-common.h >> @@ -7,7 +7,13 @@ >> >> #include <linux/damon.h> >> >> -struct page *damon_get_page(unsigned long pfn); >> +struct folio *damon_get_folio(unsigned long pfn); >> +static inline struct page *damon_get_page(unsigned long pfn) >> +{ >> + struct folio *folio = damon_get_folio(pfn); >> + >> + return &folio->page; > > I think we should check if folio is NULL before dereferencing it? I could add comment here, thanks. > >> +} >> >> void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr); >> void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr); >> -- >> 2.35.3 >>
diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c index 75409601f934..ff38a19aa92e 100644 --- a/mm/damon/ops-common.c +++ b/mm/damon/ops-common.c @@ -16,21 +16,23 @@ * Get an online page for a pfn if it's in the LRU list. Otherwise, returns * NULL. * - * The body of this function is stolen from the 'page_idle_get_page()'. We + * The body of this function is stolen from the 'page_idle_get_folio()'. We * steal rather than reuse it because the code is quite simple. */ -struct page *damon_get_page(unsigned long pfn) +struct folio *damon_get_folio(unsigned long pfn) { struct page *page = pfn_to_online_page(pfn); + struct folio *folio; if (!page || !PageLRU(page) || !get_page_unless_zero(page)) return NULL; - if (unlikely(!PageLRU(page))) { - put_page(page); - page = NULL; + folio = page_folio(page); + if (unlikely(!folio_test_lru(folio))) { + folio_put(folio); + folio = NULL; } - return page; + return folio; } void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr) diff --git a/mm/damon/ops-common.h b/mm/damon/ops-common.h index 8d82d3722204..4ee607813981 100644 --- a/mm/damon/ops-common.h +++ b/mm/damon/ops-common.h @@ -7,7 +7,13 @@ #include <linux/damon.h> -struct page *damon_get_page(unsigned long pfn); +struct folio *damon_get_folio(unsigned long pfn); +static inline struct page *damon_get_page(unsigned long pfn) +{ + struct folio *folio = damon_get_folio(pfn); + + return &folio->page; +} void damon_ptep_mkold(pte_t *pte, struct mm_struct *mm, unsigned long addr); void damon_pmdp_mkold(pmd_t *pmd, struct mm_struct *mm, unsigned long addr);
Introduce damon_get_folio(), and the temporary wrapper function damon_get_page(), which help us to convert damon related function to use folios, and it will be droped once the conversion is completed. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- mm/damon/ops-common.c | 14 ++++++++------ mm/damon/ops-common.h | 8 +++++++- 2 files changed, 15 insertions(+), 7 deletions(-)