| Message ID | 20230919095237.26613-1-link@vivo.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Damos filter cleanup code and implement workingset | expand |
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@vivo.com> 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 > >
HI SJ, Thanks for your patient response. > [Some people who received this message don't often get email from sj@kernel.org. 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@vivo.com> 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? > >> 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.) > 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? :) > 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.) > 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. > >> 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 >> >>
Hi Huan, On Fri, 22 Sep 2023 02:54:58 +0000 杨欢 <link@vivo.com> 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@kernel.org. 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@vivo.com> 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? > > > > >> 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. 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. > > > 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? > > > 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 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. Thanks, SJ > > > > >> 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 > >> > >> >
HI SJ, > Hi Huan, > > On Fri, 22 Sep 2023 02:54:58 +0000 杨欢 <link@vivo.com> 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@kernel.org. 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@vivo.com> 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. :) > > > 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 >>>> >>>>
Hi Huan, On Mon, 25 Sep 2023 02:10:58 +0000 杨欢 <link@vivo.com> wrote: > HI SJ, > > Hi Huan, > > > > On Fri, 22 Sep 2023 02:54:58 +0000 杨欢 <link@vivo.com> 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@kernel.org. 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@vivo.com> 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 > >>>> > >>>>