Unify folio_put() to make code more clear.
v2:
- use goto lable in damon_pa_pageout() and refact too more functions
Kefeng Wang (3):
mm/damon/paddr: minor refactor of damon_pa_pageout()
mm/damon/paddr: minor refactor of damon_pa_young()
mm/damon/paddr: minor refactor of
damon_pa_mark_accessed_or_deactivate()
mm/damon/paddr.c | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
--
2.35.3
Omit three lines by unified folio_put(), and make code more clear.
Signed-off-by: Kefeng Wang <[email protected]>
---
mm/damon/paddr.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 6c655d9b5639..3fda00a0f786 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -239,21 +239,18 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)
if (!folio)
continue;
- if (damos_pa_filter_out(s, folio)) {
- folio_put(folio);
- continue;
- }
+ if (damos_pa_filter_out(s, folio))
+ goto put_folio;
folio_clear_referenced(folio);
folio_test_clear_young(folio);
- if (!folio_isolate_lru(folio)) {
- folio_put(folio);
- continue;
- }
+ if (!folio_isolate_lru(folio))
+ goto put_folio;
if (folio_test_unevictable(folio))
folio_putback_lru(folio);
else
list_add(&folio->lru, &folio_list);
+put_folio:
folio_put(folio);
}
applied = reclaim_pages(&folio_list);
--
2.35.3
Omit one line by unified folio_put(), and make code more clear.
Signed-off-by: Kefeng Wang <[email protected]>
---
mm/damon/paddr.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 2ef9db0189ca..6930ebf3667c 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -266,17 +266,16 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
if (!folio)
continue;
- if (damos_pa_filter_out(s, folio)) {
- folio_put(folio);
- continue;
- }
+ if (damos_pa_filter_out(s, folio))
+ goto put_folio;
if (mark_accessed)
folio_mark_accessed(folio);
else
folio_deactivate(folio);
- folio_put(folio);
applied += folio_nr_pages(folio);
+put_folio:
+ folio_put(folio);
}
return applied * PAGE_SIZE;
}
--
2.35.3
Omit three lines by unified folio_put(), and make code more clear.
Signed-off-by: Kefeng Wang <[email protected]>
---
mm/damon/paddr.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
index 3fda00a0f786..2ef9db0189ca 100644
--- a/mm/damon/paddr.c
+++ b/mm/damon/paddr.c
@@ -130,24 +130,21 @@ static bool damon_pa_young(unsigned long paddr, unsigned long *folio_sz)
accessed = false;
else
accessed = true;
- folio_put(folio);
goto out;
}
need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
- if (need_lock && !folio_trylock(folio)) {
- folio_put(folio);
- return false;
- }
+ if (need_lock && !folio_trylock(folio))
+ goto out;
rmap_walk(folio, &rwc);
if (need_lock)
folio_unlock(folio);
- folio_put(folio);
-out:
*folio_sz = folio_size(folio);
+out:
+ folio_put(folio);
return accessed;
}
--
2.35.3
On Fri, 3 Mar 2023 16:43:43 +0800 Kefeng Wang <[email protected]> wrote:
> Omit one line by unified folio_put(), and make code more clear.
>
> Signed-off-by: Kefeng Wang <[email protected]>
> ---
> mm/damon/paddr.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 2ef9db0189ca..6930ebf3667c 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -266,17 +266,16 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
> if (!folio)
> continue;
>
> - if (damos_pa_filter_out(s, folio)) {
> - folio_put(folio);
> - continue;
> - }
> + if (damos_pa_filter_out(s, folio))
> + goto put_folio;
>
> if (mark_accessed)
> folio_mark_accessed(folio);
> else
> folio_deactivate(folio);
> - folio_put(folio);
> applied += folio_nr_pages(folio);
> +put_folio:
> + folio_put(folio);
I think this change is ok, but shouldn't the 'folio_put()' have called before
'folio_nr_pages()' anyway? If so, could we make the change as a separate fix
first, and then make this change, so that it can be easily applied to relevant
stable kernels?
Thanks,
SJ
> }
> return applied * PAGE_SIZE;
> }
> --
> 2.35.3
>
>
On Fri, 3 Mar 2023 18:26:33 +0000 SeongJae Park <[email protected]> wrote:
> On Fri, 3 Mar 2023 16:43:43 +0800 Kefeng Wang <[email protected]> wrote:
>
> > Omit one line by unified folio_put(), and make code more clear.
> >
> > Signed-off-by: Kefeng Wang <[email protected]>
> > ---
> > mm/damon/paddr.c | 9 ++++-----
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> > index 2ef9db0189ca..6930ebf3667c 100644
> > --- a/mm/damon/paddr.c
> > +++ b/mm/damon/paddr.c
> > @@ -266,17 +266,16 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
> > if (!folio)
> > continue;
> >
> > - if (damos_pa_filter_out(s, folio)) {
> > - folio_put(folio);
> > - continue;
> > - }
> > + if (damos_pa_filter_out(s, folio))
> > + goto put_folio;
> >
> > if (mark_accessed)
> > folio_mark_accessed(folio);
> > else
> > folio_deactivate(folio);
> > - folio_put(folio);
> > applied += folio_nr_pages(folio);
> > +put_folio:
> > + folio_put(folio);
>
> I think this change is ok, but shouldn't the 'folio_put()' have called before
s/before/after/
> 'folio_nr_pages()' anyway? If so, could we make the change as a separate fix
> first, and then make this change, so that it can be easily applied to relevant
> stable kernels?
>
>
> Thanks,
> SJ
>
> > }
> > return applied * PAGE_SIZE;
> > }
> > --
> > 2.35.3
> >
> >
Hi Kefeng,
On Fri, 3 Mar 2023 16:43:42 +0800 Kefeng Wang <[email protected]> wrote:
> Omit three lines by unified folio_put(), and make code more clear.
>
> Signed-off-by: Kefeng Wang <[email protected]>
> ---
> mm/damon/paddr.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 3fda00a0f786..2ef9db0189ca 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -130,24 +130,21 @@ static bool damon_pa_young(unsigned long paddr, unsigned long *folio_sz)
> accessed = false;
> else
> accessed = true;
> - folio_put(folio);
> goto out;
Because you moved 'out' label to not include *folio_sz setting, folio_sz will
not set in this case. It should be set.
> }
>
> need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
> - if (need_lock && !folio_trylock(folio)) {
> - folio_put(folio);
> - return false;
> - }
> + if (need_lock && !folio_trylock(folio))
> + goto out;
>
> rmap_walk(folio, &rwc);
>
> if (need_lock)
> folio_unlock(folio);
> - folio_put(folio);
>
> -out:
> *folio_sz = folio_size(folio);
> +out:
> + folio_put(folio);
Before this change, folio_size() is called after folio_put(). Shouldn't it be
called before folio_put()? If so, could we make a separate fix for that first,
and then make this change on top of it, so that it can be easily applied to
relevant stable kernels?
Thanks,
SJ
> return accessed;
> }
>
> --
> 2.35.3
>
>
On Fri, 3 Mar 2023 16:43:41 +0800 Kefeng Wang <[email protected]> wrote:
> Omit three lines by unified folio_put(), and make code more clear.
>
> Signed-off-by: Kefeng Wang <[email protected]>
Reviewed-by: SeongJae Park <[email protected]>
Thanks,
SJ
> ---
> mm/damon/paddr.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 6c655d9b5639..3fda00a0f786 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> @@ -239,21 +239,18 @@ static unsigned long damon_pa_pageout(struct damon_region *r, struct damos *s)
> if (!folio)
> continue;
>
> - if (damos_pa_filter_out(s, folio)) {
> - folio_put(folio);
> - continue;
> - }
> + if (damos_pa_filter_out(s, folio))
> + goto put_folio;
>
> folio_clear_referenced(folio);
> folio_test_clear_young(folio);
> - if (!folio_isolate_lru(folio)) {
> - folio_put(folio);
> - continue;
> - }
> + if (!folio_isolate_lru(folio))
> + goto put_folio;
> if (folio_test_unevictable(folio))
> folio_putback_lru(folio);
> else
> list_add(&folio->lru, &folio_list);
> +put_folio:
> folio_put(folio);
> }
> applied = reclaim_pages(&folio_list);
> --
> 2.35.3
>
>
On 2023/3/4 2:39, SeongJae Park wrote:
> Hi Kefeng,
>
> On Fri, 3 Mar 2023 16:43:42 +0800 Kefeng Wang <[email protected]> wrote:
>
>> Omit three lines by unified folio_put(), and make code more clear.
>>
>> Signed-off-by: Kefeng Wang <[email protected]>
>> ---
>> mm/damon/paddr.c | 11 ++++-------
>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
>> index 3fda00a0f786..2ef9db0189ca 100644
>> --- a/mm/damon/paddr.c
>> +++ b/mm/damon/paddr.c
>> @@ -130,24 +130,21 @@ static bool damon_pa_young(unsigned long paddr, unsigned long *folio_sz)
>> accessed = false;
>> else
>> accessed = true;
>> - folio_put(folio);
>> goto out;
>
> Because you moved 'out' label to not include *folio_sz setting, folio_sz will
> not set in this case. It should be set.
oh, it should be fixed.
>
>> }
>>
>> need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
>> - if (need_lock && !folio_trylock(folio)) {
>> - folio_put(folio);
>> - return false;
>> - }
>> + if (need_lock && !folio_trylock(folio))
>> + goto out;
>>
>> rmap_walk(folio, &rwc);
>>
>> if (need_lock)
>> folio_unlock(folio);
>> - folio_put(folio);
>>
>> -out:
>> *folio_sz = folio_size(folio);
>> +out:
>> + folio_put(folio);
>
> Before this change, folio_size() is called after folio_put(). Shouldn't it be
> called before folio_put()? If so, could we make a separate fix for that first,
> and then make this change on top of it, so that it can be easily applied to
> relevant stable kernels?
>
Yes， I could separate it, after folio_put(), the folio could be
re-allocated and the folio_size calculation is not right.
>
> Thanks,
> SJ
>
>> return accessed;
>> }
>>
>> --
>> 2.35.3
>>
>>
On 2023/3/4 2:37, SeongJae Park wrote:
> On Fri, 3 Mar 2023 18:26:33 +0000 SeongJae Park <[email protected]> wrote:
>
>> On Fri, 3 Mar 2023 16:43:43 +0800 Kefeng Wang <[email protected]> wrote:
>>
>>> Omit one line by unified folio_put(), and make code more clear.
>>>
>>> Signed-off-by: Kefeng Wang <[email protected]>
>>> ---
>>> mm/damon/paddr.c | 9 ++++-----
>>> 1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
>>> index 2ef9db0189ca..6930ebf3667c 100644
>>> --- a/mm/damon/paddr.c
>>> +++ b/mm/damon/paddr.c
>>> @@ -266,17 +266,16 @@ static inline unsigned long damon_pa_mark_accessed_or_deactivate(
>>> if (!folio)
>>> continue;
>>>
>>> - if (damos_pa_filter_out(s, folio)) {
>>> - folio_put(folio);
>>> - continue;
>>> - }
>>> + if (damos_pa_filter_out(s, folio))
>>> + goto put_folio;
>>>
>>> if (mark_accessed)
>>> folio_mark_accessed(folio);
>>> else
>>> folio_deactivate(folio);
>>> - folio_put(folio);
>>> applied += folio_nr_pages(folio);
>>> +put_folio:
>>> + folio_put(folio);
>>
>> I think this change is ok, but shouldn't the 'folio_put()' have called before
>
> s/before/after/
>
>> 'folio_nr_pages()' anyway? If so, could we make the change as a separate fix
>> first, and then make this change, so that it can be easily applied to relevant
>> stable kernels?
Yes, seem to previous one.
>>
>>
>> Thanks,
>> SJ
>>
>>> }
>>> return applied * PAGE_SIZE;
>>> }
>>> --
>>> 2.35.3
>>>
>>>
On 2023/3/6 9:10, Kefeng Wang wrote:
>
>
> On 2023/3/4 2:39, SeongJae Park wrote:
>> Hi Kefeng,
>>
>> On Fri, 3 Mar 2023 16:43:42 +0800 Kefeng Wang
>> <[email protected]> wrote:
>>
>>> Omit three lines by unified folio_put(), and make code more clear.
>>>
>>> Signed-off-by: Kefeng Wang <[email protected]>
>>> ---
>>> mm/damon/paddr.c | 11 ++++-------
>>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
>>> index 3fda00a0f786..2ef9db0189ca 100644
>>> --- a/mm/damon/paddr.c
>>> +++ b/mm/damon/paddr.c
>>> @@ -130,24 +130,21 @@ static bool damon_pa_young(unsigned long paddr,
>>> unsigned long *folio_sz)
>>> accessed = false;
>>> else
>>> accessed = true;
>>> - folio_put(folio);
>>> goto out;
>>
>> Because you moved 'out' label to not include *folio_sz setting,
>> folio_sz will
>> not set in this case. It should be set.
> oh, it should be fixed.
>>
>>> }
>>> need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
>>> - if (need_lock && !folio_trylock(folio)) {
>>> - folio_put(folio);
>>> - return false;
>>> - }
Hi SJ, apart from above issue, it looks that this branch need the
folio_size() setting, right?
Thanks
>>> + if (need_lock && !folio_trylock(folio))
>>> + goto out;
>>> rmap_walk(folio, &rwc);
>>> if (need_lock)
>>> folio_unlock(folio);
>>> - folio_put(folio);
>>> -out:
>>> *folio_sz = folio_size(folio);
>>> +out:
>>> + folio_put(folio);
>>
>> Before this change, folio_size() is called after folio_put().
>> Shouldn't it be
>> called before folio_put()? If so, could we make a separate fix for
>> that first,
>> and then make this change on top of it, so that it can be easily
>> applied to
>> relevant stable kernels?
>>
> Yes， I could separate it, after folio_put(), the folio could be
> re-allocated and the folio_size calculation is not right.
>>
>> Thanks,
>> SJ
>>
>>> return accessed;
>>> }
>>> --
>>> 2.35.3
>>>
>>>
Hi Kefeng,
On Mon, 6 Mar 2023 09:56:49 +0800 Kefeng Wang <[email protected]> wrote:
>
>
> On 2023/3/6 9:10, Kefeng Wang wrote:
> >
> >
> > On 2023/3/4 2:39, SeongJae Park wrote:
> >> Hi Kefeng,
> >>
> >> On Fri, 3 Mar 2023 16:43:42 +0800 Kefeng Wang
> >> <[email protected]> wrote:
> >>
> >>> Omit three lines by unified folio_put(), and make code more clear.
> >>>
> >>> Signed-off-by: Kefeng Wang <[email protected]>
> >>> ---
> >>> mm/damon/paddr.c | 11 ++++-------
> >>> 1 file changed, 4 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> >>> index 3fda00a0f786..2ef9db0189ca 100644
> >>> --- a/mm/damon/paddr.c
> >>> +++ b/mm/damon/paddr.c
> >>> @@ -130,24 +130,21 @@ static bool damon_pa_young(unsigned long paddr,
> >>> unsigned long *folio_sz)
> >>> accessed = false;
> >>> else
> >>> accessed = true;
> >>> - folio_put(folio);
> >>> goto out;
> >>
> >> Because you moved 'out' label to not include *folio_sz setting,
> >> folio_sz will
> >> not set in this case. It should be set.
> > oh, it should be fixed.
> >>
> >>> }
> >>> need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
> >>> - if (need_lock && !folio_trylock(folio)) {
> >>> - folio_put(folio);
> >>> - return false;
> >>> - }
>
> Hi SJ, apart from above issue, it looks that this branch need the
> folio_size() setting, right?
folio_sz is effectively used by caller of damon_pa_young() only if this
function returns true, so this branch doesn't need to set folio_sz.
Thanks,
SJ
>
> Thanks
>
> >>> + if (need_lock && !folio_trylock(folio))
> >>> + goto out;
> >>> rmap_walk(folio, &rwc);
> >>> if (need_lock)
> >>> folio_unlock(folio);
> >>> - folio_put(folio);
> >>> -out:
> >>> *folio_sz = folio_size(folio);
> >>> +out:
> >>> + folio_put(folio);
> >>
> >> Before this change, folio_size() is called after folio_put().
> >> Shouldn't it be
> >> called before folio_put()? If so, could we make a separate fix for
> >> that first,
> >> and then make this change on top of it, so that it can be easily
> >> applied to
> >> relevant stable kernels?
> >>
> > Yes， I could separate it, after folio_put(), the folio could be
> > re-allocated and the folio_size calculation is not right.
> >>
> >> Thanks,
> >> SJ
> >>
> >>> return accessed;
> >>> }
> >>> --
> >>> 2.35.3
> >>>
> >>>
>
On 2023/3/7 5:27, SeongJae Park wrote:
> Hi Kefeng,
>
> On Mon, 6 Mar 2023 09:56:49 +0800 Kefeng Wang <[email protected]> wrote:
>
>>
>>
>> On 2023/3/6 9:10, Kefeng Wang wrote:
>>>
>>>
>>> On 2023/3/4 2:39, SeongJae Park wrote:
>>>> Hi Kefeng,
>>>>
>>>> On Fri, 3 Mar 2023 16:43:42 +0800 Kefeng Wang
>>>> <[email protected]> wrote:
>>>>
>>>>> Omit three lines by unified folio_put(), and make code more clear.
>>>>>
>>>>> Signed-off-by: Kefeng Wang <[email protected]>
>>>>> ---
>>>>> mm/damon/paddr.c | 11 ++++-------
>>>>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
>>>>> index 3fda00a0f786..2ef9db0189ca 100644
>>>>> --- a/mm/damon/paddr.c
>>>>> +++ b/mm/damon/paddr.c
>>>>> @@ -130,24 +130,21 @@ static bool damon_pa_young(unsigned long paddr,
>>>>> unsigned long *folio_sz)
>>>>> accessed = false;
>>>>> else
>>>>> accessed = true;
>>>>> - folio_put(folio);
>>>>> goto out;
>>>>
>>>> Because you moved 'out' label to not include *folio_sz setting,
>>>> folio_sz will
>>>> not set in this case. It should be set.
>>> oh, it should be fixed.
>>>>
>>>>> }
>>>>> need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
>>>>> - if (need_lock && !folio_trylock(folio)) {
>>>>> - folio_put(folio);
>>>>> - return false;
>>>>> - }
>>
>> Hi SJ, apart from above issue, it looks that this branch need the
>> folio_size() setting, right?
>
> folio_sz is effectively used by caller of damon_pa_young() only if this
> function returns true, so this branch doesn't need to set folio_sz.
__damon_pa_check_access() store last_addr, last_accessed and
last_folio_sz, even damon_pa_young() return false, the following check
still use last_folio_sz,
ALIGN_DOWN(last_addr, last_folio_sz) == ALIGN_DOWN(r->sampling_addr,
last_folio_sz)
but last_folio_sz is not up to date, so I think it need to update, and
update last_folio_sz is harmless, which could let's unify the return
path, correct me if I am wrong.
On Tue, 7 Mar 2023 09:22:33 +0800 Kefeng Wang <[email protected]> wrote:
>
>
> On 2023/3/7 5:27, SeongJae Park wrote:
> > Hi Kefeng,
> >
> > On Mon, 6 Mar 2023 09:56:49 +0800 Kefeng Wang <[email protected]> wrote:
> >
> >>
> >>
> >> On 2023/3/6 9:10, Kefeng Wang wrote:
> >>>
> >>>
> >>> On 2023/3/4 2:39, SeongJae Park wrote:
> >>>> Hi Kefeng,
> >>>>
> >>>> On Fri, 3 Mar 2023 16:43:42 +0800 Kefeng Wang
> >>>> <[email protected]> wrote:
> >>>>
> >>>>> Omit three lines by unified folio_put(), and make code more clear.
> >>>>>
> >>>>> Signed-off-by: Kefeng Wang <[email protected]>
> >>>>> ---
> >>>>> mm/damon/paddr.c | 11 ++++-------
> >>>>> 1 file changed, 4 insertions(+), 7 deletions(-)
> >>>>>
> >>>>> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> >>>>> index 3fda00a0f786..2ef9db0189ca 100644
> >>>>> --- a/mm/damon/paddr.c
> >>>>> +++ b/mm/damon/paddr.c
> >>>>> @@ -130,24 +130,21 @@ static bool damon_pa_young(unsigned long paddr,
> >>>>> unsigned long *folio_sz)
> >>>>> accessed = false;
> >>>>> else
> >>>>> accessed = true;
> >>>>> - folio_put(folio);
> >>>>> goto out;
> >>>>
> >>>> Because you moved 'out' label to not include *folio_sz setting,
> >>>> folio_sz will
> >>>> not set in this case. It should be set.
> >>> oh, it should be fixed.
> >>>>
> >>>>> }
> >>>>> need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
> >>>>> - if (need_lock && !folio_trylock(folio)) {
> >>>>> - folio_put(folio);
> >>>>> - return false;
> >>>>> - }
> >>
> >> Hi SJ, apart from above issue, it looks that this branch need the
> >> folio_size() setting, right?
> >
> > folio_sz is effectively used by caller of damon_pa_young() only if this
> > function returns true, so this branch doesn't need to set folio_sz.
>
> __damon_pa_check_access() store last_addr, last_accessed and
> last_folio_sz, even damon_pa_young() return false, the following check
> still use last_folio_sz,
>
> ALIGN_DOWN(last_addr, last_folio_sz) == ALIGN_DOWN(r->sampling_addr,
> last_folio_sz)
>
> but last_folio_sz is not up to date, so I think it need to update, and
> update last_folio_sz is harmless, which could let's unify the return
> path, correct me if I am wrong.
Ah, you're right. Thank you for kind explanation. I was out of my mind for
some reason. Maybe we could just do 'goto out' in the branch.
Thanks,
SJ
On 2023/3/8 2:00, SeongJae Park wrote:
> On Tue, 7 Mar 2023 09:22:33 +0800 Kefeng Wang <[email protected]> wrote:
>
>>
>>
>> On 2023/3/7 5:27, SeongJae Park wrote:
>>> Hi Kefeng,
>>>
>>> On Mon, 6 Mar 2023 09:56:49 +0800 Kefeng Wang <[email protected]> wrote:
>>>
>>>>
>>>>
>>>> On 2023/3/6 9:10, Kefeng Wang wrote:
>>>>>
>>>>>
>>>>> On 2023/3/4 2:39, SeongJae Park wrote:
>>>>>> Hi Kefeng,
>>>>>>
>>>>>> On Fri, 3 Mar 2023 16:43:42 +0800 Kefeng Wang
>>>>>> <[email protected]> wrote:
>>>>>>
>>>>>>> Omit three lines by unified folio_put(), and make code more clear.
>>>>>>>
>>>>>>> Signed-off-by: Kefeng Wang <[email protected]>
>>>>>>> ---
>>>>>>> mm/damon/paddr.c | 11 ++++-------
>>>>>>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
>>>>>>> index 3fda00a0f786..2ef9db0189ca 100644
>>>>>>> --- a/mm/damon/paddr.c
>>>>>>> +++ b/mm/damon/paddr.c
>>>>>>> @@ -130,24 +130,21 @@ static bool damon_pa_young(unsigned long paddr,
>>>>>>> unsigned long *folio_sz)
>>>>>>> accessed = false;
>>>>>>> else
>>>>>>> accessed = true;
>>>>>>> - folio_put(folio);
>>>>>>> goto out;
>>>>>>
>>>>>> Because you moved 'out' label to not include *folio_sz setting,
>>>>>> folio_sz will
>>>>>> not set in this case. It should be set.
>>>>> oh, it should be fixed.
>>>>>>
>>>>>>> }
>>>>>>> need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
>>>>>>> - if (need_lock && !folio_trylock(folio)) {
>>>>>>> - folio_put(folio);
>>>>>>> - return false;
>>>>>>> - }
>>>>
>>>> Hi SJ, apart from above issue, it looks that this branch need the
>>>> folio_size() setting, right?
>>>
>>> folio_sz is effectively used by caller of damon_pa_young() only if this
>>> function returns true, so this branch doesn't need to set folio_sz.
>>
>> __damon_pa_check_access() store last_addr, last_accessed and
>> last_folio_sz, even damon_pa_young() return false, the following check
>> still use last_folio_sz,
>>
>> ALIGN_DOWN(last_addr, last_folio_sz) == ALIGN_DOWN(r->sampling_addr,
>> last_folio_sz)
>>
>> but last_folio_sz is not up to date, so I think it need to update, and
>> update last_folio_sz is harmless, which could let's unify the return
>> path, correct me if I am wrong.
>
> Ah, you're right. Thank you for kind explanation. I was out of my mind for
> some reason. Maybe we could just do 'goto out' in the branch.
Yes, will update this patchset with this change.
>
>
> Thanks,
> SJ