| Message ID | 20230303084343.171958-3-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/damon/paddr: minor code improvement | expand |
Hi Kefeng, On Fri, 3 Mar 2023 16:43:42 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > Omit three lines by unified folio_put(), and make code more clear. > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > 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 2023/3/4 2:39, SeongJae Park wrote: > Hi Kefeng, > > On Fri, 3 Mar 2023 16:43:42 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> Omit three lines by unified folio_put(), and make code more clear. >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> 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/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 >> <wangkefeng.wang@huawei.com> wrote: >> >>> Omit three lines by unified folio_put(), and make code more clear. >>> >>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>> --- >>> 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 <wangkefeng.wang@huawei.com> 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 > >> <wangkefeng.wang@huawei.com> wrote: > >> > >>> Omit three lines by unified folio_put(), and make code more clear. > >>> > >>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > >>> --- > >>> 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 <wangkefeng.wang@huawei.com> 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 >>>> <wangkefeng.wang@huawei.com> wrote: >>>> >>>>> Omit three lines by unified folio_put(), and make code more clear. >>>>> >>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>>>> --- >>>>> 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 <wangkefeng.wang@huawei.com> wrote: > > > On 2023/3/7 5:27, SeongJae Park wrote: > > Hi Kefeng, > > > > On Mon, 6 Mar 2023 09:56:49 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> 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 > >>>> <wangkefeng.wang@huawei.com> wrote: > >>>> > >>>>> Omit three lines by unified folio_put(), and make code more clear. > >>>>> > >>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > >>>>> --- > >>>>> 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 <wangkefeng.wang@huawei.com> wrote: > >> >> >> On 2023/3/7 5:27, SeongJae Park wrote: >>> Hi Kefeng, >>> >>> On Mon, 6 Mar 2023 09:56:49 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> 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 >>>>>> <wangkefeng.wang@huawei.com> wrote: >>>>>> >>>>>>> Omit three lines by unified folio_put(), and make code more clear. >>>>>>> >>>>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>>>>>> --- >>>>>>> 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
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; }
Omit three lines by unified folio_put(), and make code more clear. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- mm/damon/paddr.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)