Received-SPF: pass (google.com: domain of linux-kernel+bounces-182680-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: SeongJae Park <sj@kernel.org>
To: Alex Rusuf <yorha.op@gmail.com>
Cc: SeongJae Park <sj@kernel.org>,
	damon@lists.linux.dev,
	linux-kernel@vger.kernel.org,
	linux-mm@kvack.org
Subject: Re: [RFC PATCH v1 0/7] DAMON multiple contexts support
Date: Fri, 17 May 2024 15:59:21 -0700
Message-Id: <20240517225921.128477-1-sj@kernel.org>
In-Reply-To: <20240517085151.630844-1-yorha.op@gmail.com>
References: 
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

On Fri, 17 May 2024 11:51:51 +0300 Alex Rusuf <yorha.op@gmail.com> wrote:

> 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.
[...]
> > > 	[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?

I think we should keep debugfs work as was.  So I think we should ensure it's
not broken, but don't need to expand it to support this new feature.

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

Cool, I'm looking forward to the next version of this great patchset!


Thanks,
SJ

[...]