| Message ID | 20200918030051.650890-3-yuzhao@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: clean up some lru related pieces | expand |
On Thu 17-09-20 21:00:40, Yu Zhao wrote: > This patch replaces the only open-coded __ClearPageActive() with > page_off_lru(). There is no open-coded __ClearPageUnevictable()s. > > Before this patch, we have: > __ClearPageActive() > add_page_to_lru_list() > > After this patch, we have: > page_off_lru() > if PageUnevictable() > __ClearPageUnevictable() > else if PageActive() > __ClearPageActive() > add_page_to_lru_list() > > Checking PageUnevictable() shouldn't be a problem because these two > flags are mutually exclusive. Leaking either will trigger bad_page(). I am sorry but the changelog is really hard to grasp. What are you trying to achieve, why and why it is safe. This should be a general outline for any patch. I have already commented on the previous patch and asked you for the explanation why removing __ClearPageActive from this path is desirable and safe. I have specifically asked to clarify the compound page situation as that is using its oen destructor in the freeing path and that might result in page_off_lru to be not called. > Signed-off-by: Yu Zhao <yuzhao@google.com> > --- > mm/vmscan.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 503fc5e1fe32..f257d2f61574 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -1845,7 +1845,6 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec, > int nr_pages, nr_moved = 0; > LIST_HEAD(pages_to_free); > struct page *page; > - enum lru_list lru; > > while (!list_empty(list)) { > page = lru_to_page(list); > @@ -1860,14 +1859,11 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec, > lruvec = mem_cgroup_page_lruvec(page, pgdat); > > SetPageLRU(page); > - lru = page_lru(page); > - > add_page_to_lru_list(page, lruvec, lru); > > if (put_page_testzero(page)) { > __ClearPageLRU(page); > - __ClearPageActive(page); > - del_page_from_lru_list(page, lruvec, lru); > + del_page_from_lru_list(page, lruvec, page_off_lru(page)); > > if (unlikely(PageCompound(page))) { > spin_unlock_irq(&pgdat->lru_lock); > -- > 2.28.0.681.g6f77f65b4e-goog
On Fri, Sep 18, 2020 at 09:37:00AM +0200, Michal Hocko wrote: > On Thu 17-09-20 21:00:40, Yu Zhao wrote: > > This patch replaces the only open-coded __ClearPageActive() with > > page_off_lru(). There is no open-coded __ClearPageUnevictable()s. > > > > Before this patch, we have: > > __ClearPageActive() > > add_page_to_lru_list() > > > > After this patch, we have: > > page_off_lru() > > if PageUnevictable() > > __ClearPageUnevictable() > > else if PageActive() > > __ClearPageActive() > > add_page_to_lru_list() > > > > Checking PageUnevictable() shouldn't be a problem because these two > > flags are mutually exclusive. Leaking either will trigger bad_page(). > > I am sorry but the changelog is really hard to grasp. What are you > trying to achieve, why and why it is safe. This should be a general > outline for any patch. I have already commented on the previous patch > and asked you for the explanation why removing __ClearPageActive from > this path is desirable and safe. I have specifically asked to clarify > the compound page situation as that is using its oen destructor in the > freeing path and that might result in page_off_lru to be not called. Haven't I explained we are NOT removing __ClearPageActive()? Is my notion of the code structure above confusing you? Or 'open-coded' could mean different things? And I have asked this before: why does 'the compound page situation' even matter here? Perhaps if you could give a concrete example related to the code change and help me understand your concern? > > Signed-off-by: Yu Zhao <yuzhao@google.com> > > --- > > mm/vmscan.c | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > index 503fc5e1fe32..f257d2f61574 100644 > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -1845,7 +1845,6 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec, > > int nr_pages, nr_moved = 0; > > LIST_HEAD(pages_to_free); > > struct page *page; > > - enum lru_list lru; > > > > while (!list_empty(list)) { > > page = lru_to_page(list); > > @@ -1860,14 +1859,11 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec, > > lruvec = mem_cgroup_page_lruvec(page, pgdat); > > > > SetPageLRU(page); > > - lru = page_lru(page); > > - > > add_page_to_lru_list(page, lruvec, lru); > > > > if (put_page_testzero(page)) { > > __ClearPageLRU(page); > > - __ClearPageActive(page); > > - del_page_from_lru_list(page, lruvec, lru); > > + del_page_from_lru_list(page, lruvec, page_off_lru(page)); > > > > if (unlikely(PageCompound(page))) { > > spin_unlock_irq(&pgdat->lru_lock); > > -- > > 2.28.0.681.g6f77f65b4e-goog > > -- > Michal Hocko > SUSE Labs
On Fri 18-09-20 04:27:13, Yu Zhao wrote: > On Fri, Sep 18, 2020 at 09:37:00AM +0200, Michal Hocko wrote: > > On Thu 17-09-20 21:00:40, Yu Zhao wrote: > > > This patch replaces the only open-coded __ClearPageActive() with > > > page_off_lru(). There is no open-coded __ClearPageUnevictable()s. > > > > > > Before this patch, we have: > > > __ClearPageActive() > > > add_page_to_lru_list() > > > > > > After this patch, we have: > > > page_off_lru() > > > if PageUnevictable() > > > __ClearPageUnevictable() > > > else if PageActive() > > > __ClearPageActive() > > > add_page_to_lru_list() > > > > > > Checking PageUnevictable() shouldn't be a problem because these two > > > flags are mutually exclusive. Leaking either will trigger bad_page(). > > > > I am sorry but the changelog is really hard to grasp. What are you > > trying to achieve, why and why it is safe. This should be a general > > outline for any patch. I have already commented on the previous patch > > and asked you for the explanation why removing __ClearPageActive from > > this path is desirable and safe. I have specifically asked to clarify > > the compound page situation as that is using its oen destructor in the > > freeing path and that might result in page_off_lru to be not called. > > Haven't I explained we are NOT removing __ClearPageActive()? Is my > notion of the code structure above confusing you? Or 'open-coded' > could mean different things? Please read through my reply carefuly. I am not saying what you are doing is wrong. I am expressing a lack of justification which is the case throughout this patch series. You do not explain why we need it and why reviewers should spend time on this. Because the review is not as trivial as looking at the diff.
On Fri 18-09-20 04:27:13, Yu Zhao wrote: > On Fri, Sep 18, 2020 at 09:37:00AM +0200, Michal Hocko wrote: [...] > And I have asked this before: why does 'the compound page situation' > even matter here? Perhaps if you could give a concrete example related > to the code change and help me understand your concern? Forgot to answer this part. The compound page concern is a misreading of the patch on my end. I have missed you are using page_off_lru in this (move_pages_to_lru) path and that you rely on release_pages to do the clean up on you. I still find it rather awkward that page_off_lru has such side effects but I am not deeply familiar with the reasoning behind so I will rather shut up now. [...] > > > @@ -1860,14 +1859,11 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec, > > > lruvec = mem_cgroup_page_lruvec(page, pgdat); > > > > > > SetPageLRU(page); > > > - lru = page_lru(page); > > > - > > > add_page_to_lru_list(page, lruvec, lru); > > > > > > if (put_page_testzero(page)) { > > > __ClearPageLRU(page); > > > - __ClearPageActive(page); > > > - del_page_from_lru_list(page, lruvec, lru); > > > + del_page_from_lru_list(page, lruvec, page_off_lru(page)); > > > > > > if (unlikely(PageCompound(page))) { > > > spin_unlock_irq(&pgdat->lru_lock); > > > -- > > > 2.28.0.681.g6f77f65b4e-goog > > > > -- > > Michal Hocko > > SUSE Labs
On Fri, Sep 18, 2020 at 01:09:14PM +0200, Michal Hocko wrote: > On Fri 18-09-20 04:27:13, Yu Zhao wrote: > > On Fri, Sep 18, 2020 at 09:37:00AM +0200, Michal Hocko wrote: > > > On Thu 17-09-20 21:00:40, Yu Zhao wrote: > > > > This patch replaces the only open-coded __ClearPageActive() with > > > > page_off_lru(). There is no open-coded __ClearPageUnevictable()s. > > > > > > > > Before this patch, we have: > > > > __ClearPageActive() > > > > add_page_to_lru_list() > > > > > > > > After this patch, we have: > > > > page_off_lru() > > > > if PageUnevictable() > > > > __ClearPageUnevictable() > > > > else if PageActive() > > > > __ClearPageActive() > > > > add_page_to_lru_list() > > > > > > > > Checking PageUnevictable() shouldn't be a problem because these two > > > > flags are mutually exclusive. Leaking either will trigger bad_page(). > > > > > > I am sorry but the changelog is really hard to grasp. What are you > > > trying to achieve, why and why it is safe. This should be a general > > > outline for any patch. I have already commented on the previous patch > > > and asked you for the explanation why removing __ClearPageActive from > > > this path is desirable and safe. I have specifically asked to clarify > > > the compound page situation as that is using its oen destructor in the > > > freeing path and that might result in page_off_lru to be not called. > > > > Haven't I explained we are NOT removing __ClearPageActive()? Is my > > notion of the code structure above confusing you? Or 'open-coded' > > could mean different things? > > Please read through my reply carefuly. I am not saying what you are > doing is wrong. I am expressing a lack of justification which is the > case throughout this patch series. You do not explain why we need it and > why reviewers should spend time on this. Because the review is not as > trivial as looking at the diff. I appreciate your time. But if you are looking for some grand justification, I'm afraid I won't be able to give one, because, as it's titled, this is just a series of cleanup patches. My questions above are meant to determine which parts are not clear. Well, I still don't know. So let's try this. What's your impression upon reading the first sentence of this patch? > > > > This patch replaces the only open-coded __ClearPageActive() with > > > > page_off_lru(). Here is how I would think when I read it (which is purely subjective since I wrote it): 'replaces the only (an outlier) open-coded (bad) with page_off_lru() (the norm)' It seems to me it has everything I need to know (or say). Of course I could spell them all out for you if that's how you'd prefer. And if it's not enough, then please show me some examples and I'll study them carefully and try my best to follow them.
On Fri 18-09-20 12:53:58, Yu Zhao wrote: > On Fri, Sep 18, 2020 at 01:09:14PM +0200, Michal Hocko wrote: > > On Fri 18-09-20 04:27:13, Yu Zhao wrote: > > > On Fri, Sep 18, 2020 at 09:37:00AM +0200, Michal Hocko wrote: > > > > On Thu 17-09-20 21:00:40, Yu Zhao wrote: > > > > > This patch replaces the only open-coded __ClearPageActive() with > > > > > page_off_lru(). There is no open-coded __ClearPageUnevictable()s. > > > > > > > > > > Before this patch, we have: > > > > > __ClearPageActive() > > > > > add_page_to_lru_list() > > > > > > > > > > After this patch, we have: > > > > > page_off_lru() > > > > > if PageUnevictable() > > > > > __ClearPageUnevictable() > > > > > else if PageActive() > > > > > __ClearPageActive() > > > > > add_page_to_lru_list() > > > > > > > > > > Checking PageUnevictable() shouldn't be a problem because these two > > > > > flags are mutually exclusive. Leaking either will trigger bad_page(). > > > > > > > > I am sorry but the changelog is really hard to grasp. What are you > > > > trying to achieve, why and why it is safe. This should be a general > > > > outline for any patch. I have already commented on the previous patch > > > > and asked you for the explanation why removing __ClearPageActive from > > > > this path is desirable and safe. I have specifically asked to clarify > > > > the compound page situation as that is using its oen destructor in the > > > > freeing path and that might result in page_off_lru to be not called. > > > > > > Haven't I explained we are NOT removing __ClearPageActive()? Is my > > > notion of the code structure above confusing you? Or 'open-coded' > > > could mean different things? > > > > Please read through my reply carefuly. I am not saying what you are > > doing is wrong. I am expressing a lack of justification which is the > > case throughout this patch series. You do not explain why we need it and > > why reviewers should spend time on this. Because the review is not as > > trivial as looking at the diff. > > I appreciate your time. But if you are looking for some grand > justification, I'm afraid I won't be able to give one, because, as it's > titled, this is just a series of cleanup patches. You likely had some reason to do that clean up, right? What was that? An inconcistency in handling some of the page flags when it is moved around LRU lists? Was the code too hard to reason about? Was it error prone? Please do not take me wrong, I am not trying to discourage you from clean up work. There is a lot of code that would benefit from clean ups. But it certainly helps to outline your motivation and the goal you would like to achieve. Without that it would boil down to guessing what you might have thought or simly moving things around without a very good long term reason. Thanks!
diff --git a/mm/vmscan.c b/mm/vmscan.c index 503fc5e1fe32..f257d2f61574 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -1845,7 +1845,6 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec, int nr_pages, nr_moved = 0; LIST_HEAD(pages_to_free); struct page *page; - enum lru_list lru; while (!list_empty(list)) { page = lru_to_page(list); @@ -1860,14 +1859,11 @@ static unsigned noinline_for_stack move_pages_to_lru(struct lruvec *lruvec, lruvec = mem_cgroup_page_lruvec(page, pgdat); SetPageLRU(page); - lru = page_lru(page); - add_page_to_lru_list(page, lruvec, lru); if (put_page_testzero(page)) { __ClearPageLRU(page); - __ClearPageActive(page); - del_page_from_lru_list(page, lruvec, lru); + del_page_from_lru_list(page, lruvec, page_off_lru(page)); if (unlikely(PageCompound(page))) { spin_unlock_irq(&pgdat->lru_lock);
This patch replaces the only open-coded __ClearPageActive() with page_off_lru(). There is no open-coded __ClearPageUnevictable()s. Before this patch, we have: __ClearPageActive() add_page_to_lru_list() After this patch, we have: page_off_lru() if PageUnevictable() __ClearPageUnevictable() else if PageActive() __ClearPageActive() add_page_to_lru_list() Checking PageUnevictable() shouldn't be a problem because these two flags are mutually exclusive. Leaking either will trigger bad_page(). Signed-off-by: Yu Zhao <yuzhao@google.com> --- mm/vmscan.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)