Hi Huan, On Mon, 25 Sep 2023 02:10:58 +0000 杨欢 <link@xxxxxxxx> wrote: > HI SJ, > > Hi Huan, > > > > On Fri, 22 Sep 2023 02:54:58 +0000 杨欢 <link@xxxxxxxx> wrote: > > > >> HI SJ, > >> > >> Thanks for your patient response. > > Happy to hear this kind acknowledgement :) > > > >>> [Some people who received this message don't often get email from sj@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > >>> > >>> 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 > >> Thanks for these instructions to help me understand the design idea of > >> damos filter. > >>> 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. > >> IMO, some simple filter can have a common implement, like anon filter? And, > >> for some special type, each layer offer their own? > > Do you mean there could be two filter types that better to be handled in > > different layer for efficiency, but share common implementation? Could you > > please give me a more specific example? > > It's hard to offer a specific example due to current ops implement is so > great to cover > > much situation. Maybe Heterogeneous memory may add a new ops(just > examples of > > imprudence, like intel's or other network memory?) . And offer a common > ops filter code > > can may some simple type be reused. > > > > >>>> 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 > >> Yes, indeed. > >>> 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. > >> I think the filter code scatter into each layer may make code hard to > >> reuse, other ops > >> > >> may need anon filter or something. So, make code into a dedicated file > >> may good? > >> > >> (just my opinion.) > > Again, I'm not super confident about my understanding. But sounds like you're > > partly worrying about duplication of some code in each ops implementation > > (modules in same layer). Please correct me if I'm wrong, with specific, > > detailed and realistic examples. > > > > If my guess is not that wrong, I can agree to the concern. Nevertheless, we > > already have a dedicated source file for such common code between ops > > implementaions, namely ops-common.c. > Yes, no need to split a single file to drive filter. > > > > That said, we don't have duplicated DAMOS filters implementation code in > > multipe ops implementation at the moment. It could have such duplication in > > the future, but I think it's not too late to make such cleanup after or just > > before such duplication heppen. IOW, I'd suggest to not make changes for > > something that _might_ happen in future. > > Yes, indeed. We needn't to make this right now.(That's the why RFC, > meanwhile, my code is > > not good.) > > > > >>> 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 > >> Yes, indeed, current code not clean, the idea didn't take shape. > >>> 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. > >> In this code, maybe just a little benefit when other ops need to filter > >> anon page? :) > > As mentioned above, it's unclear when, how, and for what benefit we will > > support anon pages filter from vaddr. I'd again suggest to not make change > > only for future change that not clear if we really want to make for now. > > > >>> 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 > >> IMO, page if marked as workingset, mean page in task's core > >> workload(maybe have > >> > >> seen the refault, and trigger refault protect). So, for lru sort or reclaim, > >> > >> we'd better not touch it?(If any wrong, please let me know.) > > Still unclear to me. Could I ask you more specific and detailed case? > > I may have misunderstood, I suggest you just look: > > Page workingset flag will mark to page when page need to deactive or > refault and > > find it already marked. In some memory path(migrate, reclaim or else.), > touch > > workingset page require special attention.(Enter psi mm stall or else) > > So, I think help filter out this is helpful.(Of course, just thought > experiment, no helpful data). > > As, above and below. This RFC patch. :). I will share when get valuable > data. > > > > >>> 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. > >> Sorry, this type just from my knowledge of MM, have no test data. > >> > >> For futher learn of DAMON, I'll try it. > > Yes, that will be very helpful. > > > > And from this point, I'm getting an impression that the purpose of this RFC is > > not for making a real change for specific use case that assumed to make real > > benefits, but just for getting opinions about some imaginable changes that > > _might_ be helpful, and _might_ be made in future. If so, making the point > Yes, I just learn DAMON a little time, and offer some thinking for this. > > more clear would be helpful for me to give you opinion earlier. If that's the > > case, my opinion is this. I agree DAMON code has many rooms of improvement in > > terms of readability, so cleanup patches are welcome. Nevertheless, I'd prefer > > to make changes only when it is reasonable to expect it's providing _real_ > > improvement just after be applied, or at least very near and specific future. > Yes, keep this and change when we need indeed. :) Ok, makes sense. Let's do further discussion after some more data and realistic detailed example be available. :) Looking forward to! Thanks, SJ > > > > > > Thanks, > > SJ > > Thans, > > Huan > > >>>> 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 > >> Thanks, > >> > >> Huan > >> > >>>> 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 > >>>> > >>>>
