Re: [PATCH V3] mm/damon: Remove duplicate get_monitoring_region() definitions

[haoxin]

在 2022/9/9 上午2:19, SeongJae Park 写道:
> Hi Xin,
>
>
> Thanks for your efforts!  But, please answer to some more comments below.
>
> > In lru_sort.c and reclaim.c, they are all defining get_monitoring_region()
> > function, there is no need to define it separately.
> >
> > BTW, this patch removes two struct 'damon_lru_sort_ram_walk_arg' and
> > 'damon_reclaim_ram_walk_arg', though the two structs are removed, if we
> > want to add more fields to these structs for other purposes later, it will
> > not too late for us to use them again.
> > For example:
> >      struct damon_reclaim_ram_walk_arg {
> > 	struct damon_addr_range raw_walk;
> > 	xxx  A;
> > 	xxx  B;
> >      }
> >      struct damon_lru_sort_ram_walk_arg {
> > 	struct damon_addr_range raw_walk;
> > 	xxx  A;
> > 	xxx  B;
> >      }
> Sorry, seems I didn't make my words clear enough.  What I'm concerning[1] is,
> any possible future change to 'struct damon_addr_range', not to
> 'struct damon_{reclaim,lru_sort}_ram_walk_arg'.
>
> Also, after all, as mentioned before, the purpose of 'struct damon_addr_range'
> is not saving the 'start' and 'end' fields of 'struct resource'.  Let's use
> different types for different purposes to avoid any unneeded dependencies.
Ok， i will define another struct.
> [1] https://lore.kernel.org/damon/20220907172712.61006-1-sj@xxxxxxxxxx/
>
> > Signed-off-by: Xin Hao <xhao@xxxxxxxxxxxxxxxxx>
> > ---
> >   include/linux/damon.h |  1 +
> >   mm/damon/core.c       | 28 ++++++++++++++++++++++++++++
> >   mm/damon/lru_sort.c   | 37 ++-----------------------------------
> >   mm/damon/reclaim.c    | 37 ++-----------------------------------
> >   4 files changed, 33 insertions(+), 70 deletions(-)
> >
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 7b1f4a488230..f27b92e5afc2 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -500,6 +500,7 @@ void damon_add_region(struct damon_region *r, struct damon_target *t);
> >   void damon_destroy_region(struct damon_region *r, struct damon_target *t);
> >   int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges,
> >   		unsigned int nr_ranges);
> > +bool damon_get_sram_monitoring_region(unsigned long *start, unsigned long *end);
> >
> >   struct damos *damon_new_scheme(
> >   		unsigned long min_sz_region, unsigned long max_sz_region,
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 7d25dc582fe3..05a2d1b9d03d 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -276,6 +276,34 @@ struct damos *damon_new_scheme(
> >   	return scheme;
> >   }
> >
> > +static inline int walk_system_ram(struct resource *res, void *arg)
> We will pass pointer to this function.  I guess 'inline' makes no sense?
 Ok, i will remove it.
> > +{
> > +	struct damon_addr_range *a = arg;
> > +
> > +	if (a->end - a->start < resource_size(res)) {
> > +		a->start = res->start;
> > +		a->end = res->end;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Find biggest 'System RAM' resource and store its start and end address in
> > + * @start and @end, respectively.  If no System RAM is found, returns false.
> > + */
> > +bool damon_get_sram_monitoring_region(unsigned long *start, unsigned long *end)
> We should avoid too verbose name, so 'sram' looks nice, but it might be able to
> be more clear to let readers know what it does.  For an example, how about
> 'damon_find_biggest_system_ram()'?
Ok.
> Below parts all look good.
>
>
> Thanks,
> SJ
>
> [...]