* [PATCH v2 0/3] mm/damon/paddr: minor code improvement @ 2023-03-03 8:43 Kefeng Wang 2023-03-03 8:43 ` [PATCH v2 1/3] mm/damon/paddr: minor refactor of damon_pa_pageout() Kefeng Wang ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Kefeng Wang @ 2023-03-03 8:43 UTC (permalink / raw) To: SeongJae Park, Andrew Morton; +Cc: linux-mm, linux-kernel, damon, Kefeng Wang 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/3] mm/damon/paddr: minor refactor of damon_pa_pageout() 2023-03-03 8:43 [PATCH v2 0/3] mm/damon/paddr: minor code improvement Kefeng Wang @ 2023-03-03 8:43 ` Kefeng Wang 2023-03-03 18:41 ` SeongJae Park 2023-03-03 8:43 ` [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() Kefeng Wang 2023-03-03 8:43 ` [PATCH v2 3/3] mm/damon/paddr: minor refactor of damon_pa_mark_accessed_or_deactivate() Kefeng Wang 2 siblings, 1 reply; 15+ messages in thread From: Kefeng Wang @ 2023-03-03 8:43 UTC (permalink / raw) To: SeongJae Park, Andrew Morton; +Cc: linux-mm, linux-kernel, damon, Kefeng Wang 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 | 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/3] mm/damon/paddr: minor refactor of damon_pa_pageout() 2023-03-03 8:43 ` [PATCH v2 1/3] mm/damon/paddr: minor refactor of damon_pa_pageout() Kefeng Wang @ 2023-03-03 18:41 ` SeongJae Park 0 siblings, 0 replies; 15+ messages in thread From: SeongJae Park @ 2023-03-03 18:41 UTC (permalink / raw) To: Kefeng Wang; +Cc: SeongJae Park, Andrew Morton, linux-mm, linux-kernel, damon On Fri, 3 Mar 2023 16:43:41 +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> Reviewed-by: SeongJae Park <sj@kernel.org> 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() 2023-03-03 8:43 [PATCH v2 0/3] mm/damon/paddr: minor code improvement Kefeng Wang 2023-03-03 8:43 ` [PATCH v2 1/3] mm/damon/paddr: minor refactor of damon_pa_pageout() Kefeng Wang @ 2023-03-03 8:43 ` Kefeng Wang 2023-03-03 18:39 ` SeongJae Park 2023-03-03 8:43 ` [PATCH v2 3/3] mm/damon/paddr: minor refactor of damon_pa_mark_accessed_or_deactivate() Kefeng Wang 2 siblings, 1 reply; 15+ messages in thread From: Kefeng Wang @ 2023-03-03 8:43 UTC (permalink / raw) To: SeongJae Park, Andrew Morton; +Cc: linux-mm, linux-kernel, damon, Kefeng Wang 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; } 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() 2023-03-03 8:43 ` [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() Kefeng Wang @ 2023-03-03 18:39 ` SeongJae Park 2023-03-06 1:10 ` Kefeng Wang 0 siblings, 1 reply; 15+ messages in thread From: SeongJae Park @ 2023-03-03 18:39 UTC (permalink / raw) To: Kefeng Wang; +Cc: SeongJae Park, Andrew Morton, linux-mm, linux-kernel, damon 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() 2023-03-03 18:39 ` SeongJae Park @ 2023-03-06 1:10 ` Kefeng Wang 2023-03-06 1:56 ` Kefeng Wang 0 siblings, 1 reply; 15+ messages in thread From: Kefeng Wang @ 2023-03-06 1:10 UTC (permalink / raw) To: SeongJae Park; +Cc: Andrew Morton, linux-mm, linux-kernel, damon 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 >> >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() 2023-03-06 1:10 ` Kefeng Wang @ 2023-03-06 1:56 ` Kefeng Wang 2023-03-06 21:27 ` SeongJae Park 0 siblings, 1 reply; 15+ messages in thread From: Kefeng Wang @ 2023-03-06 1:56 UTC (permalink / raw) To: SeongJae Park; +Cc: Andrew Morton, linux-mm, linux-kernel, damon 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 >>> >>> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() 2023-03-06 1:56 ` Kefeng Wang @ 2023-03-06 21:27 ` SeongJae Park 2023-03-07 1:22 ` Kefeng Wang 0 siblings, 1 reply; 15+ messages in thread From: SeongJae Park @ 2023-03-06 21:27 UTC (permalink / raw) To: Kefeng Wang; +Cc: SeongJae Park, Andrew Morton, linux-mm, linux-kernel, damon 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 > >>> > >>> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() 2023-03-06 21:27 ` SeongJae Park @ 2023-03-07 1:22 ` Kefeng Wang 2023-03-07 18:00 ` SeongJae Park 0 siblings, 1 reply; 15+ messages in thread From: Kefeng Wang @ 2023-03-07 1:22 UTC (permalink / raw) To: SeongJae Park; +Cc: Andrew Morton, linux-mm, linux-kernel, damon 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() 2023-03-07 1:22 ` Kefeng Wang @ 2023-03-07 18:00 ` SeongJae Park 2023-03-08 1:03 ` Kefeng Wang 0 siblings, 1 reply; 15+ messages in thread From: SeongJae Park @ 2023-03-07 18:00 UTC (permalink / raw) To: Kefeng Wang; +Cc: SeongJae Park, Andrew Morton, linux-mm, linux-kernel, damon 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() 2023-03-07 18:00 ` SeongJae Park @ 2023-03-08 1:03 ` Kefeng Wang 0 siblings, 0 replies; 15+ messages in thread From: Kefeng Wang @ 2023-03-08 1:03 UTC (permalink / raw) To: SeongJae Park; +Cc: Andrew Morton, linux-mm, linux-kernel, damon 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/3] mm/damon/paddr: minor refactor of damon_pa_mark_accessed_or_deactivate() 2023-03-03 8:43 [PATCH v2 0/3] mm/damon/paddr: minor code improvement Kefeng Wang 2023-03-03 8:43 ` [PATCH v2 1/3] mm/damon/paddr: minor refactor of damon_pa_pageout() Kefeng Wang 2023-03-03 8:43 ` [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() Kefeng Wang @ 2023-03-03 8:43 ` Kefeng Wang 2023-03-03 18:26 ` SeongJae Park 2 siblings, 1 reply; 15+ messages in thread From: Kefeng Wang @ 2023-03-03 8:43 UTC (permalink / raw) To: SeongJae Park, Andrew Morton; +Cc: linux-mm, linux-kernel, damon, Kefeng Wang Omit one line by unified folio_put(), and make code more clear. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] mm/damon/paddr: minor refactor of damon_pa_mark_accessed_or_deactivate() 2023-03-03 8:43 ` [PATCH v2 3/3] mm/damon/paddr: minor refactor of damon_pa_mark_accessed_or_deactivate() Kefeng Wang @ 2023-03-03 18:26 ` SeongJae Park 2023-03-03 18:37 ` SeongJae Park 0 siblings, 1 reply; 15+ messages in thread From: SeongJae Park @ 2023-03-03 18:26 UTC (permalink / raw) To: Kefeng Wang; +Cc: SeongJae Park, Andrew Morton, linux-mm, linux-kernel, damon On Fri, 3 Mar 2023 16:43:43 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > Omit one line by unified folio_put(), and make code more clear. > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] mm/damon/paddr: minor refactor of damon_pa_mark_accessed_or_deactivate() 2023-03-03 18:26 ` SeongJae Park @ 2023-03-03 18:37 ` SeongJae Park 2023-03-06 1:12 ` Kefeng Wang 0 siblings, 1 reply; 15+ messages in thread From: SeongJae Park @ 2023-03-03 18:37 UTC (permalink / raw) To: SeongJae Park; +Cc: Kefeng Wang, Andrew Morton, linux-mm, linux-kernel, damon On Fri, 3 Mar 2023 18:26:33 +0000 SeongJae Park <sj@kernel.org> wrote: > On Fri, 3 Mar 2023 16:43:43 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > > Omit one line by unified folio_put(), and make code more clear. > > > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > > --- > > 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 > > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] mm/damon/paddr: minor refactor of damon_pa_mark_accessed_or_deactivate() 2023-03-03 18:37 ` SeongJae Park @ 2023-03-06 1:12 ` Kefeng Wang 0 siblings, 0 replies; 15+ messages in thread From: Kefeng Wang @ 2023-03-06 1:12 UTC (permalink / raw) To: SeongJae Park; +Cc: Andrew Morton, linux-mm, linux-kernel, damon On 2023/3/4 2:37, SeongJae Park wrote: > On Fri, 3 Mar 2023 18:26:33 +0000 SeongJae Park <sj@kernel.org> wrote: > >> On Fri, 3 Mar 2023 16:43:43 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >> >>> Omit one line by unified folio_put(), and make code more clear. >>> >>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>> --- >>> 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 >>> >>> ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-03-08 1:03 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-03 8:43 [PATCH v2 0/3] mm/damon/paddr: minor code improvement Kefeng Wang 2023-03-03 8:43 ` [PATCH v2 1/3] mm/damon/paddr: minor refactor of damon_pa_pageout() Kefeng Wang 2023-03-03 18:41 ` SeongJae Park 2023-03-03 8:43 ` [PATCH v2 2/3] mm/damon/paddr: minor refactor of damon_pa_young() Kefeng Wang 2023-03-03 18:39 ` SeongJae Park 2023-03-06 1:10 ` Kefeng Wang 2023-03-06 1:56 ` Kefeng Wang 2023-03-06 21:27 ` SeongJae Park 2023-03-07 1:22 ` Kefeng Wang 2023-03-07 18:00 ` SeongJae Park 2023-03-08 1:03 ` Kefeng Wang 2023-03-03 8:43 ` [PATCH v2 3/3] mm/damon/paddr: minor refactor of damon_pa_mark_accessed_or_deactivate() Kefeng Wang 2023-03-03 18:26 ` SeongJae Park 2023-03-03 18:37 ` SeongJae Park 2023-03-06 1:12 ` Kefeng Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.