On Thu, 24 Dec 2020 08:11:11 +0100 SeongJae Park [off-list ref] wrote:

On Wed, 23 Dec 2020 14:54:02 -0800 Shakeel Butt [off-list ref] wrote:

quoted

On Wed, Dec 23, 2020 at 8:47 AM SeongJae Park [off-list ref] wrote:

quoted

[snip]

quoted

quoted

[snip]

quoted

+
+static bool damon_va_young(struct mm_struct *mm, unsigned long addr,
+                       unsigned long *page_sz)
+{
+       pte_t *pte = NULL;
+       pmd_t *pmd = NULL;
+       spinlock_t *ptl;
+       bool young = false;
+
+       if (follow_pte_pmd(mm, addr, NULL, &pte, &pmd, &ptl))
+               return false;
+
+       *page_sz = PAGE_SIZE;
+       if (pte) {
+               young = pte_young(*pte);
+               if (!young)
+                       young = !page_is_idle(pte_page(*pte));
+               pte_unmap_unlock(pte, ptl);
+               return young;
+       }
+
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+       young = pmd_young(*pmd);
+       if (!young)
+               young = !page_is_idle(pmd_page(*pmd));
+       spin_unlock(ptl);
+       *page_sz = ((1UL) << HPAGE_PMD_SHIFT);
+#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
+
+       return young;

You need mmu_notifier_test_young() here. Hmm I remember mentioning
this in some previous version as well.

Your question and my answer was as below:

    > Don't you need mmu_notifier_clear_young() here?

    I think we don't need it here because we only read the Accessed bit and PG_Idle
    if Accessed bit was not set.

I should notice that you mean 'test_young()' but didn't, sorry.  I will add it
in the next version.

I should have said mmu_notifier_test_young() instead of
mmu_notifier_clear_young().

quoted

quoted

BTW have you tested this on a VM?

Yes.  Indeed, I'm testing this on a QEMU/KVM environment.  You can get more
detail at: https://damonitor.github.io/doc/html/latest/vm/damon/eval.html#setup

Hmm without mmu_notifier_test_young() you should be missing the kvm
mmu access updates. Can you please recheck if your eval is correctly
seeing the memory accesses from the VM?

Seems I didn't clearly answered, sorry.  My test setup installs the
DAMON-enabled kernel in a guest VM and run it for workloads in the guest,
rather than running DAMON in host to monitor accesses of VMs.  The MMU notifier
is for latter case, AFAIU, so my test setup didn't see the problem.

Just FYI.  I confirmed the mmu_notifier_test_young() added version works for
the use case.  I tested it by running a program accessing 200MB memory in a
QEMU/KVM guest having 120GB memory and monitoring the qemu process' virtual
address space from the host using DAMON.  The 200MB memory region was clearly
identifiable.

Thanks,
SeongJae Park