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

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

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.
> >
> > Thank you for catching this and fixing!
> >
> >>
> >> Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
> >> ---
> >> I also found below patch(https://lore.kernel.org/damon/20250730200239.60984-1-sj@kernel.org/T/#t)
> >> So, Is it means that DAMOS_STAT is only supported by paddr now?
> >
> > No.  'vaddr' supports DAMOS_STAT.  The patch is not adding DAMOS_STAT support
> > on vaddr, but extending the support for DAMOS filters.
> >
> > Thank you for checking the thread and asking this question, though.  I see this
> > as an important signal of another rooms to improve on the documentation.
> >
> >> If my understanding is correct, then we need to fix Design documentation
> >> and comments both.
> >>
> >> However, since it will be fixed soon. Keeping current status is also
> >> affordable. So could you check this point as well?
> >>
> >> ---
> >>
> >>   include/linux/damon.h | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/damon.h b/include/linux/damon.h
> >> index cddf82eaac4e..0870e9f48b0f 100644
> >> --- a/include/linux/damon.h
> >> +++ b/include/linux/damon.h
> >> @@ -128,7 +128,7 @@ struct damon_access_report {
> >
> > Seems you are using damon/next as your baseline.  If there is no real
> > dependencies to damon/next, please use mm-new as your baseline.  For more
> > contexts about that, please refer to the DAMON maintainer-profile[1].
> >
> >>    *
> >>    * @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.

My intention was just to synchronize with the Design documentation.

So how about changing the description to `Page out the region`, Would
this be also confusing?
I feel like it would be clearer than using word "reclaim"

> Thanks,
> Honggyu
>
> >>    * @DAMOS_HUGEPAGE:  Call ``madvise()`` for the region with MADV_HUGEPAGE.
> >>    * @DAMOS_NOHUGEPAGE:        Call ``madvise()`` for the region with MADV_NOHUGEPAGE.
> >>    * @DAMOS_LRU_PRIO:  Prioritize the region on its LRU lists.
> >> @@ -146,8 +146,8 @@ struct damon_access_report {
> >>    * The support of each action is up to running &struct damon_operations.
> >>    * &enum DAMON_OPS_VADDR and &enum DAMON_OPS_FVADDR supports all actions except
> >>    * &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.
> >> + * supports all actions except &enum DAMOS_WILLNEED, &enum DAMOS_COLD,
> >> + * &enum DAMOS_HUGEPAGE, &enum DAMOS_NOHUGEPAGE.
> >
> > Thank you for updating this!
> >
> > But, now I find this place is having outdated information that partially
> > duplicated with the DAMON design document[2].  I think the design document is
> > better to maintain this kind of details.  What about keeping the high level
> > information but giving a reference to the documentation for details, for
> > example, like below?
> >
> >      @@ -143,11 +143,9 @@ struct damon_access_report {
> >        * @DAMOS_STAT:                Do nothing but count the stat.
> >        * @NR_DAMOS_ACTIONS:  Total number of DAMOS actions
> >        *
> >      - * The support of each action is up to running &struct damon_operations.
> >      - * &enum DAMON_OPS_VADDR and &enum DAMON_OPS_FVADDR supports all actions except
> >      - * &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.
> >      + * The support of each action is up to running &struct damon_operations.  Refer
> >      + * to 'Operation Action' section of Documentation/mm/damon/design.rst for
> >      + * status of the supports.
> >        */
> >
> > Thanks,
> > SJ
> >
> >
> > [1] https://origin.kernel.org/doc/html/next/mm/damon/maintainer-profile.html#scm-trees
> > [2] https://origin.kernel.org/doc/html/next/mm/damon/design.html#operation-action
>

Best Regards.
Sang-Heon Jeon