On Mon, 13 Jan 2025 18:53:32 +0000 Usama Arif <usamaarif642@xxxxxxxxx> wrote:
>
>
> On 13/01/2025 18:43, SeongJae Park wrote:
> > Hi Usama,
> >
> >
> > Let's use "mm/damon/paddr: " prefix for the patch title, to be more consistent
> > with others.
> >
> > On Mon, 13 Jan 2025 18:03:40 +0000 Usama Arif <usamaarif642@xxxxxxxxx> wrote:
> >
> >> This is to take into account for folios with size > 1 page.
> >> Iterating at PAGE_SIZE increment would increment sz_filter_passed
> >> multiple times for the same folio by folio_size, providing incorrect
> >> stats.
> >
> > damon_get_folio() returns NULL if the page is a tail page. Hence I think it
> > will not increment sz_filter_passed multiple times?
>
> ahh I didn't look at the definition of damon_get_folio! just assumed it will get
> the folio irrespective of if its a tail page or not. Will change the commit message.
>
> Just curious if returning NULL is what is expected by the user?
> I see damon_get_folio used in a bunch a places. If the user limits damos action/
> damon monitoring to a specific address range, and that covers some of the tail pages,
> but not the head page, I guess the damos action wont be applied.
Yes, I think damon_get_folio() users (mostly DAMON operations set layer) should
aware that and use different functions if it can be a problem, depending on
each case. I find no problematic use cases of the function off the top of my
head, but if you find, please help us fixing those :)
[...]
> >> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> >> index 6b4397de4199..cc789a97c6f5 100644
> >> --- a/mm/damon/paddr.c
> >> +++ b/mm/damon/paddr.c
> >> @@ -504,7 +504,8 @@ static unsigned long damon_pa_stat(struct damon_region *r, struct damos *s,
> >> if (!damon_pa_scheme_has_filter(s))
> >> return 0;
> >>
> >> - for (addr = r->ar.start; addr < r->ar.end; addr += PAGE_SIZE) {
> >> + addr = r->ar.start;
> >> + while (addr < r->ar.end) {
> >> struct folio *folio = damon_get_folio(PHYS_PFN(addr));
> >>
> >> if (!folio)
> >
> > In this case, the code does "continue". 'addr' is not advanced, so it will
> > result in an infinite loop. Let's do 'addr += PAGE_SIZE' here, to avoid that.
>
> Thanks! Will fix this in v2.
Nice, looking forward to it!
Thanks,
SJ
[...]
