[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <20240517225921.128477-1-sj@kernel.org>
Date: Fri, 17 May 2024 15:59:21 -0700
From: SeongJae Park <sj@...nel.org>
To: Alex Rusuf <yorha.op@...il.com>
Cc: SeongJae Park <sj@...nel.org>,
damon@...ts.linux.dev,
linux-kernel@...r.kernel.org,
linux-mm@...ck.org
Subject: Re: [RFC PATCH v1 0/7] DAMON multiple contexts support
On Fri, 17 May 2024 11:51:51 +0300 Alex Rusuf <yorha.op@...il.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@...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.
[...]
> > > [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
[...]
Powered by blists - more mailing lists