Re: [RFC 0/2] Damos filter cleanup code and implement workingset

[SeongJae Park]

Hi Huan,


First of all, thank you for this patch.  I have some high level comments and
questions as below.

On Tue, 19 Sep 2023 17:52:33 +0800 Huan Yang <link@xxxxxxxx> wrote:

> Now damos support filter contains two type.
> The first is scheme filter which will invoke each scheme apply,
> second is scheme action filter, which will filter out unwanted action.

IMHO, that's not clear definition of the types.  Conceptually, all the filters
are same.  Nonetheless, they are having different behaviors because they are
handled in different layer depending on which layer is more efficient to handle
those[1].

I agree this is complicated and a bit verbose explanation, but I don't feel
your scheme vs action definition is making it easier to understand.

> 
> But this implement is scattered in different implementations

Indeed the implementation is scattered in core layer and the ops layer
depending on the filter types.  But I think this is needed for efficient
handling of those.

> and hard to reuse or extend.

>From your first patch, which extending the filter, the essential change for the
extension is adding another enum to 'enum damos_filter_type' (1 line) and
addition of switch case in '__damos_pa_filter_out()' (3 lines).  I don't think
it looks too complicated.  If you're saying it was hard to understand which
part really need to be modifed, I think that makes sense.  But in the case,
we might need more comments rather than structural change.

> 
> This patchset clean up those filter code, move into filter.c and add header
> to expose filter func.(patch 2)

Again, I agree more code cleanup is needed.  But I'm not sure if this is the
right way.  Especially, I'm unsure if it really need to have a dedicated file.
To my humble eyes, it doesn't look like making code clearly easier to read
compared to the current version.  This is probably because I don't feel your
scheme vs action definition is clear to understand.  Neither it is
deduplicating code.  The patch adds lines more that deleted ones, according to
its diffstat.  For the reason, I don't see clear benefit of this change.  I
might be biased, having NIH (Not Invented Here) sindrome, or missing something.
Please let me know if so.

> And add a new filter "workingset" to protect workingset page.

Can you explain expected use cases of this new filter type in more detail?
Specifically, for what scheme action and under what situation this new filter
type will be needed?  If you have some test data for the use case and could
share it, it would be very helpful for me to understand why it is needed.

> 
> Do we need this and cleanup it?

I think I cannot answer for now, and your further clarification and patient
explanation could be helpful.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/tree/Documentation/mm/damon/design.rst?h=mm-everything-2023-09-20-19-38#n400


Thanks,
SJ

> 
> Huan Yang (2):
>   mm/damos/filter: Add workingset page filter
>   mm/damos/filter: Damos filter cleanup
> 
>  include/linux/damon.h    |  62 +-----------------
>  mm/damon/Makefile        |   2 +-
>  mm/damon/core-test.h     |   7 ++
>  mm/damon/core.c          |  93 ++++-----------------------
>  mm/damon/filter.c        | 135 +++++++++++++++++++++++++++++++++++++++
>  mm/damon/filter.h        | 119 ++++++++++++++++++++++++++++++++++
>  mm/damon/paddr.c         |  29 +++------
>  mm/damon/reclaim.c       |  48 +++++++++++---
>  mm/damon/sysfs-schemes.c |   1 +
>  9 files changed, 325 insertions(+), 171 deletions(-)
>  create mode 100644 mm/damon/filter.c
>  create mode 100644 mm/damon/filter.h
> 
> -- 
> 2.34.1
> 
>