Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210624102623.24563-1-sjpark@amazon.de> <20210624102623.24563-3-sjpark@amazon.de>
In-Reply-To: <20210624102623.24563-3-sjpark@amazon.de>
From:   Shakeel Butt <shakeelb@google.com>
Date:   Thu, 24 Jun 2021 07:42:44 -0700
Message-ID: <CALvZod5dFVxJVFUP4zBCC97C7rr5pGjRZQoixzs=GcNRAosKgw@mail.gmail.com>
Subject: Re: [PATCH v31 05/13] mm/damon: Implement primitives for the virtual
 memory address spaces
To:     SeongJae Park <sj38.park@gmail.com>
Cc:     SeongJae Park <sjpark@amazon.de>, Jonathan.Cameron@huawei.com,
        acme@kernel.org, alexander.shishkin@linux.intel.com,
        amit@kernel.org, benh@kernel.crashing.org,
        Brendan Higgins <brendanhiggins@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        David Hildenbrand <david@redhat.com>, dwmw@amazon.com,
        Marco Elver <elver@google.com>, "Du, Fan" <fan.du@intel.com>,
        foersleo@amazon.de, greg@kroah.com,
        Greg Thelen <gthelen@google.com>, guoju.fgj@alibaba-inc.com,
        jgowans@amazon.com, Mel Gorman <mgorman@suse.de>, mheyne@amazon.de,
        Minchan Kim <minchan@kernel.org>,
        Ingo Molnar <mingo@redhat.com>, namhyung@kernel.org,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Rik van Riel <riel@surriel.com>,
        David Rientjes <rientjes@google.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mike Rapoport <rppt@kernel.org>, Shuah Khan <shuah@kernel.org>,
        sieberf@amazon.com, snu@zelle79.org,
        Vlastimil Babka <vbabka@suse.cz>,
        Vladimir Davydov <vdavydov.dev@gmail.com>,
        zgf574564920@gmail.com, linux-damon@amazon.com,
        Linux MM <linux-mm@kvack.org>, linux-doc@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jun 24, 2021 at 3:26 AM SeongJae Park <sj38.park@gmail.com> wrote:
>
[...]
> > > +/*
> > > + * Get the three regions in the given target (task)
> > > + *
> > > + * Returns 0 on success, negative error code otherwise.
> > > + */
> > > +static int damon_va_three_regions(struct damon_target *t,
> > > +                               struct damon_addr_range regions[3])
> > > +{
> > > +       struct mm_struct *mm;
> > > +       int rc;
> > > +
> > > +       mm = damon_get_mm(t);
> > > +       if (!mm)
> > > +               return -EINVAL;
> > > +
> > > +       mmap_read_lock(mm);
> > > +       rc = __damon_va_three_regions(mm->mmap, regions);
> > > +       mmap_read_unlock(mm);
> >
> > This is being called for each target every second by default. Seems
> > too aggressive. Applications don't change their address space every
> > second. I would recommend to default ctx->primitive_update_interval to
> > a higher default value.
>
> Good point.  If there are many targets and each target has a huge number of
> VMAs, the overhead could be high.  Nevertheless, I couldn't find the overhead
> in my test setup.  Also, it seems someone are already started exploring DAMON
> patchset with the default value. and usages from others.  Silently changing the
> default value could distract such people.  So, if you think it's ok, I'd like
> to change the default value only after someone finds the overhead from their
> usages and asks a change.
>
> If you disagree or you found the overhead from your usage, please feel free to
> let me know.
>

mmap lock is a source contention in the real world workloads. We do
observe in our fleet and many others (like Facebook) do complain on
this issue. This is the whole motivation behind SFP, maple tree and
many other mmap lock scalability work. I would be really careful to
add another source of contention on mmap lock. Yes, the user can
change this interval themselves but we should not burden them with
this internal knowledge like "oh if you observe high mmap contention
you may want to increase this specific interval". We should set a good
default value to avoid such situations (most of the time).