Received-SPF: pass (google.com: domain of linux-kernel+bounces-198327-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
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: [PATCH v2 0/2] DAMON multiple contexts support
Date: Sun,  2 Jun 2024 18:31:02 +0300
Message-ID: <20240602153102.926789-1-yorha.op@gmail.com>
In-Reply-To: <20240531193307.71424-1-sj@kernel.org>
References: <20240531193307.71424-1-sj@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit

Hi SJ,

> Hello Alex,
> 
> On Fri, 31 May 2024 15:23:18 +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.
> > 
> > This patch-set implements support for multiple contexts
> > per kdamond.
> >
> [...]
> > 
> > ---
> > Changes from v1 (https://lore.kernel.org/damon/20240515152457.603724-1-yorha.op@gmail.com/)
> > - Compatibility for DebugFS interface is kept
> > - Kunit tests build/execution issues are fixed
> > - Patches from v1 are sqaushed, so that consistency between patches is
> > kept
> 
> My request was to avoid unnecessary temporal changes that will be removed in
> next patches.  Some of those are well removed in this version, but I still show
> some.  E.g., nr_contexts field.  Also, this resulted in two big patches.

This makes sense and I actually wanted that as well, so I tried to separate
them in previous version, looks like I misunderstood your request.

Anyway, don't you mind if lru_sort/traceevents/etc. will not function
correctly without applying the whole patch-set? I mean if we use the
approach below, once core.c is modified at least lru_sort and reclaim
will not work correctly, they even will not be built.

> 
> I'd also appreciate if you can separate changes into smaller ones of logical
> single change.  For example, changes for lru_sort.c, reclaim.c, and sysfs.c on
> first patch could be much smaller in my opinion.  Traceevent change can also be
> separated from patch 2.  Some of multi-context support seems mixed in patch 1.
> 
> I'd suggest below patches flow.
> 
> Patch 1: Introduce new struct and control functions for the struct.  Don't
> really use the struct and the functions.
> 
> Patch 2: Modify core.c to use the struct and implement multiple contexts
> support.  Minimize changes to core.c users.  Just keep those work as before.
> Don't implement multi contexts support on sysfs.c or trace events at this
> point.
> 
> Patch 3: Update sysfs.c to support the multiple contexts.
> 
> Patch 4: Update trace events to better support it.
> 
> > - Added/Fixed comments about data structures/functions
> 
> Also, you don't need to put version history under '---' marker if it is a cover
> letter.  You can put it on the body.
> 
> > 
> > Alex Rusuf (2):
> >   mm/damon/core: add 'struct kdamond' abstraction layer
> >   mm/damon/core: implement multi-context support
> 
> I will try to put more detailed comments on each patch.
> 
> > 
> >  include/linux/damon.h        |  80 ++++--
> >  include/trace/events/damon.h |  14 +-
> >  mm/damon/core-test.h         |   2 +-
> >  mm/damon/core.c              | 509 ++++++++++++++++++++++-------------
> >  mm/damon/dbgfs-test.h        |   4 +-
> >  mm/damon/dbgfs.c             | 342 ++++++++++++++---------
> >  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             | 303 +++++++++++++--------
> >  11 files changed, 872 insertions(+), 481 deletions(-)
> > 
> > -- 
> > 2.42.0
> 
> 
> Thanks,
> SJ

BR,
Alex