| Message ID | 20230303084343.171958-4-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/damon/paddr: minor code improvement | expand |
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 > >
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 > > > >
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 >>> >>>
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; }
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(-)