lkml.org 
[lkml]   [2022]   [Aug]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V2 1/2] mm/damon/lru_sort: Move target memory region check to head of func
From

在 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@linux.alibaba.com> 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 ?






>

\
 
 \ /
  Last update: 2022-08-19 04:49    [W:0.135 / U:0.620 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site