lkml.org 
[lkml]   [2020]   [Jul]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: Re: Re: [PATCH v18 06/14] mm/damon: Implement callbacks for the virtual memory address spaces
    Date
    On Tue, 28 Jul 2020 10:42:11 -0700 Shakeel Butt <shakeelb@google.com> wrote:

    > On Mon, Jul 27, 2020 at 2:03 AM SeongJae Park <sjpark@amazon.com> wrote:
    > >
    > > On Mon, 27 Jul 2020 00:34:54 -0700 Greg Thelen <gthelen@google.com> wrote:
    > >
    > > > SeongJae Park <sjpark@amazon.com> wrote:
    > > >
    > > > > From: SeongJae Park <sjpark@amazon.de>
    > > > >
    > > > > 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.
    > > [...]
    > > > > diff --git a/mm/damon.c b/mm/damon.c
    > > > > index b844924b9fdb..386780739007 100644
    > > > > --- a/mm/damon.c
    > > > > +++ b/mm/damon.c
    > > > > @@ -9,6 +9,9 @@
    > > [...]
    > > > > +/*
    > > > > + * Functions for the access checking of the regions
    > > > > + */
    > > > > +
    > > > > +static void damon_mkold(struct mm_struct *mm, unsigned long addr)
    > > > > +{
    > > > > + pte_t *pte = NULL;
    > > > > + pmd_t *pmd = NULL;
    > > > > + spinlock_t *ptl;
    > > > > +
    > > > > + if (follow_pte_pmd(mm, addr, NULL, &pte, &pmd, &ptl))
    > > > > + return;
    > > > > +
    > > > > + if (pte) {
    > > > > + if (pte_young(*pte)) {
    > > > > + clear_page_idle(pte_page(*pte));
    > > > > + set_page_young(pte_page(*pte));
    > > >
    > > > While this compiles without support for PG_young and PG_idle, I assume
    > > > it won't work well because it'd clear pte.young without setting
    > > > PG_young. And this would mess with vmscan.
    > >
    > > You're right, thanks for catching this up! This definitely need to be fixed in
    > > the next spin.
    > >
    > > >
    > > > So this code appears to depend on PG_young and PG_idle, which are
    > > > currently only available via CONFIG_IDLE_PAGE_TRACKING. DAMON could
    > > > depend on CONFIG_IDLE_PAGE_TRACKING via Kconfig. But I assume that
    > > > CONFIG_IDLE_PAGE_TRACKING and CONFIG_DAMON cannot be concurrently used
    > > > because they'll stomp on each other's use of pte.young, PG_young,
    > > > PG_idle.
    > > > So I suspect we want:
    > > > 1. CONFIG_DAMON to depend on !CONFIG_IDLE_PAGE_TRACKING and vise-versa.
    > > > 2. PG_young,PG_idle and related helpers to depend on
    > > > CONFIG_DAMON||CONFIG_IDLE_PAGE_TRACKING.
    > >
    > > Awesome insights and suggestions, thanks!
    > >
    > > I would like to note that DAMON could be interfered by IDLE_PAGE_TRACKING and
    > > vmscan, but not vice versa, as DAMON respects PG_idle and PG_young. This
    > > design came from the weak goal of DAMON. DAMON aims to provide not perfect
    > > monitoring but only best effort accuracy that would be sufficient for
    > > performance-centric DRAM level memory management. So, at that time, I thought
    > > being interfered by IDLE_PAGE_TRACKING and the reclaim logic would not be a
    > > real problem but letting IDLE_PAGE_TRACKING coexist is somehow beneficial.
    > > That said, I couldn't find a real benefit of the coexistance yet, and the
    > > problem of being interference now seems bigger as we will support more cases
    > > including the page granularity.
    > >
    > > Maybe we could make IDLE_PAGE_TRACKING and DAMON coexist but mutual exclusive
    > > in runtime, if the beneficial of coexistance turns out big. However, I would
    > > like to make it simple first and optimize the case later if real requirement
    > > found.
    >
    > If you are planning to have support for tracking at page granularity
    > and physical memory monitoring in DAMON then I don't see any benefit
    > of coexistence of DAMON with IDLE_PAGE_TRACKING. Though I will not
    > push you to go that route if the code with coexistence is simple
    > enough.

    Agreed, I don't see the benefit, neither. I already selected the mutual
    exclusive way :)


    Thanks,
    SeongJae Park

    \
     
     \ /
      Last update: 2020-07-29 08:22    [W:3.784 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site