As my previous comments are almost only cosmetic trivial nits and I don't want to make this unnecessarily delayed long, I made the changes on my own and posted it: https://lore.kernel.org/damon/20220909213606.136221-1-sj@xxxxxxxxxx/ Xin, if there was anything I missed or there is anything you disagree about my changes, please let me know. Thanks, SJ On Fri, 9 Sep 2022 20:45:20 +0000 SeongJae Park <sj@xxxxxxxxxx> wrote: > On Fri, 9 Sep 2022 10:41:05 +0800 Xin Hao <xhao@xxxxxxxxxxxxxxxxx> wrote: > > > In lru_sort.c and reclaim.c, they are all defining get_monitoring_region() > > function, there is no need to define it separately. > > > > As 'get_monitoring_region()' is not a 'static' function anymore, we try > > to use a prefix to distinguish with other functions, so there rename it > > to 'damon_find_biggest_system_ram'. > > > > Suggested-by: SeongJae Park <sj@xxxxxxxxxx> > > Signed-off-by: Xin Hao <xhao@xxxxxxxxxxxxxxxxx> > > --- > > include/linux/damon.h | 11 +++++++++++ > > mm/damon/core.c | 29 +++++++++++++++++++++++++++++ > > mm/damon/lru_sort.c | 37 ++----------------------------------- > > mm/damon/reclaim.c | 37 ++----------------------------------- > > 4 files changed, 44 insertions(+), 70 deletions(-) > > > > diff --git a/include/linux/damon.h b/include/linux/damon.h > > index 7b1f4a488230..6c863b281fb2 100644 > > --- a/include/linux/damon.h > > +++ b/include/linux/damon.h > > @@ -448,6 +448,16 @@ struct damon_ctx { > > struct list_head schemes; > > }; > > > > +/** > > + * struct damon_system_ram_region - System RAM resource address region of [@start, @end). > > I prefer 80 columns, let's break down this line. > https://docs.kernel.org/process/coding-style.html#breaking-long-lines-and-strings > > Also this struct is gonna be used by only damon_find_biggest_system_ram(), so I > think it might make more sense to move this into core.c. > > And, as this is not aimed to directly be used by external API users, I think it > would make more sense to hide from kernel doc (/* instead of /**). > > > + * @start: Start address of the (inclusive). > > of the 'region'? > > > + * @end: End address of the region (exclusive). > > I like the nice explanation: whether its inclusive or exclusive. > > > + */ > > +struct damon_system_ram_region { > > + unsigned long start; > > + unsigned long end; > > +}; > > + > > As this struct is only used by damon_find_biggest_system_ram(), I think it > might make more sense to move this into core.c? > > Below parts all look good. > > Also, this patch seems cannot cleanly applied on top of the latest > mm/mm-unstable branch. Would need rebase. > > > Thanks, > SJ > > [...] >
