Re: [PATCH v23 05/15] mm/damon: Implement primitives for the virtual memory address spaces
On Wed, Jan 27, 2021 at 8:57 AM SeongJae Park wrote:
>
> On Thu, 24 Dec 2020 08:11:11 +0100 SeongJae Park wrote:
>
> > On Wed, 23 Dec 2020 14:54:02 -0800 Shakeel Butt wrote:
> >
> > > On Wed, Dec 23, 2020 at 8:47 AM SeongJae Park wrote:
> > > >
> > > [snip]
> > > > > [snip]
> > > > > > +
> > > > > > +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().
> > >
> > > > >
> > > > > 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 for confirming. I am still going over the whole series and will
send out the emails in a couple of days.
Shakeel
Re: [PATCH v23 05/15] mm/damon: Implement primitives for the virtual memory address spaces
On Thu, 24 Dec 2020 08:11:11 +0100 SeongJae Park wrote:
> On Wed, 23 Dec 2020 14:54:02 -0800 Shakeel Butt wrote:
>
> > On Wed, Dec 23, 2020 at 8:47 AM SeongJae Park wrote:
> > >
> > [snip]
> > > > [snip]
> > > > > +
> > > > > +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().
> >
> > > >
> > > > 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
Re: [PATCH v23 05/15] mm/damon: Implement primitives for the virtual memory address spaces
On Wed, 23 Dec 2020 14:54:02 -0800 Shakeel Butt wrote:
> On Wed, Dec 23, 2020 at 8:47 AM SeongJae Park wrote:
> >
> [snip]
> > > [snip]
> > > > +
> > > > +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().
>
> > >
> > > 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.
If I'm missing something, please let me know.
Thanks,
SeongJae Park
Re: [PATCH v23 05/15] mm/damon: Implement primitives for the virtual memory address spaces
On Wed, Dec 23, 2020 at 8:47 AM SeongJae Park wrote:
>
[snip]
> > [snip]
> > > +
> > > +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().
> >
> > 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?
Re: [PATCH v23 05/15] mm/damon: Implement primitives for the virtual memory address spaces
On Wed, 23 Dec 2020 07:31:45 -0800 Shakeel Butt wrote:
> On Tue, Dec 15, 2020 at 3:58 AM SeongJae Park wrote:
> >
> > From: SeongJae Park
> >
> > This commit introduces a reference implementation of the address space
> > specific low level primitives for the virtual address space, so that
> > users of DAMON can easily monitor the data accesses on virtual address
> > spaces of specific processes by simply configuring the implementation to
> > be used by DAMON.
> >
> > The low level primitives for the fundamental access monitoring are
> > defined in two parts:
> >
> > 1. Identification of the monitoring target address range for the address
> >space.
> > 2. Access check of specific address range in the target space.
> >
> > The reference implementation for the virtual address space does the
> > works as below.
> >
> > PTE Accessed-bit Based Access Check
> > ---
> >
> > The implementation uses PTE Accessed-bit for basic access checks. That
> > is, it clears the bit for next sampling target page and checks whether
>
> 'for the next'
>
> > it set again after one sampling period. This could disturb the reclaim
>
> 'it is set'
Good catch! Will fix in the next version.
>
> > logic. DAMON uses ``PG_idle`` and ``PG_young`` page flags to solve the
> > conflict, as Idle page tracking does.
> >
> > VMA-based Target Address Range Construction
> > ---
> >
> > Only small parts in the super-huge virtual address space of the
> > processes are mapped to physical memory and accessed. Thus, tracking
> > the unmapped address regions is just wasteful. However, because DAMON
> > can deal with some level of noise using the adaptive regions adjustment
> > mechanism, tracking every mapping is not strictly required but could
> > even incur a high overhead in some cases. That said, too huge unmapped
> > areas inside the monitoring target should be removed to not take the
> > time for the adaptive mechanism.
> >
> > For the reason, this implementation converts the complex mappings to
> > three distinct regions that cover every mapped area of the address
> > space. Also, the two gaps between the three regions are the two biggest
> > unmapped areas in the given address space. The two biggest unmapped
> > areas would be the gap between the heap and the uppermost mmap()-ed
> > region, and the gap between the lowermost mmap()-ed region and the stack
> > in most of the cases. Because these gaps are exceptionally huge in
> > usual address spacees, excluding these will be sufficient to make a
>
> *spaces
Good eye! Will fix in the next version.
>
> > reasonable trade-off. Below shows this in detail::
> >
> >
> >
> >
> > (small mmap()-ed regions and munmap()-ed regions)
> >
> >
> >
> >
> > Signed-off-by: SeongJae Park
> > Reviewed-by: Leonard Foerster
> > ---
> > include/linux/damon.h | 13 +
> > mm/damon/Kconfig | 9 +
> > mm/damon/Makefile | 1 +
> > mm/damon/vaddr.c | 579 ++
> > 4 files changed, 602 insertions(+)
> > create mode 100644 mm/damon/vaddr.c
> >
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index f446f8433599..39b4d6d3ddee 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -274,4 +274,17 @@ int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
> >
> > #endif /* CONFIG_DAMON */
> >
> > +#ifdef CONFIG_DAMON_VADDR
> > +
> > +/* Monitoring primitives for virtual memory address spaces */
> > +void damon_va_init_regions(struct damon_ctx *ctx);
> > +void damon_va_update_regions(struct damon_ctx *ctx);
> > +void damon_va_prepare_access_checks(struct damon_ctx *ctx);
> > +unsigned int damon_va_check_accesses(struct damon_ctx *ctx);
> > +bool damon_va_target_valid(void *t);
> > +void damon_va_cleanup(struct damon_ctx *ctx);
> > +void damon_va_set_primitives(struct damon_ctx *ctx);
>
> Any reason for these to be in the header?
To let DAMON API users (in kernel space) and other primitives developers to use
those.
>
> > +
> [snip]
> > +
> > +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);
> > + *pag
Re: [PATCH v23 05/15] mm/damon: Implement primitives for the virtual memory address spaces
On Tue, Dec 15, 2020 at 3:58 AM SeongJae Park wrote:
>
> From: SeongJae Park
>
> This commit introduces a reference implementation of the address space
> specific low level primitives for the virtual address space, so that
> users of DAMON can easily monitor the data accesses on virtual address
> spaces of specific processes by simply configuring the implementation to
> be used by DAMON.
>
> The low level primitives for the fundamental access monitoring are
> defined in two parts:
>
> 1. Identification of the monitoring target address range for the address
>space.
> 2. Access check of specific address range in the target space.
>
> The reference implementation for the virtual address space does the
> works as below.
>
> PTE Accessed-bit Based Access Check
> ---
>
> The implementation uses PTE Accessed-bit for basic access checks. That
> is, it clears the bit for next sampling target page and checks whether
'for the next'
> it set again after one sampling period. This could disturb the reclaim
'it is set'
> logic. DAMON uses ``PG_idle`` and ``PG_young`` page flags to solve the
> conflict, as Idle page tracking does.
>
> VMA-based Target Address Range Construction
> ---
>
> Only small parts in the super-huge virtual address space of the
> processes are mapped to physical memory and accessed. Thus, tracking
> the unmapped address regions is just wasteful. However, because DAMON
> can deal with some level of noise using the adaptive regions adjustment
> mechanism, tracking every mapping is not strictly required but could
> even incur a high overhead in some cases. That said, too huge unmapped
> areas inside the monitoring target should be removed to not take the
> time for the adaptive mechanism.
>
> For the reason, this implementation converts the complex mappings to
> three distinct regions that cover every mapped area of the address
> space. Also, the two gaps between the three regions are the two biggest
> unmapped areas in the given address space. The two biggest unmapped
> areas would be the gap between the heap and the uppermost mmap()-ed
> region, and the gap between the lowermost mmap()-ed region and the stack
> in most of the cases. Because these gaps are exceptionally huge in
> usual address spacees, excluding these will be sufficient to make a
*spaces
> reasonable trade-off. Below shows this in detail::
>
>
>
>
> (small mmap()-ed regions and munmap()-ed regions)
>
>
>
>
> Signed-off-by: SeongJae Park
> Reviewed-by: Leonard Foerster
> ---
> include/linux/damon.h | 13 +
> mm/damon/Kconfig | 9 +
> mm/damon/Makefile | 1 +
> mm/damon/vaddr.c | 579 ++
> 4 files changed, 602 insertions(+)
> create mode 100644 mm/damon/vaddr.c
>
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index f446f8433599..39b4d6d3ddee 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -274,4 +274,17 @@ int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
>
> #endif /* CONFIG_DAMON */
>
> +#ifdef CONFIG_DAMON_VADDR
> +
> +/* Monitoring primitives for virtual memory address spaces */
> +void damon_va_init_regions(struct damon_ctx *ctx);
> +void damon_va_update_regions(struct damon_ctx *ctx);
> +void damon_va_prepare_access_checks(struct damon_ctx *ctx);
> +unsigned int damon_va_check_accesses(struct damon_ctx *ctx);
> +bool damon_va_target_valid(void *t);
> +void damon_va_cleanup(struct damon_ctx *ctx);
> +void damon_va_set_primitives(struct damon_ctx *ctx);
Any reason for these to be in the header?
> +
[snip]
> +
> +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.
BTW have you tested this on a VM?
The patch looks good overall.
