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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
