Re: [RFC PATCH v3 1/4] mm/damon/sysfs: set goal_tuner after scheme creation

[SeongJae Park]

On Wed, 25 Feb 2026 16:53:48 -0800 SeongJae Park <sj@xxxxxxxxxx> wrote:

> On Wed, 25 Feb 2026 10:23:09 -0800 Ravi Jonnalagadda <ravis.opensrc@xxxxxxxxx> wrote:
> 
> > On Mon, Feb 23, 2026 at 5:40 PM SeongJae Park <sj@xxxxxxxxxx> wrote:
> > >
> > > On Mon, 23 Feb 2026 12:32:29 +0000 Ravi Jonnalagadda <ravis.opensrc@xxxxxxxxx> wrote:
> > >
> > > > damon_new_scheme() always sets quota.goal_tuner to CONSIST (the default)
> > > > regardless of what was passed in the quota struct. This caused the sysfs
> > > > goal_tuner setting to be ignored.
> > > >
> > > > The comment in damon_new_scheme() says "quota.goals and .goal_tuner
> > > > should be separately set by caller", but the sysfs code wasn't doing
> > > > this. Add explicit assignment of goal_tuner after damon_new_scheme()
> > > > returns to properly apply the user's setting.
> > > >
> > > > Without this fix, setting goal_tuner to "temporal" via sysfs has no
> > > > effect - the scheme always uses the CONSIST (feed loop) tuner, causing
> > > > overshoot when the goal is reached instead of immediate stop.
> > >
> > > Thank you for catching this, Ravi!  So, this is a fix for the RFC patch series
> > > [1] that not yet merged, right?  I think this fix is better to be carried with
> > > the series, and squashed into the broken commit to not introduce unnecessary
> > > regression.
> > >
> > > So, if you don't mind, I will squash this into the sysfs-schemes part change on
> > > my tree, with your Co-developed-by: tag.
> > 
> > Yes. Please go ahead and squash it into your series.
> > 
> > >
> > > [1] https://lore.kernel.org/20260212062314.69961-1-sj@xxxxxxxxxx
> > > [2] https://lore.kernel.org/20260212062314.69961-4-sj@xxxxxxxxxx
> > >
> > > >
> > > > Signed-off-by: Ravi Jonnalagadda <ravis.opensrc@xxxxxxxxx>
> > >
> > > Reviewed-by: SeongJae Park <sj@xxxxxxxxxx>
> > >
> > > > ---
> > > >  mm/damon/sysfs-schemes.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> > > > index bbea908074bb..fe2e3b2db9e1 100644
> > > > --- a/mm/damon/sysfs-schemes.c
> > > > +++ b/mm/damon/sysfs-schemes.c
> > > > @@ -2809,6 +2809,9 @@ static struct damos *damon_sysfs_mk_scheme(
> > > >       if (!scheme)
> > > >               return NULL;
> > > >
> > > > +     /* Set goal_tuner after damon_new_scheme() as it defaults to CONSIST */
> > > > +     scheme->quota.goal_tuner = sysfs_quotas->goal_tuner;
> > > > +
> > > >       err = damos_sysfs_add_quota_score(sysfs_quotas->goals, &scheme->quota);
> > > >       if (err) {
> > > >               damon_destroy_scheme(scheme);
> > >
> > > To follow the order on the comment ("quota.goals and .goal_tuner should be
> > > separately set by caller"), I'd prefer setting the goal_tuner after
> > > damos_sysfs_add_quota_Score() call here, if you don't mind.  Let me know if you
> > > prefer keeping the current order.  If not, I will just make the change when I
> > > apply this to damon/next.
> > >
> > 
> > Agreed. Please make that change when you apply it.
> 
> Thank you, I will do!

I now think it is better to just remove the damon_new_scheme() internal
initialization.  I added the detail [1] to the original thread.  I'll keep your
Co-developed-by: tag with the new fix though, unless you mind.  Let's keep
discussing there if you have more opinions.

[1] https://lore.kernel.org/20260227015613.91346-1-sj@xxxxxxxxxx


Thanks,
SJ

[...]