Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
From: Shakeel Butt <shakeelb@google.com>
To: SeongJae Park <sjpark@amazon.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	SeongJae Park <sjpark@amazon.de>,
	 Jonathan.Cameron@huawei.com,
	Andrea Arcangeli <aarcange@redhat.com>,
	acme@kernel.org,  alexander.shishkin@linux.intel.com,
	amit@kernel.org, benh@kernel.crashing.org,
	 brendan.d.gregg@gmail.com,
	Brendan Higgins <brendanhiggins@google.com>,
	 Qian Cai <cai@lca.pw>, Colin Ian King <colin.king@canonical.com>,
	Jonathan Corbet <corbet@lwn.net>,
	 David Hildenbrand <david@redhat.com>,
	dwmw@amazon.com, foersleo@amazon.de,
	 Ian Rogers <irogers@google.com>,
	jolsa@redhat.com,  "Kirill A. Shutemov" <kirill@shutemov.name>,
	mark.rutland@arm.com, Mel Gorman <mgorman@suse.de>,
	 Minchan Kim <minchan@kernel.org>, Ingo Molnar <mingo@redhat.com>,
	namhyung@kernel.org,
	 "Peter Zijlstra (Intel)" <peterz@infradead.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	 Rik van Riel <riel@surriel.com>,
	David Rientjes <rientjes@google.com>,
	 Steven Rostedt <rostedt@goodmis.org>,
	rppt@kernel.org, sblbir@amazon.com, shuah@kernel.org,
	 sj38.park@gmail.com, snu@amazon.de,
	Vlastimil Babka <vbabka@suse.cz>,
	 Vladimir Davydov <vdavydov.dev@gmail.com>,
	Yang Shi <yang.shi@linux.alibaba.com>,
	 Huang Ying <ying.huang@intel.com>,
	linux-damon@amazon.com,  Linux MM <linux-mm@kvack.org>,
	linux-doc@vger.kernel.org,  LKML <linux-kernel@vger.kernel.org>
Subject: Re: Re: [PATCH v18 02/14] mm: Introduce Data Access MONitor (DAMON)
Date: Wed, 29 Jul 2020 08:31:29 -0700
Message-ID: <CALvZod6cd4698njE+_dQ6UiL=uRL9xm=aKVm5=-sV28T9Y+fVw@mail.gmail.com> (raw)
In-Reply-To: <20200718133106.4787-1-sjpark@amazon.com>

On Sat, Jul 18, 2020 at 6:31 AM SeongJae Park <sjpark@amazon.com> wrote:
>
> On Fri, 17 Jul 2020 19:47:50 -0700 Shakeel Butt <shakeelb@google.com> wrote:
>
> > On Mon, Jul 13, 2020 at 1:43 AM SeongJae Park <sjpark@amazon.com> wrote:
> > >
> > > From: SeongJae Park <sjpark@amazon.de>
> > >
> > > DAMON is a data access monitoring framework subsystem for the Linux
> > > kernel.  The core mechanisms of DAMON make it
> > >
> > >  - accurate (the monitoring output is useful enough for DRAM level
> > >    memory management; It might not appropriate for CPU Cache levels,
> > >    though),
> > >  - light-weight (the monitoring overhead is low enough to be applied
> > >    online), and
> > >  - scalable (the upper-bound of the overhead is in constant range
> > >    regardless of the size of target workloads).
> > >
> > > Using this framework, therefore, the kernel's memory management
> > > mechanisms can make advanced decisions.  Experimental memory management
> > > optimization works that incurring high data accesses monitoring overhead
> > > could implemented again.  In user space, meanwhile, users who have some
> > > special workloads can write personalized applications for better
> > > understanding and optimizations of their workloads and systems.
> > >
> > > This commit is implementing only the stub for the module load/unload,
> > > basic data structures, and simple manipulation functions of the
> > > structures to keep the size of commit small.  The core mechanisms of
> > > DAMON will be implemented one by one by following commits.
> > >
> > > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> > > Reviewed-by: Leonard Foerster <foersleo@amazon.de>
> > > Reviewed-by: Varad Gautam <vrd@amazon.de>
> > > ---
> > >  include/linux/damon.h |  63 ++++++++++++++
> > >  mm/Kconfig            |  12 +++
> > >  mm/Makefile           |   1 +
> > >  mm/damon.c            | 188 ++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 264 insertions(+)
> > >  create mode 100644 include/linux/damon.h
> > >  create mode 100644 mm/damon.c
> > >
> > > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > > new file mode 100644
> > > index 000000000000..c8f8c1c41a45
> > > --- /dev/null
> > > +++ b/include/linux/damon.h
> > > @@ -0,0 +1,63 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * DAMON api
> > > + *
> > > + * Copyright 2019-2020 Amazon.com, Inc. or its affiliates.
> > > + *
> > > + * Author: SeongJae Park <sjpark@amazon.de>
> > > + */
> > > +
> [...]
> > > +
> > > +/**
> > > + * struct damon_task - Represents a monitoring target task.
> > > + * @pid:               Process id of the task.
> > > + * @regions_list:      Head of the monitoring target regions of this task.
> > > + * @list:              List head for siblings.
> > > + *
> > > + * If the monitoring target address space is task independent (e.g., physical
> > > + * memory address space monitoring), @pid should be '-1'.
> > > + */
> > > +struct damon_task {
> > > +       int pid;
> >
> > Storing and accessing pid like this is racy. Why not save the "struct
> > pid" after getting the reference? I am still going over the usage,
> > maybe storing mm_struct would be an even better choice.
> >
> > > +       struct list_head regions_list;
> > > +       struct list_head list;
> > > +};
> > > +
> [...]
> > > +
> > > +#define damon_get_task_struct(t) \
> > > +       (get_pid_task(find_vpid(t->pid), PIDTYPE_PID))
> >
> > You need at least rcu lock around find_vpid(). Also you need to be
> > careful about the context. If you accept my previous suggestion then
> > you just need to do this in the process context which is registering
> > the pid (no need to worry about the pid namespace).
> >
> > I am wondering if there should be an interface to register processes
> > with DAMON using pidfd instead of integer pid.
>
> Good points!  I will use pidfd for this purpose, instead.
>
> BTW, 'struct damon_task' was introduced while DAMON supports only virtual
> address spaces and recently extended to support physical memory address
> monitoring case by defining an exceptional pid (-1) for such case.  I think it
> doesn't smoothly fit with the design.
>
> Therefore, I would like to change it with more general named struct, e.g.,
>
>     struct damon_target {
>             void *id;
>             struct list_head regions_list;
>             struct list_head list;
>     };
>
> The 'id' field will be able to store or point pid_t, struct mm_struct, struct
> pid, or anything relevant, depending on the target address space.
>
> Only one part of the address space independent logics of DAMON, namely
> 'kdamon_need_stop()', uses '->pid' of the 'struct damon_task'.  It will be
> introduced by the next patch ("mm/damon: Implement region based sampling").
> Therefore, the conversion will be easy.  For the part, I could add another
> callback, e.g.,
>
>     struct damon_ctx {
>             [...]
>             bool (*is_target_valid)(struct damon_target *t);
>     };
>
> And let the address space specific primitives to implement this.
>
> Then, damon_get_task_struct() and damon_get_mm() will be introduced by the
> sixth patch ("mm/damon: Implement callbacks for the virtual memory address
> spaces") as a part of the virtual address space specific primitives
> implementation.
>
> I gonna make the change in the next spin.  If you have some opinions on this,
> please let me know.
>
>

Sorry for the late response. I think the general direction you are
taking is fine but there are still some open questions. I am trying to
reason if 'address space' is general enough abstraction for different
types of monitoring targets. It fits well for the 'processes' targets.
For the physical memory, the monitoring part of the abstraction (i.e.
damon_ctx) seems fine but I am not sure about the optimization part
(i.e. [merge|split]_regions) which raises the question that should the
merge/split functionality be part of the abstraction.

I am also very interested in the 'cgroups' as the target and I am not
sure if 'address space' is the right abstraction for the cgroups as
well. Well we can think of cgroups as a combination of tasks but
cgroup also contains unmapped pages. So, maybe it is a combination of
virtual and physical address space targets damon can monitor but I am
still not clear how to specify that in the abstractions provided by
damon. Anyways these are the questions for later and we can start
simple with just processes but I would like to not expose these
abstractions/interfaces to userspace otherwise it would be really hard
to change later.

Another topic I want to discuss is managing/charging the resource
(cpu) usage of monitoring. Yes, damon with optimization has low cpu
cost but as the number of targets increase the cpu cost will increase
which will be in a range which can not be ignored as system overhead.
At the moment, it seems like there is one kthread doing all the
monitoring, since we can control the cpu usage of kthreads, it might
make sense to allow different kthreads for different sets of targets
(processes in a cgroup).


  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-13  8:41 [PATCH v18 00/14] " SeongJae Park
2020-07-13  8:41 ` [PATCH v18 01/14] mm/page_ext: Export lookup_page_ext() to GPL modules SeongJae Park
2020-07-13 12:08   ` Mike Rapoport
2020-07-13 12:21     ` SeongJae Park
2020-07-13 17:19       ` Mike Rapoport
2020-07-13 17:38         ` SeongJae Park
2020-07-17  9:59           ` SeongJae Park
2020-07-13  8:41 ` [PATCH v18 02/14] mm: Introduce Data Access MONitor (DAMON) SeongJae Park
2020-07-18  2:47   ` Shakeel Butt
2020-07-18 13:31     ` SeongJae Park
2020-07-29 15:31       ` Shakeel Butt [this message]
2020-07-29 17:29         ` SeongJae Park
2020-07-13  8:41 ` [PATCH v18 03/14] mm/damon: Implement region based sampling SeongJae Park
2020-07-13  8:41 ` [PATCH v18 04/14] mm/damon: Adaptively adjust regions SeongJae Park
2020-07-13  8:41 ` [PATCH v18 05/14] mm/damon: Track dynamic monitoring target regions update SeongJae Park
2020-07-13  8:41 ` [PATCH v18 06/14] mm/damon: Implement callbacks for the virtual memory address spaces SeongJae Park
2020-07-17  0:46   ` Shakeel Butt
2020-07-17  6:53     ` SeongJae Park
2020-07-17 15:17       ` Shakeel Butt
2020-07-17 16:24         ` SeongJae Park
2020-07-18  2:23           ` Shakeel Butt
2020-07-18  2:51             ` SeongJae Park
2020-07-27  7:34   ` Greg Thelen
2020-07-27  9:02     ` SeongJae Park
2020-07-28 17:42       ` Shakeel Butt
2020-07-29  6:20         ` SeongJae Park
2020-07-13  8:41 ` [PATCH v18 07/14] mm/damon: Implement access pattern recording SeongJae Park
2020-07-13  8:41 ` [PATCH v18 08/14] mm/damon: Add a tracepoint SeongJae Park
2020-07-13  8:41 ` [PATCH v18 09/14] mm/damon: Implement a debugfs interface SeongJae Park
2020-07-22 10:36   ` SeongJae Park
2020-07-13  8:41 ` [PATCH v18 10/14] tools: Introduce a minimal user-space tool for DAMON SeongJae Park
2020-07-13  8:41 ` [PATCH v18 11/14] Documentation: Add documents " SeongJae Park
2020-07-27  7:19   ` Greg Thelen
2020-07-27  7:38     ` SeongJae Park
2020-07-13  8:41 ` [PATCH v18 12/14] mm/damon: Add kunit tests SeongJae Park
2020-07-13  8:41 ` [PATCH v18 13/14] mm/damon: Add user space selftests SeongJae Park
2020-07-13  8:41 ` [PATCH v18 14/14] MAINTAINERS: Update for DAMON SeongJae Park

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CALvZod6cd4698njE+_dQ6UiL=uRL9xm=aKVm5=-sV28T9Y+fVw@mail.gmail.com' \
    --to=shakeelb@google.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=aarcange@redhat.com \
    --cc=acme@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=amit@kernel.org \
    --cc=benh@kernel.crashing.org \
    --cc=brendan.d.gregg@gmail.com \
    --cc=brendanhiggins@google.com \
    --cc=cai@lca.pw \
    --cc=colin.king@canonical.com \
    --cc=corbet@lwn.net \
    --cc=david@redhat.com \
    --cc=dwmw@amazon.com \
    --cc=foersleo@amazon.de \
    --cc=irogers@google.com \
    --cc=jolsa@redhat.com \
    --cc=kirill@shutemov.name \
    --cc=linux-damon@amazon.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mark.rutland@arm.com \
    --cc=mgorman@suse.de \
    --cc=minchan@kernel.org \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rdunlap@infradead.org \
    --cc=riel@surriel.com \
    --cc=rientjes@google.com \
    --cc=rostedt@goodmis.org \
    --cc=rppt@kernel.org \
    --cc=sblbir@amazon.com \
    --cc=shuah@kernel.org \
    --cc=sj38.park@gmail.com \
    --cc=sjpark@amazon.com \
    --cc=sjpark@amazon.de \
    --cc=snu@amazon.de \
    --cc=vbabka@suse.cz \
    --cc=vdavydov.dev@gmail.com \
    --cc=yang.shi@linux.alibaba.com \
    --cc=ying.huang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git