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