Re: [PATCH] mm/damon: Validate if the pmd entry is present before accessing

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 8/18/2022 10:29 AM, SeongJae Park wrote:
> Hi Baolin,
> 
> On Thu, 18 Aug 2022 09:05:58 +0800 Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> 
>>
>>
>> On 8/18/2022 12:09 AM, SeongJae Park wrote:
>>> Hi Baolin,
>>>
>>>
>>> Thank you always for your great patch!
>>>
>>> On Wed, 17 Aug 2022 14:21:12 +0800 Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
>>>
>>>> The pmd_huge() is used to validate if the pmd entry is mapped by a huge
>>>> page, also including the case of non-present (migration or hwpoisoned)
>>>> pmd entry on arm64 or x86 architectures. Thus we should validate if it
>>>> is present before making the pmd entry old or getting young state,
>>>> otherwise we can not get the correct corresponding page.
>>>
>>> Maybe I'm missing something, but... I'm unsure if the page is present or not
>>> really matters from the perspective of access checking.  In the case, DAMON
>>> could simply report the page has accessed once for the first check after the
>>> page being non-present if it really accessed before, and then report the page
>>> as not accessed, which is true.
>>
>> Yes, that's the patch's goal to make the accesses correct. However if
>> the PMD entry is not present, we can not get the correct page object by
>> pmd_pfn(*pmd), since the non-present pmd entry will contain swap type
>> and swap offset with below format on ARM64, that means the pfn number is
>> saved in bits 8-57 in a migration or poisoned entry, but pmd_pfn() still
>> treat bits 12-47 as the pfn number on ARM64, which may get an incorrect
>> page struct (also maybe is NULL by pfn_to_online_page()) to make the
>> access statistics incorrect.
>>
>> /*
>>    * Encode and decode a swap entry:
>>    *	bits 0-1:	present (must be zero)
>>    *	bits 2:		remember PG_anon_exclusive
>>    *	bits 3-7:	swap type
>>    *	bits 8-57:	swap offset
>>    *	bit  58:	PTE_PROT_NONE (must be zero)
>>    */
>>
>>
>> Moreoever I don't think we should still waste time to get the page of
>> the non-present entry, just treat it as not-accessed and skip it, that
>> keeps consistent with non-present pte level entry.
>>
>> Does that make sense for you? Thanks.
> 
> Yes, that totally makes sense.  Thank you very much for the kind answer.  I
> think it would be great if we could put the detailed explanation in the commit
> message.  Could you please update the commit message and post v2 of the patch?

Sure, will update the commit message to make it more clear and I think 
that can also answer Andrew's concern.

> 
> Reviewed-by: SeongJae Park <sj@kernel.org>

Thanks.