From: SeongJae Park <sj@kernel.org>
To: Alex Rusuf <yorha.op@gmail.com>
Cc: SeongJae Park <sj@kernel.org>,
damon@lists.linux.dev, linux-mm@kvack.org,
linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v1 0/7] DAMON multiple contexts support
Date: Thu, 16 May 2024 15:17:35 -0700 [thread overview]
Message-ID: <20240516221735.82564-1-sj@kernel.org> (raw)
In-Reply-To: <20240515152457.603724-1-yorha.op@gmail.com>
Hello Alex,
Adding high level comments first. I will try to read each patch and add
detailed comments to those as soon as I get some time.
Also, please Cc linux-mm@ for DAMON patches. I'd also recommend cc-ing
linux-kernel@.
On Wed, 15 May 2024 18:24:50 +0300 Alex Rusuf <yorha.op@gmail.com> wrote:
> Currently kdamond uses only one context per kthread
> and most of its time it sleeps, so utilizing several
> contexts can scale kdamond and allow it to use
> another set of operations.
Thank you for this patchset. I believe this change is important for DAMON's
long term vision.
A quick question for a clarification and proper prioritization, though, since
size of this patch series is not very tiny. Does this patch series is for your
real usage? If so, could you please clarify your usage and how this patch
series can help?
>
> This patch-set implements support for multiple contexts
> per kdamond.
>
> In pseudo code previous versions worked like
> the following:
>
> while (!kdamond_should_stop()) {
>
> /* prepare accesses for only 1 context */
> prepare_accesses(damon_context);
>
> sleep(sample_interval);
>
> /* check accesses for only 1 context */
> check_accesses(damon_context);
>
> ...
> }
>
> With this patch kdamond workflow will look
> like the following:
>
> while (!kdamond_shoule_stop()) {
>
> /* prepare accesses for all contexts in kdamond */
> damon_for_each_context(ctx, kdamond)
> prepare_accesses(ctx);
>
> sleep(sample_interval);
>
> /* check_accesses for all contexts in kdamond */
> damon_for_each_context(ctx, kdamond)
> check_accesses(ctx);
>
> ...
> }
The overall idea makes sense to me.
>
> To try this you can use modified kernel[1] and
> damo[2]. I also have written few simple shell scripts[3]
> to collect data for damo.
>
> [1] https://github.com/onlyoneofme/damon-multi-contexts.git
> [2] https://github.com/onlyoneofme/damo/tree/multi-contexts
Looking forward to the patch for DAMO be submitted, or PR-ed!
> [3] https://github.com/onlyoneofme/damon-multi-contexts-tests.git
Do you have a plan to integrate this into DAMON selftests or damon-tests?
>
> Alex Rusuf (7):
> mm/damon/core: kdamond_struct abstraction layer
Let's make the subjects clear what it does. For example, this patch's subject
could be "add kdamonds_struct abstraction layer". Similar comment for other
patches. Also, I think '_struct' suffix of 'kdamond_struct' is not really
needed. Let's remove it if there is no special reason to add it.
> mm/damon/core: list-based contexts organization
I think this can be squashed into the first patch? If not, could you please
let clarify?
> mm/damon/lru_sort: kdamond_struct abstraction layer
> mm/damon/reclaim: kdamon_struct abstraction layer
Does these two patches mean lru_sort and reclaim are broken by the first patch?
Let's keep everything unbroken in middle of the patchset, to help bisect.
> mm/damon/core: rename nr_running_ctxs -> nr_running_kdamonds
I think this would also better to be together with the first patch? I know
this does not break something, but makes reading patch bit complex.
> mm/damon/core: multi-context support
> mm/damon/core: multi-context awarness for trace events
I think these two patches should be squashed into one patch. Otherwise, the
trace point is broken in the middle of the patch series, right?
>
> include/linux/damon.h | 48 +++-
> include/trace/events/damon.h | 14 +-
> mm/damon/core.c | 497 +++++++++++++++++++++--------------
> mm/damon/lru_sort.c | 31 ++-
> mm/damon/modules-common.c | 35 ++-
> mm/damon/modules-common.h | 3 +-
> mm/damon/reclaim.c | 30 ++-
> mm/damon/sysfs.c | 306 +++++++++++++--------
> 8 files changed, 629 insertions(+), 335 deletions(-)
>
> --
> 2.42.0
Thanks,
SJ
next prev parent reply other threads:[~2024-05-16 22:17 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-15 15:24 [RFC PATCH v1 0/7] DAMON multiple contexts support Alex Rusuf
2024-05-15 15:24 ` [RFC PATCH v1 1/7] mm/damon/core: kdamond_struct abstraction layer Alex Rusuf
2024-05-15 15:24 ` [RFC PATCH v1 2/7] mm/damon/core: list-based contexts organization Alex Rusuf
2024-05-15 15:24 ` [RFC PATCH v1 3/7] mm/damon/lru_sort: kdamond_struct abstraction layer Alex Rusuf
2024-05-15 15:24 ` [RFC PATCH v1 4/7] mm/damon/reclaim: kdamon_struct " Alex Rusuf
2024-05-15 15:24 ` [RFC PATCH v1 5/7] mm/damon/core: rename nr_running_ctxs -> nr_running_kdamonds Alex Rusuf
2024-05-15 15:24 ` [RFC PATCH v1 6/7] mm/damon/core: multi-context support Alex Rusuf
2024-05-15 15:24 ` [RFC PATCH v1 7/7] mm/damon/core: multi-context awarness for trace events Alex Rusuf
2024-05-16 22:17 ` SeongJae Park [this message]
2024-05-17 8:51 ` [RFC PATCH v1 0/7] DAMON multiple contexts support Alex Rusuf
2024-05-17 22:59 ` 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=20240516221735.82564-1-sj@kernel.org \
--to=sj@kernel.org \
--cc=damon@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=yorha.op@gmail.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).