Re: [PATCH V2 1/2] mm/damon/lru_sort: Move target memory region check to head of func

[haoxin]

在 2022/8/19 上午10:28, SeongJae Park 写道:
> Hi Xin,
>
> > 在 2022/8/19 上午1:11, SeongJae Park 写道:
> > > Hi Xin,
> > >
> > >
> > > On Thu, 18 Aug 2022 18:57:31 +0800 Xin Hao <xhao@xxxxxxxxxxxxxxxxx> wrote:
> > >
> > > > In damon_lru_sort_apply_parameters(), if "monitor_region_start"
> > > > and "monitor_region_end" is not a valid physical address range,
> > > > There no need to run the remainder codes in it.
> > > The function, 'damon_lru_sort_apply_parameters()', checks validity of
> > > parameters and construct the DAMON context one by one.  For example,
> > > 'damon_set_attrs()' returns an error if the parameters are invalid.  So the
> > > intended flow is,
> > >
> > > 1. check DAMON attributes parameters,
> > > 2. apply DAMON attributes parameters,
> > > 3. check scheme parameters,
> > > 4. apply scheme parameters,
> > > 5. check target region parameters, and
> > > 6. apply target region parameters.
> > >
> > > Therefore what this patch does is making the target regions validity check to
> > > be done earlier than validity checks of other parameters.  There is no special
> > > reason to check the region earlier than others.  Also, this change makes the
> > > flow of the function a little bit weird in my humble opinion, as the flow will
> > > be
> > >
> > > 1. check target region parameters,
> > > 2. check DAMON attributes parameters,
> > > 3. apply DAMON attributes parameters,
> > > 4. check scheme parameters,
> > > 5. apply scheme parameters, and
> > > 6. apply target region parameters.
> > Ok, understand what you mean,   my fix looks ugly,  buy any apply above
> > are not not necessary if one of them checks failed, why not check all
> > fisrt and then apply them, like this:
> >
> > 1. check target region parameters,
> >
> > 2. check DAMON attributes parameters,
> >
> > 3. check scheme parameters,
> The parameter values could be changed by users after the check, so we should
> cache those somewhere anyway.  In other words, we cache those in the DAMON
> context.  Therefore I think the above works were not totally waste of the time.
> Also, because the parameters applying functions like 'damon_set_attrs()' does
> the check and applying of the parameters together, I feel like current flow is
> natural.

Ok,  Thank you for your detailed  explain, just keep it. but there still 
a problem in damon_lru_sort_apply_parameters

if (!monitor_region_start && !monitor_region_end &&
		!get_monitoring_region(&monitor_region_start,
			&monitor_region_end))

if (!monitor_region_start || !monitor_region_end ||
		!get_monitoring_region(&monitor_region_start,
			&monitor_region_end))

the '&&' should fix to '||',  anyone checks fail, it should return ?






>