linux-kernel - Re: [PATCH v2 0/2] DAMON multiple contexts support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240602153102.926789-1-yorha.op@gmail.com>
Date: Sun,  2 Jun 2024 18:31:02 +0300
From: Alex Rusuf <yorha.op@...il.com>
To: sj@...nel.org
Cc: damon@...ts.linux.dev,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org,
	yorha.op@...il.com
Subject: Re: [PATCH v2 0/2] DAMON multiple contexts support

Hi SJ,

> Hello Alex,
> 
> On Fri, 31 May 2024 15:23:18 +0300 Alex Rusuf <yorha.op@...il.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