[PATCH] mm/damon: increment pa_stat damon address range by folio size

[PATCH] mm/damon: increment pa_stat damon address range by folio size

[Usama Arif]

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(-)

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;
-- 
2.43.5

Re: [PATCH] mm/damon: increment pa_stat damon address range by folio size

[SeongJae Park]

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

Re: [PATCH] mm/damon: increment pa_stat damon address range by folio size

[Usama Arif]

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

Re: [PATCH] mm/damon: increment pa_stat damon address range by folio size

[SeongJae Park]

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

[...]