| Message ID | 20250113180340.805474-1-usamaarif642@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/damon: increment pa_stat damon address range by folio size | expand |
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@gmail.com> 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? > Hence go through the folio only once. I tink this is still nice to do, for more efficiency. Can you post v2 of this patch after updating the commit message and addressing below comments? > > Fixes: 6347f3385dd0 ("mm/damon/paddr: report filter-passed bytes back for DAMOS_STAT action") > Signed-off-by: Usama Arif <usamaarif642@gmail.com> > --- > mm/damon/paddr.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > 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. > @@ -515,6 +516,7 @@ static unsigned long damon_pa_stat(struct damon_region *r, struct damos *s, > else > *sz_filter_passed += folio_size(folio); > put_folio: > + addr += folio_size(folio); > folio_put(folio); > } > return 0; > -- > 2.43.5 Thanks, SJ
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@gmail.com> 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. > >> Hence go through the folio only once. > > I tink this is still nice to do, for more efficiency. Can you post v2 of this > patch after updating the commit message and addressing below comments? > >> >> Fixes: 6347f3385dd0 ("mm/damon/paddr: report filter-passed bytes back for DAMOS_STAT action") >> Signed-off-by: Usama Arif <usamaarif642@gmail.com> >> --- >> mm/damon/paddr.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> 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. > >> @@ -515,6 +516,7 @@ static unsigned long damon_pa_stat(struct damon_region *r, struct damos *s, >> else >> *sz_filter_passed += folio_size(folio); >> put_folio: >> + addr += folio_size(folio); >> folio_put(folio); >> } >> return 0; >> -- >> 2.43.5 > > > Thanks, > SJ
On Mon, 13 Jan 2025 18:53:32 +0000 Usama Arif <usamaarif642@gmail.com> 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@gmail.com> 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 [...]
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) @@ -515,6 +516,7 @@ static unsigned long damon_pa_stat(struct damon_region *r, struct damos *s, else *sz_filter_passed += folio_size(folio); put_folio: + addr += folio_size(folio); folio_put(folio); } return 0;
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. Hence go through the folio only once. Fixes: 6347f3385dd0 ("mm/damon/paddr: report filter-passed bytes back for DAMOS_STAT action") Signed-off-by: Usama Arif <usamaarif642@gmail.com> --- mm/damon/paddr.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)