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@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
Thanks.
