| Message ID | 20200918030051.650890-1-yuzhao@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | mm: clean up some lru related pieces | expand |
On Thu 17-09-20 21:00:38, Yu Zhao wrote: > Hi Andrew, > > I see you have taken this: > mm: use add_page_to_lru_list()/page_lru()/page_off_lru() > Do you mind dropping it? > > Michal asked to do a bit of additional work. So I thought I probably > should create a series to do more cleanups I've been meaning to. > > This series contains the change in the patch above and goes a few > more steps farther. It's intended to improve readability and should > not have any performance impacts. There are minor behavior changes in > terms of debugging and error reporting, which I have all highlighted > in the individual patches. All patches were properly tested on 5.8 > running Chrome OS, with various debug options turned on. > > Michal, > > Do you mind taking a looking at the entire series? I have stopped at patch 3 as all patches until then are really missing any justification. What is the point for all this to be done? The code is far from trivial and just shifting around sounds like a risk. You are removing ~50 LOC which is always nice but I am not sure the resulting code is better maintainble or easier to read and understand. Just consider __ClearPageLRU moving to page_off_lru patch. What is the additional value of having the flag moved and burry it into a function to have even more side effects? I found the way how __ClearPageLRU is nicely close to removing it from LRU easier to follow. This is likely subjective and other might think differently but as it is not clear what is your actual goal here it is hard to judge pros and cons.
On Fri, Sep 18, 2020 at 09:45:49AM +0200, Michal Hocko wrote: > On Thu 17-09-20 21:00:38, Yu Zhao wrote: > > Hi Andrew, > > > > I see you have taken this: > > mm: use add_page_to_lru_list()/page_lru()/page_off_lru() > > Do you mind dropping it? > > > > Michal asked to do a bit of additional work. So I thought I probably > > should create a series to do more cleanups I've been meaning to. > > > > This series contains the change in the patch above and goes a few > > more steps farther. It's intended to improve readability and should > > not have any performance impacts. There are minor behavior changes in > > terms of debugging and error reporting, which I have all highlighted > > in the individual patches. All patches were properly tested on 5.8 > > running Chrome OS, with various debug options turned on. > > > > Michal, > > > > Do you mind taking a looking at the entire series? > > I have stopped at patch 3 as all patches until then are really missing > any justification. What is the point for all this to be done? The code > is far from trivial and just shifting around sounds like a risk. You are I appreciate your caution, and if you let me know what exactly your concerns are, we could probably work them out together. > removing ~50 LOC which is always nice but I am not sure the resulting > code is better maintainble or easier to read and understand. Just > consider __ClearPageLRU moving to page_off_lru patch. What is the > additional value of having the flag moved and burry it into a function > to have even more side effects? I found the way how __ClearPageLRU is Mind elaborating the side effects? > nicely close to removing it from LRU easier to follow. This is likely > subjective and other might think differently but as it is not clear what > is your actual goal here it is hard to judge pros and cons. I like this specific example from patch 3. Here is what it does: we have three places using the same boilerplate, i.e., page_off_lru() + __ClearPageLRU(), the patch moves __ClearPageLRU() into page_off_lru(), which already does __ClearPageActive() and __ClearPageUnevictable(). Later on, we rename page_off_lru() to __clear_page_lru_flags() (patch 8). Its point seems quite clear to me. Why would *anybody* want to use two helper functions *repeatedly* when the job can be done with just one? Nobody is paid by the number of lines they add, right? :) And for that matter, why would anybody want any boilerplate to be open coded from the same group of helper functions arranged in various ways? I don't think the answer is subjective, but I don't expect everybody to agree with me. Now back to your general question: what's the point of this series? Readability -- less error prone and easier to maintain. This series consolidate open-coded boilerplate like the following in many places. Take lru_lazyfree_fn() as an example: - bool active = PageActive(page); int nr_pages = thp_nr_pages(page); - del_page_from_lru_list(page, lruvec, - LRU_INACTIVE_ANON + active); + del_page_from_lru_list(page, lruvec); ClearPageActive(page); ClearPageReferenced(page); <snipped> ClearPageSwapBacked(page); - add_page_to_lru_list(page, lruvec, LRU_INACTIVE_FILE); + add_page_to_lru_list(page, lruvec); I hope this helps, but if it doesn't, I'd be more than happy to have more discussions on the details. And not that I don't appreciate your review, but please be more specific than 'sounds like a risk' or 'have even more side effects' so I can address your concerns effectively.
On Thu, 17 Sep 2020, Yu Zhao wrote: > Hi Andrew, > > I see you have taken this: > mm: use add_page_to_lru_list()/page_lru()/page_off_lru() > Do you mind dropping it? > > Michal asked to do a bit of additional work. So I thought I probably > should create a series to do more cleanups I've been meaning to. > > This series contains the change in the patch above and goes a few > more steps farther. It's intended to improve readability and should > not have any performance impacts. There are minor behavior changes in > terms of debugging and error reporting, which I have all highlighted > in the individual patches. All patches were properly tested on 5.8 > running Chrome OS, with various debug options turned on. > > Michal, > > Do you mind taking a looking at the entire series? > > Thank you. > > Yu Zhao (13): > mm: use add_page_to_lru_list() > mm: use page_off_lru() > mm: move __ClearPageLRU() into page_off_lru() > mm: shuffle lru list addition and deletion functions > mm: don't pass enum lru_list to lru list addition functions > mm: don't pass enum lru_list to trace_mm_lru_insertion() > mm: don't pass enum lru_list to del_page_from_lru_list() > mm: rename page_off_lru() to __clear_page_lru_flags() > mm: inline page_lru_base_type() > mm: VM_BUG_ON lru page flags > mm: inline __update_lru_size() > mm: make lruvec_lru_size() static > mm: enlarge the int parameter of update_lru_size() > > include/linux/memcontrol.h | 14 ++-- > include/linux/mm_inline.h | 115 ++++++++++++++------------------- > include/linux/mmzone.h | 2 - > include/linux/vmstat.h | 2 +- > include/trace/events/pagemap.h | 11 ++-- > mm/compaction.c | 2 +- > mm/memcontrol.c | 10 +-- > mm/mlock.c | 2 +- > mm/swap.c | 53 ++++++--------- > mm/vmscan.c | 28 +++----- > 10 files changed, 95 insertions(+), 144 deletions(-) > > -- > 2.28.0.681.g6f77f65b4e-goog Sorry, Yu, I may be out-of-line in sending this: but as you know, Alex Shi has a long per-memcg lru_lock series playing in much the same area (particularly conflicting in mm/swap.c and mm/vmscan.c): a patchset that makes useful changes, that I'm very keen to help into mmotm a.s.a.p (but not before I've completed diligence). We've put a lot of effort into its testing, I'm currently reviewing it patch by patch (my general silence indicating that I'm busy on that, but slow as ever): so I'm a bit discouraged to have its stability potentially undermined by conflicting cleanups at this stage. If there's general agreement that your cleanups are safe and welcome (Michal's initial reaction sheds some doubt on that), great: I hope that Andrew can fast-track them into mmotm, then Alex rebase on top of them, and I then re-test and re-review. But if that quick agreement is not forthcoming, may I ask you please to hold back, and resend based on top of Alex's next posting? Thanks, Hugh
On Fri, Sep 18, 2020 at 01:46:59PM -0700, Hugh Dickins wrote: > On Thu, 17 Sep 2020, Yu Zhao wrote: > > > Hi Andrew, > > > > I see you have taken this: > > mm: use add_page_to_lru_list()/page_lru()/page_off_lru() > > Do you mind dropping it? > > > > Michal asked to do a bit of additional work. So I thought I probably > > should create a series to do more cleanups I've been meaning to. > > > > This series contains the change in the patch above and goes a few > > more steps farther. It's intended to improve readability and should > > not have any performance impacts. There are minor behavior changes in > > terms of debugging and error reporting, which I have all highlighted > > in the individual patches. All patches were properly tested on 5.8 > > running Chrome OS, with various debug options turned on. > > > > Michal, > > > > Do you mind taking a looking at the entire series? > > > > Thank you. > > > > Yu Zhao (13): > > mm: use add_page_to_lru_list() > > mm: use page_off_lru() > > mm: move __ClearPageLRU() into page_off_lru() > > mm: shuffle lru list addition and deletion functions > > mm: don't pass enum lru_list to lru list addition functions > > mm: don't pass enum lru_list to trace_mm_lru_insertion() > > mm: don't pass enum lru_list to del_page_from_lru_list() > > mm: rename page_off_lru() to __clear_page_lru_flags() > > mm: inline page_lru_base_type() > > mm: VM_BUG_ON lru page flags > > mm: inline __update_lru_size() > > mm: make lruvec_lru_size() static > > mm: enlarge the int parameter of update_lru_size() > > > > include/linux/memcontrol.h | 14 ++-- > > include/linux/mm_inline.h | 115 ++++++++++++++------------------- > > include/linux/mmzone.h | 2 - > > include/linux/vmstat.h | 2 +- > > include/trace/events/pagemap.h | 11 ++-- > > mm/compaction.c | 2 +- > > mm/memcontrol.c | 10 +-- > > mm/mlock.c | 2 +- > > mm/swap.c | 53 ++++++--------- > > mm/vmscan.c | 28 +++----- > > 10 files changed, 95 insertions(+), 144 deletions(-) > > > > -- > > 2.28.0.681.g6f77f65b4e-goog > > Sorry, Yu, I may be out-of-line in sending this: but as you know, > Alex Shi has a long per-memcg lru_lock series playing in much the > same area (particularly conflicting in mm/swap.c and mm/vmscan.c): > a patchset that makes useful changes, that I'm very keen to help > into mmotm a.s.a.p (but not before I've completed diligence). > > We've put a lot of effort into its testing, I'm currently reviewing > it patch by patch (my general silence indicating that I'm busy on that, > but slow as ever): so I'm a bit discouraged to have its stability > potentially undermined by conflicting cleanups at this stage. > > If there's general agreement that your cleanups are safe and welcome > (Michal's initial reaction sheds some doubt on that), great: I hope > that Andrew can fast-track them into mmotm, then Alex rebase on top > of them, and I then re-test and re-review. > > But if that quick agreement is not forthcoming, may I ask you please > to hold back, and resend based on top of Alex's next posting? The per-memcg lru lock series seems a high priority, and I have absolutely no problem accommodate your request. In return, may I ask you or Alex to review this series after you have finished with per-memcg lru lock (to make sure that I resolve all the conflicts correctly at least)?
On Fri, 18 Sep 2020, Yu Zhao wrote: > On Fri, Sep 18, 2020 at 01:46:59PM -0700, Hugh Dickins wrote: > > On Thu, 17 Sep 2020, Yu Zhao wrote: > > > > > Hi Andrew, > > > > > > I see you have taken this: > > > mm: use add_page_to_lru_list()/page_lru()/page_off_lru() > > > Do you mind dropping it? > > > > > > Michal asked to do a bit of additional work. So I thought I probably > > > should create a series to do more cleanups I've been meaning to. > > > > > > This series contains the change in the patch above and goes a few > > > more steps farther. It's intended to improve readability and should > > > not have any performance impacts. There are minor behavior changes in > > > terms of debugging and error reporting, which I have all highlighted > > > in the individual patches. All patches were properly tested on 5.8 > > > running Chrome OS, with various debug options turned on. > > > > > > Michal, > > > > > > Do you mind taking a looking at the entire series? > > > > > > Thank you. > > > > > > Yu Zhao (13): > > > mm: use add_page_to_lru_list() > > > mm: use page_off_lru() > > > mm: move __ClearPageLRU() into page_off_lru() > > > mm: shuffle lru list addition and deletion functions > > > mm: don't pass enum lru_list to lru list addition functions > > > mm: don't pass enum lru_list to trace_mm_lru_insertion() > > > mm: don't pass enum lru_list to del_page_from_lru_list() > > > mm: rename page_off_lru() to __clear_page_lru_flags() > > > mm: inline page_lru_base_type() > > > mm: VM_BUG_ON lru page flags > > > mm: inline __update_lru_size() > > > mm: make lruvec_lru_size() static > > > mm: enlarge the int parameter of update_lru_size() > > > > > > include/linux/memcontrol.h | 14 ++-- > > > include/linux/mm_inline.h | 115 ++++++++++++++------------------- > > > include/linux/mmzone.h | 2 - > > > include/linux/vmstat.h | 2 +- > > > include/trace/events/pagemap.h | 11 ++-- > > > mm/compaction.c | 2 +- > > > mm/memcontrol.c | 10 +-- > > > mm/mlock.c | 2 +- > > > mm/swap.c | 53 ++++++--------- > > > mm/vmscan.c | 28 +++----- > > > 10 files changed, 95 insertions(+), 144 deletions(-) > > > > > > -- > > > 2.28.0.681.g6f77f65b4e-goog > > > > Sorry, Yu, I may be out-of-line in sending this: but as you know, > > Alex Shi has a long per-memcg lru_lock series playing in much the > > same area (particularly conflicting in mm/swap.c and mm/vmscan.c): > > a patchset that makes useful changes, that I'm very keen to help > > into mmotm a.s.a.p (but not before I've completed diligence). > > > > We've put a lot of effort into its testing, I'm currently reviewing > > it patch by patch (my general silence indicating that I'm busy on that, > > but slow as ever): so I'm a bit discouraged to have its stability > > potentially undermined by conflicting cleanups at this stage. > > > > If there's general agreement that your cleanups are safe and welcome > > (Michal's initial reaction sheds some doubt on that), great: I hope > > that Andrew can fast-track them into mmotm, then Alex rebase on top > > of them, and I then re-test and re-review. > > > > But if that quick agreement is not forthcoming, may I ask you please > > to hold back, and resend based on top of Alex's next posting? > > The per-memcg lru lock series seems a high priority, and I have > absolutely no problem accommodate your request. Many thanks! > > In return, may I ask you or Alex to review this series after you > have finished with per-memcg lru lock (to make sure that I resolve > all the conflicts correctly at least)? Fair enough: I promise to do so. And your rebasing will necessarily lead you to review some parts of Alex's patchset, which will help us all too. Andrew, Yu asked at the start: > > > I see you have taken this: > > > mm: use add_page_to_lru_list()/page_lru()/page_off_lru() > > > Do you mind dropping it? Dropping that for now will help too. Thanks, Hugh
在 2020/9/19 上午5:19, Hugh Dickins 写道: >>>> 2.28.0.681.g6f77f65b4e-goog >>> Sorry, Yu, I may be out-of-line in sending this: but as you know, >>> Alex Shi has a long per-memcg lru_lock series playing in much the >>> same area (particularly conflicting in mm/swap.c and mm/vmscan.c): >>> a patchset that makes useful changes, that I'm very keen to help >>> into mmotm a.s.a.p (but not before I've completed diligence). >>> >>> We've put a lot of effort into its testing, I'm currently reviewing >>> it patch by patch (my general silence indicating that I'm busy on that, >>> but slow as ever): so I'm a bit discouraged to have its stability >>> potentially undermined by conflicting cleanups at this stage. >>> >>> If there's general agreement that your cleanups are safe and welcome >>> (Michal's initial reaction sheds some doubt on that), great: I hope >>> that Andrew can fast-track them into mmotm, then Alex rebase on top >>> of them, and I then re-test and re-review. >>> >>> But if that quick agreement is not forthcoming, may I ask you please >>> to hold back, and resend based on top of Alex's next posting? >> The per-memcg lru lock series seems a high priority, and I have >> absolutely no problem accommodate your request. > Many thanks! > >> In return, may I ask you or Alex to review this series after you >> have finished with per-memcg lru lock (to make sure that I resolve >> all the conflicts correctly at least)? > Fair enough: I promise to do so. > > And your rebasing will necessarily lead you to review some parts > of Alex's patchset, which will help us all too. > > Andrew, Yu asked at the start: >>>> I see you have taken this: >>>> mm: use add_page_to_lru_list()/page_lru()/page_off_lru() >>>> Do you mind dropping it? > Dropping that for now will help too. Hi Hugh & Yu, Thanks for all your considerations! I will looking into this series after thing on lru_lock finished. Thanks a lot! Alex
On Fri, 18 Sep 2020, Hugh Dickins wrote: > On Fri, 18 Sep 2020, Yu Zhao wrote: > > On Fri, Sep 18, 2020 at 01:46:59PM -0700, Hugh Dickins wrote: > > > On Thu, 17 Sep 2020, Yu Zhao wrote: > > > > > > > Hi Andrew, > > > > > > > > I see you have taken this: > > > > mm: use add_page_to_lru_list()/page_lru()/page_off_lru() > > > > Do you mind dropping it? > > > > > > > > Michal asked to do a bit of additional work. So I thought I probably > > > > should create a series to do more cleanups I've been meaning to. > > > > > > > > This series contains the change in the patch above and goes a few > > > > more steps farther. It's intended to improve readability and should > > > > not have any performance impacts. There are minor behavior changes in > > > > terms of debugging and error reporting, which I have all highlighted > > > > in the individual patches. All patches were properly tested on 5.8 > > > > running Chrome OS, with various debug options turned on. > > > > > > > > Michal, > > > > > > > > Do you mind taking a looking at the entire series? > > > > > > > > Thank you. > > > > > > > > Yu Zhao (13): > > > > mm: use add_page_to_lru_list() > > > > mm: use page_off_lru() > > > > mm: move __ClearPageLRU() into page_off_lru() > > > > mm: shuffle lru list addition and deletion functions > > > > mm: don't pass enum lru_list to lru list addition functions > > > > mm: don't pass enum lru_list to trace_mm_lru_insertion() > > > > mm: don't pass enum lru_list to del_page_from_lru_list() > > > > mm: rename page_off_lru() to __clear_page_lru_flags() > > > > mm: inline page_lru_base_type() > > > > mm: VM_BUG_ON lru page flags > > > > mm: inline __update_lru_size() > > > > mm: make lruvec_lru_size() static > > > > mm: enlarge the int parameter of update_lru_size() > > > > > > > > include/linux/memcontrol.h | 14 ++-- > > > > include/linux/mm_inline.h | 115 ++++++++++++++------------------- > > > > include/linux/mmzone.h | 2 - > > > > include/linux/vmstat.h | 2 +- > > > > include/trace/events/pagemap.h | 11 ++-- > > > > mm/compaction.c | 2 +- > > > > mm/memcontrol.c | 10 +-- > > > > mm/mlock.c | 2 +- > > > > mm/swap.c | 53 ++++++--------- > > > > mm/vmscan.c | 28 +++----- > > > > 10 files changed, 95 insertions(+), 144 deletions(-) > > > > > > > > -- > > > > 2.28.0.681.g6f77f65b4e-goog > > > > > > Sorry, Yu, I may be out-of-line in sending this: but as you know, > > > Alex Shi has a long per-memcg lru_lock series playing in much the > > > same area (particularly conflicting in mm/swap.c and mm/vmscan.c): > > > a patchset that makes useful changes, that I'm very keen to help > > > into mmotm a.s.a.p (but not before I've completed diligence). > > > > > > We've put a lot of effort into its testing, I'm currently reviewing > > > it patch by patch (my general silence indicating that I'm busy on that, > > > but slow as ever): so I'm a bit discouraged to have its stability > > > potentially undermined by conflicting cleanups at this stage. > > > > > > If there's general agreement that your cleanups are safe and welcome > > > (Michal's initial reaction sheds some doubt on that), great: I hope > > > that Andrew can fast-track them into mmotm, then Alex rebase on top > > > of them, and I then re-test and re-review. > > > > > > But if that quick agreement is not forthcoming, may I ask you please > > > to hold back, and resend based on top of Alex's next posting? > > > > The per-memcg lru lock series seems a high priority, and I have > > absolutely no problem accommodate your request. > > Many thanks! > > > > > In return, may I ask you or Alex to review this series after you > > have finished with per-memcg lru lock (to make sure that I resolve > > all the conflicts correctly at least)? > > Fair enough: I promise to do so. > > And your rebasing will necessarily lead you to review some parts > of Alex's patchset, which will help us all too. > > Andrew, Yu asked at the start: > > > > I see you have taken this: > > > > mm: use add_page_to_lru_list()/page_lru()/page_off_lru() > > > > Do you mind dropping it? > Dropping that for now will help too. Andrew, please drop Yu Zhao's mm-use-add_page_to_lru_list-page_lru-page_off_lru.patch from the mmotm tree. Yu wants to replace it by this cleanup series, and he has graciously agreed to rebase his series on top of Alex Shi's per-memcg lru_lock series; but mm-use-add_page_to_lru_list-page_lru-page_off_lru.patch is getting in the way of adding them to mmotm. The three of us are all hoping that Alex's v19 series can go into mmotm when the merge window closes, then I'll review Yu's series rebased on top of it. Thanks, Hugh