| Message ID | 20221207002158.418789-3-vishal.moola@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Convert deactivate_page() to deactivate_folio() | expand |
Hi Vishal, Thank you for this patch. Nit pick. Could we please replace 'damon:' on the subject with 'mm/damon:' to look more consistent with other DAMON patches? On Tue, 6 Dec 2022 16:21:57 -0800 "Vishal Moola (Oracle)" <vishal.moola@gmail.com> wrote: > This change replaces 2 calls to compound_head() with one. I guess you mean _compound_head() calls in page_folio() that called from mark_page_accessed() and folio_mark_accessed(). However, deactivate_page() calls page_folio() anyway, so this patch doesn't reduce the number of calls to one but keep the number, correct? Am I missing something? If I'm not, I'd like to clean up the wording. > This is in preparation for the conversion of deactivate_page() to > deactivate_folio(). I think folio_deactivate() might be a more consistent naming. What do you think? Also, you may keep the above sentence if you implement folio_deactivate() first, update deactivate_page() calls to use folio_deactivate(), and finally remove deactivate_page() definition. I don't really care about the name and the sequence, though. > > Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com> > --- > mm/damon/paddr.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c > index e1a4315c4be6..73548bc82297 100644 > --- a/mm/damon/paddr.c > +++ b/mm/damon/paddr.c > @@ -238,15 +238,18 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate( > > for (addr = r->ar.start; addr < r->ar.end; addr += PAGE_SIZE) { > struct page *page = damon_get_page(PHYS_PFN(addr)); > + struct folio *folio; > > if (!page) > continue; > + folio = page_folio(page); One _compound_head() call here, > + > if (mark_accessed) > - mark_page_accessed(page); > + folio_mark_accessed(folio); > else > - deactivate_page(page); > - put_page(page); > - applied++; > + deactivate_page(&folio->page); And second _compound_head() call here. > + folio_put(folio); > + applied += folio_nr_pages(folio); > } > return applied * PAGE_SIZE; > } > -- > 2.38.1 > Thanks, SJ
On Wed, 7 Dec 2022 00:56:24 +0000 SeongJae Park <sj@kernel.org> wrote: > Hi Vishal, > > > Thank you for this patch. > > Nit pick. Could we please replace 'damon:' on the subject with 'mm/damon:' to > look more consistent with other DAMON patches? > > On Tue, 6 Dec 2022 16:21:57 -0800 "Vishal Moola (Oracle)" <vishal.moola@gmail.com> wrote: > > > This change replaces 2 calls to compound_head() with one. > > I guess you mean _compound_head() calls in page_folio() that called from > mark_page_accessed() and folio_mark_accessed(). s/folio_mark_accessed()/deactivate_page()/ Sorry for the typo. Thanks, SJ [...]
On Tue, Dec 6, 2022 at 4:56 PM SeongJae Park <sj@kernel.org> wrote: > > Hi Vishal, > > > Thank you for this patch. > > Nit pick. Could we please replace 'damon:' on the subject with 'mm/damon:' to > look more consistent with other DAMON patches? Thanks for looking over this patch! I'll make the subject mm/damon in a v2. > On Tue, 6 Dec 2022 16:21:57 -0800 "Vishal Moola (Oracle)" <vishal.moola@gmail.com> wrote: > > > This change replaces 2 calls to compound_head() with one. > > I guess you mean _compound_head() calls in page_folio() that called from > mark_page_accessed() and folio_mark_accessed(). However, deactivate_page() > calls page_folio() anyway, so this patch doesn't reduce the number of calls to > one but keep the number, correct? Am I missing something? If I'm not, I'd > like to clean up the wording. The 2 calls I was referring to were from mark_page_accessed() and put_page(). As you've noticed, deactivate_page() still calls page_folio() here :). > > This is in preparation for the conversion of deactivate_page() to > > deactivate_folio(). > > I think folio_deactivate() might be a more consistent naming. What do you > think? I do like the name folio_deactivate() better than deactivate_folio(), I'll change that in v2 as well.
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c index e1a4315c4be6..73548bc82297 100644 --- a/mm/damon/paddr.c +++ b/mm/damon/paddr.c @@ -238,15 +238,18 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate( for (addr = r->ar.start; addr < r->ar.end; addr += PAGE_SIZE) { struct page *page = damon_get_page(PHYS_PFN(addr)); + struct folio *folio; if (!page) continue; + folio = page_folio(page); + if (mark_accessed) - mark_page_accessed(page); + folio_mark_accessed(folio); else - deactivate_page(page); - put_page(page); - applied++; + deactivate_page(&folio->page); + folio_put(folio); + applied += folio_nr_pages(folio); } return applied * PAGE_SIZE; }
This change replaces 2 calls to compound_head() with one. This is in preparation for the conversion of deactivate_page() to deactivate_folio(). Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com> --- mm/damon/paddr.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)