Received-SPF: pass (google.com: domain of linux-kernel+bounces-181883-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Alex Rusuf <yorha.op@gmail.com>
To: sj@kernel.org
Cc: damon@lists.linux.dev,
	linux-kernel@vger.kernel.org,
	linux-mm@kvack.org,
	yorha.op@gmail.com
Subject: Re: [RFC PATCH v1 0/7] DAMON multiple contexts support
Date: Fri, 17 May 2024 11:51:51 +0300
Message-ID: <20240517085151.630844-1-yorha.op@gmail.com>
In-Reply-To: <20240516221735.82564-1-sj@kernel.org>
References: <20240516221735.82564-1-sj@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit

Hi SJ

> 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@.

Thank you for clarification, I'll add them!

> 
> 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?

No, this is not for some commercial use or so that I could describe. It is
rather just a RFC, because I'm even not sure how much this can help
in real use cases.

> 
> > 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!

Sure, I will submit a PR for DAMO later. I supported only 'record'
and 'report heats' commands, so others haven't touched, that's
why I decided to post it there (because it is not ready).

> 
> > 	[3] https://github.com/onlyoneofme/damon-multi-contexts-tests.git
> 
> Do you have a plan to integrate this into DAMON selftests or damon-tests?

Not in the form they are for time being. These are just simple shell scripts
that set up kdamond to use multiple contexts and these scripts only
collect information like DAMO (but only as perf.data).

Anyway, I think we can integrate them with damon-tests with some modifications
to be able to actually _test_ if multiple contexts work.

As for DAMON selftests, I didn't touch them for time being, so they truly
need to be modified and expanded, but before doing that I would like
you to look at implementation first, because changes in implementation
could affect selftests, so once we agree on that I will implement selftests.

Also note, that I didn't integrate changes with debugfs. I remember this is
deprecated interface, but I'm not sure if compatibility need to be preseved
with it, so do we need to expand debugfs for this?

> 
> > 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.

Sure, I'll change that in next version, thanks!

> 
> >   mm/damon/core: list-based contexts organization
> 
> I think this can be squashed into the first patch?  If not, could you please
> let clarify?

I just tried to separate those patches for them to be as simple as
possible (actually I failed at that...), but sure, we can squash them.

> 
> >   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.

Yes, they're broken by the first patch, I'll squash them, thanks!

> >   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.

No problem, thanks!

> 
> >   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?

You're right, I'll squash them, thanks!

> 
> >  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(-)
> 
> Thanks,
> SJ

BR,
Alex