| Message ID | 20210710100329.49174-6-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanup and fixup for vmscan | expand |
On Sat 10-07-21 18:03:29, Miaohe Lin wrote: > We couldn't know whether the page is being freed elsewhere until we failed > to increase the page count. This is moving a hard to understand comment from one place to another. If anything this would benefit from what that elsewhere might be typically or simply drop the comment altogether. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/vmscan.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index a74760c48bd8..6e26b3c93242 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -1891,7 +1891,6 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan, > */ > scan += nr_pages; > if (!__isolate_lru_page_prepare(page, mode)) { > - /* It is being freed elsewhere */ > list_move(&page->lru, src); > continue; > } > @@ -1901,6 +1900,7 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan, > * page release code relies on it. > */ > if (unlikely(!get_page_unless_zero(page))) { > + /* It is being freed elsewhere. */ > list_move(&page->lru, src); > continue; > } > -- > 2.23.0
On 2021/7/12 15:28, Michal Hocko wrote: > On Sat 10-07-21 18:03:29, Miaohe Lin wrote: >> We couldn't know whether the page is being freed elsewhere until we failed >> to increase the page count. > > This is moving a hard to understand comment from one place to another. If get_page_unless_zero failed, the page could have been freed elsewhere. I think this looks straightforward but doesn't help a lot. Are you preferring to just remove this comment ? Thank you. > If anything this would benefit from what that elsewhere might be > typically or simply drop the comment altogether. > >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/vmscan.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/vmscan.c b/mm/vmscan.c >> index a74760c48bd8..6e26b3c93242 100644 >> --- a/mm/vmscan.c >> +++ b/mm/vmscan.c >> @@ -1891,7 +1891,6 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan, >> */ >> scan += nr_pages; >> if (!__isolate_lru_page_prepare(page, mode)) { >> - /* It is being freed elsewhere */ >> list_move(&page->lru, src); >> continue; >> } >> @@ -1901,6 +1900,7 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan, >> * page release code relies on it. >> */ >> if (unlikely(!get_page_unless_zero(page))) { >> + /* It is being freed elsewhere. */ >> list_move(&page->lru, src); >> continue; >> } >> -- >> 2.23.0 >
On Mon 12-07-21 19:16:47, Miaohe Lin wrote: > On 2021/7/12 15:28, Michal Hocko wrote: > > On Sat 10-07-21 18:03:29, Miaohe Lin wrote: > >> We couldn't know whether the page is being freed elsewhere until we failed > >> to increase the page count. > > > > This is moving a hard to understand comment from one place to another. > > If get_page_unless_zero failed, the page could have been freed elsewhere. I think > this looks straightforward but doesn't help a lot. Are you preferring to just > remove this comment ? Yes the comment in its current form is not really helpful much. Does it deserve a single liner to drop it? Likely not on its own without more changes in that area.
On 2021/7/13 17:32, Michal Hocko wrote: > On Mon 12-07-21 19:16:47, Miaohe Lin wrote: >> On 2021/7/12 15:28, Michal Hocko wrote: >>> On Sat 10-07-21 18:03:29, Miaohe Lin wrote: >>>> We couldn't know whether the page is being freed elsewhere until we failed >>>> to increase the page count. >>> >>> This is moving a hard to understand comment from one place to another. >> >> If get_page_unless_zero failed, the page could have been freed elsewhere. I think >> this looks straightforward but doesn't help a lot. Are you preferring to just >> remove this comment ? > > Yes the comment in its current form is not really helpful much. Does it > deserve a single liner to drop it? Likely not on its own without more > changes in that area. Sure, I will drop this single patch. And I would send a new patch when I collect enough misleading/obsolete comments to fix. Thanks. >
diff --git a/mm/vmscan.c b/mm/vmscan.c index a74760c48bd8..6e26b3c93242 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -1891,7 +1891,6 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan, */ scan += nr_pages; if (!__isolate_lru_page_prepare(page, mode)) { - /* It is being freed elsewhere */ list_move(&page->lru, src); continue; } @@ -1901,6 +1900,7 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan, * page release code relies on it. */ if (unlikely(!get_page_unless_zero(page))) { + /* It is being freed elsewhere. */ list_move(&page->lru, src); continue; }
We couldn't know whether the page is being freed elsewhere until we failed to increase the page count. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/vmscan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)