Received-SPF: pass (google.com: domain of linux-kernel+bounces-198370-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
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: [PATCH v2 1/2] mm/damon/core: add 'struct kdamond' abstraction layer
Date: Sun,  2 Jun 2024 09:51:48 -0700
Message-Id: <20240602165148.92368-1-sj@kernel.org>
In-Reply-To: <20240602161557.927788-1-yorha.op@gmail.com>
References: 
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

On Sun,  2 Jun 2024 19:15:57 +0300 Alex Rusuf <yorha.op@gmail.com> wrote:

> Hi SJ,
> 
> > Hi Alex,
> > 
> > On Fri, 31 May 2024 15:23:19 +0300 Alex Rusuf <yorha.op@gmail.com> wrote:
> > 
> > > In current implementation kdamond tracks only 1
> > > context, that is kdamond _is_ damon_ctx what makes
> > > it very difficult to implement multiple contexts.
> > > 
> > > This patch adds another level of abstraction, that is
> > > 'struct kdamond' - structure which represents kdamond itself.
> > > It holds references to all contexts organized in list.
> > > 
> > > Few fields like ->kdamond_started and ->kdamond_lock
> > > (just ->lock for 'struct kdamond') also has been moved
> > > to 'struct kdamond', because they have nothing to do
> > > with the context itself, but with the whole kdamond
> > > daemon.
> > > 
> > > Signed-off-by: Alex Rusuf <yorha.op@gmail.com>
> > > ---
> > >  include/linux/damon.h     |  73 ++++++++---
> > >  mm/damon/core.c           | 249 ++++++++++++++++++++++-------------
> > >  mm/damon/lru_sort.c       |  31 +++--
> > >  mm/damon/modules-common.c |  36 +++--
> > >  mm/damon/modules-common.h |   3 +-
> > >  mm/damon/reclaim.c        |  30 +++--
> > >  mm/damon/sysfs.c          | 268 ++++++++++++++++++++++++--------------
> > >  7 files changed, 463 insertions(+), 227 deletions(-)
> > >
[...]
> > > +/**
> > > + * damon_kdamond_running() - Return true if kdamond is running
> > > + * false otherwise.
> > > + */
> > > +bool damon_kdamond_running(struct kdamond *kdamond)
> > > +{
> > > +	bool running;
> > > +
> > > +	mutex_lock(&kdamond->lock);
> > > +	running = kdamond->self != NULL;
> > > +	mutex_unlock(&kdamond->lock);
> > > +	return running;
> > >  }
> > 
> > Seems this function is used by only DAMON sysfs interface.  Don't implement
> > unnecessary public function.  Implement it in mm/damon/sysfs.c please.
> 
> DebugFS interface also uses this.

I think dbgfs.c is using damon_nr_running_ctxs() now?  De-duplicating code is
nice, but I think that can be done in a separate patch.  Moreover, DAMON
debugfs interface is deprecated now.  I'd like to keep changes to the file as
minimum as possible.

[...]
> > > +static inline struct damon_ctx *damon_lru_sort_ctx(void)
> > > +{
> > > +	return damon_first_ctx(kdamond);
> > > +}
> > > +
> > > +static inline struct damon_target *damon_lru_sort_target(void)
> > > +{
> > > +	return damon_first_target(
> > > +			damon_lru_sort_ctx());
> > > +}
> > 
> > Do we really need above two functions?  Can't we simply add 'kdamond' global
> > variable but still uses 'ctx' and 'target' as before, except interface changed
> > function calls?
> 
> We can, I just tried to use 'readable' interface, I thought
> that using raw pointers from kdamond will be less readable.

I agree this will eventually be better code.  But let's do such things in
separte patches.  Keeping individual patch simple and small would help poor
reviewers :)

[...]
> > > @@ -1065,15 +1055,15 @@ static struct damon_sysfs_cmd_request damon_sysfs_cmd_request;
> > >  static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
> > >  		char *buf)
> > >  {
> > > -	struct damon_sysfs_kdamond *kdamond = container_of(kobj,
> > > +	struct damon_sysfs_kdamond *sys_kdamond = container_of(kobj,
> > >  			struct damon_sysfs_kdamond, kobj);
> > 
> > This makes reviewing bit difficult.  Let's keep current name for now.
> > 
> > > -	struct damon_ctx *ctx = kdamond->damon_ctx;
> > > +	struct kdamond *kdamond = sys_kdamond->kdamond;
> > 
> > Keep 'kdamond' variable name as is, and use shorter name for this variable,
> > say, kd, or do kdamond->kdamond.
> 
> It usually makes reading code difficult, these are two different abstraction of
> kdamond, but the name will almost be the same, I don't think this is a good idea.
> 
> > 
> > >  	bool running;
> > >  
> > > -	if (!ctx)
> > > +	if (!kdamond)
> > 
> > "if (!kd)" or "if (!kdamond->kdamond)"
> 
> And this is an example, I can't say which one belong to sysfs's abstraction
> and which represents core's kdamond. With "sys_" prefix or "sysfs_" sometimes,
> it is much easier to read.

I agree that.  But from my perspective, these name changes are not essential
for the purpose of this patch but only make review time consuming.  Do one
thing with minimum change, please.  You could do the code cleanup in other
patches.

[...]
> > 
> > >From this point, I think below changes could be significantly changed in next
> > version of this patchset if you agree to above comments.  So I'm stop reviewing
> > this patch from here for now.  Please let me know if you have different opinion
> > about it.
> 
> Thank you very much for your comments, I really appreciate that you spent time to
> review this. I'll try to improve the patch-set in next versions!

No problem, and looking forward to the next version!


Thanks,
SJ

[...]