Received-SPF: pass (google.com: domain of linux-kernel+bounces-39736-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 <sj38.park@gmail.com>
To: cuiyangpei <cuiyangpei@gmail.com>
Cc: SeongJae Park <sj@kernel.org>,
	akpm@linux-foundation.org,
	damon@lists.linux.dev,
	linux-mm@kvack.org,
	linux-kernel@vger.kernel.org,
	xiongping1@xiaom.com
Subject: Re: [PATCH 1/2] mm/damon/sysfs: Implement recording feature
Date: Fri, 26 Jan 2024 00:04:54 -0800
Message-Id: <20240126080454.15649-1-sj@kernel.org>
In-Reply-To: <20240126065706.GA16652@cuiyangpei>
Precedence: bulk

On Fri, 26 Jan 2024 14:57:06 +0800 cuiyangpei <cuiyangpei@gmail.com> wrote:

> On Mon, Jan 22, 2024 at 09:56:11AM -0800, SeongJae Park wrote:
> > Hi cuiyangpei,
> > 
> > On Mon, 22 Jan 2024 13:46:31 +0800 cuiyangpei <cuiyangpei@gmail.com> wrote:
> > 
> > > On Sun, Dec 03, 2023 at 07:37:45PM +0000, SeongJae Park wrote:
> > > > On 2023-12-03T13:43:13+08:00 cuiyangpei <cuiyangpei@gmail.com> wrote:
> > > > 
> > > > > On Fri, Dec 01, 2023 at 05:31:12PM +0000, SeongJae Park wrote:
> > > > > > Hi Cuiyangpei,
> > > > > > 
> > > > > > On Fri, 1 Dec 2023 20:25:07 +0800 cuiyangpei <cuiyangpei@gmail.com> wrote:
> > > > > > 
> > > > > > > On Thu, Nov 30, 2023 at 07:44:20PM +0000, SeongJae Park wrote:
> > > > > > > > Hi Cuiyangpei,
> > > > > > > > 
> > > > > > > > On Thu, 30 Nov 2023 17:14:26 +0800 cuiyangpei <cuiyangpei@gmail.com> wrote:
[...]
> > > Is there any way to catch sampling result immediately after setting the
> > > "update_schemes_tried_regions" state?
> > 
> > There is no way for exactly doing this.  You would need to proactively collect
> > snapshots while the app is foreground, and use the latest one that collected
> > before the app goes background, like recording-based approach would do.
> > 
> > I think recent DAMON changes might make an alternative approach available,
> > though.  From v6.7, DAMON provides pseudo-moving-average monitoring result in
> > sampling interval granualrity, since patchset "mm/damon: provide pseudo-moving
> > sum based access rate".  And a followup patchset, namely "mm/damon: implement
> > DAMOS apply intervals", has made DAMOS works in the sampling interval
> > granualrity.  Both patchsets are merged into v6.7-rc1.
> > 
> > Hence, I think you could use 'update_schemes_tried_regions' after you noticed
> > the app's state transition, with DAMOS apply interval of one sampling interval.
> > Then you will get the monitoring results after one sampling interval.  Of
> > course, the snapshot may contain some of background access pattern, but
> > wouldn't made it changed significantly, unless you set aggregation interval too
> > short.
> 
> All other actions will apply at one sampling interval except for the
> `stat` action.
> 
> We use 'update_schemes_tried_regions' after switch to the background. The 
> before_damos_apply callback function will only be set when the next aggregation
> interval arrives. The `tried_regions` will only be updated after setting the 
> callback function. 
> 
> DAMON is still sampling during setting 'update_schemes_tried_regions' to the next
> aggregation time, which is not what we expected. The pseudo-moving-average
> monitoring result can reduce nr_accesees inaccuracy, but age is still being modified
> during this time, so it can't improve this issue.
> 
> Please let me know if my understanding is incorrect. Thank you.

So, 'update_schemes_tried_regions' command is firstly handled by
'damon_sysfs_cmd_request_callback()', which is registered as
after_wmarks_check() and after_aggregation() callback.  Hence
'update_schemes_tried_regions' command is still effectively working in
aggregation interval granularity.  I think this is what you found, right?

If I'm not wrongly understanding your point, I think the concern is valid.  I
think we should make it works in sampling interval granularity.  I will try to
make so.  Would that work for your use case?

> > 
> > > Alternatively, can it return the "last_nr_accesses" and "last_age" values in
> > > tried_regions/<N> directory?
> > 
> > This could also be a good alternative in my think.  Nice idea.  But, because
> > the previously mentioned alternative is already available while this require a
> > bit small but additional changes, could we check if the previously one make
> > sense and works first?  We could revisit this idea if it turns out the previous
> > alternative is not suffice in my opinion.
> > 
> Can you consider adding "last_nr_accesses" and "last_age"  two files in
> 'tried_regions/<N>' directory?

Actually we don't have 'last_age' field, right?  And in case of
'last_nr_accesses', it is a hidden private field, since it is intended to be
accessed by only DAMON core code.  Making it exposed to user means exposing
implementation details, and the mechanism that coupled with an exposed
interface is hard to be changed, so be unflexible.  Hence I'd prefer making
'update_schemes_tried_regions' works in sampling interval granularity, more
than exposing the two information if it works for your use case.


Thanks,
SJ

[...]