Re: [PATCH] mm/damon: update expired description of damos_action

[Sang-Heon Jeon <ekffu200098@gmail.com>]

On Mon, Aug 4, 2025 at 2:42 AM SeongJae Park <sj@kernel.org> wrote:
>
> On Sun, 3 Aug 2025 22:22:05 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
>
> > Hi, SeongJae and Honggyu,
> >
> > On Sun, Aug 3, 2025 at 2:41 PM Honggyu Kim <honggyu.kim@sk.com> wrote:
> > >
> > >
> > >
> > > On 8/3/2025 2:30 PM, SeongJae Park wrote:
> > > > On Sun, 3 Aug 2025 13:43:03 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> > > >
> > > >> Hi SeongJae,
> > > >>
> > > >> On 8/3/2025 1:22 PM, SeongJae Park wrote:
> > > >>> On Sun, 3 Aug 2025 11:03:12 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> > > >>>
> > > >>>> Hi SeongJae and Sang-Heon,
> > > >>>>
> > > >>>> On 8/2/2025 1:50 AM, SeongJae Park wrote:
> > > >>>>> On Sat, 2 Aug 2025 01:11:09 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > > >>>>>
> > > >>>>>> Hi, Honggyu
> > > >>>>>>
> > > >>>>>> On Fri, Aug 1, 2025 at 8:35 PM Honggyu Kim <honggyu.kim@sk.com> wrote:
> > > >>>>>>>
> > > >>>>>>> Hi Sang-Heon and SeongJae,
> > > >>>>>>>
> > > >>>>>>> On 8/1/2025 2:58 AM, SeongJae Park wrote:
> > > >>>>>>>> Hello Sang-Heon,
> > > >>>>>>>>
> > > >>>>>>>> On Thu, 31 Jul 2025 22:22:30 +0900 Sang-Heon Jeon <ekffu200098@gmail.com> wrote:
> > > >>>>>>>>
> > > >>>>>>>>> Nowadays, damos operation actions support more various operation set.
> > > >>>>>>>>> But comments(also, generated documentation) doesn't updated.
> > > >>>>>>>>> So, fix the comments with current support status.
> > > >>>>> [...]
> > > >>>>>>>>> diff --git a/include/linux/damon.h b/include/linux/damon.h
> > > >>>>> [...]
> > > >>>>>>>>>       * @DAMOS_WILLNEED:  Call ``madvise()`` for the region with MADV_WILLNEED.
> > > >>>>>>>>>       * @DAMOS_COLD:              Call ``madvise()`` for the region with MADV_COLD.
> > > >>>>>>>>> - * @DAMOS_PAGEOUT:   Call ``madvise()`` for the region with MADV_PAGEOUT.
> > > >>>>>>>>> + * @DAMOS_PAGEOUT:   Reclaim the region.
> > > >>>>>>>>
> > > >>>>>>>> Nice!
> > > >>>>>>>
> > > >>>>>>> But doesn't it make confusion about whether this pages out to disk or does
> > > >>>>>>> demotion to the lower tier memory?  It's because PAGEOUT action doesn't do
> > > >>>>>>> demotion, but it looks "reclaim" includes pageout and demotion together in my
> > > >>>>>>> understanding since /sys/kernel/mm/numa/demotion_enabled was introduced.
> > > >>>>>
> > > >>>>> To my understanding, DAMOS_PAGEOUT can also do demotion when demotion_enabled
> > > >>>>> is set.  Am I missing something?
> > > >>>>
> > > >>>> Actually no, please see below.
> > > >>>
> > > >>> I'm unsure to what point you are saying "no".  Are you saying DAMOS_PAGEOUT can
> > > >>> also do demotion when demotion_enabled is set?  Or not?  Could you please
> > > >>> clarify, and add more explanations about why you think so?
> > > >>
> > > >> I checked it again and found I pointed out in the wrong place. Please see below.
> > > >>
> > > >>>
> > > >>>>
> > > >>>> do_demote_pass in shrink_folio_list()
> > > >>>> https://github.com/torvalds/linux/blob/v6.16/mm/vmscan.c#L1122
> > > >>>>
> > > >>>> The do_demote_pass is used here.
> > > >>>> https://github.com/torvalds/linux/blob/v6.16/mm/vmscan.c#L1293-L1302
> > > >>>>
> > > >>>> can_demote() implementation returns false when demotion_enabled is on.
> > > >>>> https://github.com/torvalds/linux/blob/v6.16/mm/vmscan.c#L350-L351
> > > >>>
> > > >>> I'm again get confused.  Isn't it opposite?
> > > >>
> > > >> The thing is that DAMOS_PAGEOUT call sequence is as follows.
> > > >>
> > > >>     DAMOS_PAGEOUT
> > > >>     -> damon_pa_pageout
> > > >>     -> reclaim_pages
> > > >>     -> reclaim_folio_list
> > > >>
> > > >> In reclaim_folio_list(), it sets "no_demotion = 1" in scan_control, then invokes
> > > >> shrink_folio_list().
> > > >
> > > > Thank you, this clarifies.  DAMOS_PAGEOUT doesn't demote pages even if
> > > > demotion_enabled is set.  Thank you for enlightening me.
> > >
> > > Thank you too. Glad to hear that.
> > >
> > > >
> > > > So, "reclaim" means "reclaim".  shrink_folio_list() can do demotions when
> > > > demotion_enabled is set.  I hence still don't think this patch is saying
> > > > something very wrong, and how it could be improved.  Do you have more specific
> > > > change suggestions for this patch for an improvment?
> > >
> > > I would just like to make the term "reclaim" clearer and we may be able to
> > > define what "reclaim" is.  I think we can choose between the following two
> > > different definitions.
> > >
> > > Definition 1. "reclaim" includes "pageout" and "demotion".
> > > In this case, we better clarify all the other documents that mentions about
> > > those terms.
> > >
> > > Definition 2. "reclaim" only includes "pageout", but "demotion" is out of scope.
> > > In this case, shrink_folio_list just do pageout, but "demotion" is only
> > > exceptional case so we can say the "demotion" escapes from "reclaim" logic.
>
> I was thinking in the first way, and apparently I was simply wrong.  Now I
> think the second one is more correct, at least for {DAMOS,MADV}_PAGEOUT.
>
> Yet another way to do the reclaim would be memory.reclaim cgroup file.  Seems
> in the case, demotion can happen since user_proactive_reclaim() doesn't set
> scan_control->no_demotion.
>
> I didn't read the code, but apparently memory pressure-based reactive
> traditional reclaim logic would also do demotion?
>
> So, apparently "reclaim" can mean at least the two definitions.
>
> > >
> > > We might have to clarify the term "reclaim" for those cases whether it includes
> > > "demotion" or not.  We might have to discuss with other mm developers together.
>
> I think that could be a nice discussion.  Looking forward to.  As I previously
> mentioned, I don't think we need to have only 1:1 mapping terminologies, though
> I have no strong opinion here.
>
> Regardless of terms, I agree there are many rooms to improve on our
> documentation.  At least DAMOS_PAGEOUT documentation may better to clarify what
> you pointed out.
>
> > > Thanks,
> > > Honggyu
> >
> > Because of the above thread, I got to know the details more clearly.
> > Thank you guys!
> > When the discussion of "reclaim" finishes, I'll make v2 patch as soon
> > as possible.
>
> I don't think we need to block v2 of this patch for the discussion.  What about
> adding a note about the demotion behavior on the details comment second, e.g.,

I see. That sounds reasonable because we can revisit after the
discussion finishes.

> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -150,6 +150,8 @@ struct damon_access_report {
>   * &enum DAMOS_LRU_PRIO and &enum DAMOS_LRU_DEPRIO.  &enum DAMON_OPS_PADDR
>   * supports only &enum DAMOS_PAGEOUT, &enum DAMOS_LRU_PRIO, &enum
>   * DAMOS_LRU_DEPRIO, and &DAMOS_STAT.
> + *
> + * Note that DAMOS_PAGEOUT doesn't trigger demotions.
>   */
>  enum damos_action {
>         DAMOS_WILLNEED,
>
> >
> > However, I want to talk about a slightly different topic. How about
> > adding support demotion to DAMOS operation action?
> > Maybe we can add another action type or change implementation of DAMOS_PAGEOUT.
>
> We actually tried to implement DAMOS_DEMOTE, but eventually decided[1] to have
> a more general action called DAMOS_MIGRATE_COLD.
>
> Do you think there are use cases where DAMOS_MIGRATE_COLD cannot cover?

Thanks for your comments. Above suggestion was based on my lack of
knowledge of DAMON.
I tracked linked RFC threads; now I understand that DAMOS_MIGRATE_COLD
supports both demotion or lateral migration, depending on the
environment.

Maybe there could be a new use case, but I think now it's not at the moment.
It's just my bad :( let's just skip that suggestion.

> >
> > IMHO, I think we should first check whether it's possible to set
> > no_demotion in the `madvise` -> `foilo` flow we're using.
> > Since I'm still quite new to these things, I'd like to check whether
> > my idea and direction are correct.
> >
> > I can't thank you all enough for your kindness :)
>
> The pleasure is mine :)
>
> >
> > Best Regards.
> > Sang-Heon Jeon
> >
>
> [1] https://lore.kernel.org/all/20240614030010.751-1-honggyu.kim@sk.com/
>
>
> Thanks,
> SJ

Best Regards.
Sang-Heon Jeon