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

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

Hi, SeongJae

On Fri, Aug 1, 2025 at 2:58 AM SeongJae Park <sj@kernel.org> 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.

Thanks for your kind review. I really don't want to bother you
anymore. But I would like to check my understanding.

So the basic operation of stat is covered by outside of
`damon_pa_apply_scheme` and `damon_va_apply_scheme`.
If it's not too much trouble, could you point me to where they are?

And some extension stat (maybe it only renewed that filter exists) is
covered inside of upper functions. Is it also right point?

> > 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].

I see. I'll keep this in my mind. Thank you for telling me!

> >   *
> >   * @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!
>
> >   * @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?

I totally agree with you. Unexpected duplication sometimes leads to
unexpected expiration like this one
So, I think that I need to send v2 patch, If that's right, which way
would you prefer?
reply to this thread or make a new thread?

>     @@ -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