| Message ID | 20220329132619.18689-4-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few cleanup and fixup patches for migration | expand |
On Tue, Mar 29, 2022 at 09:26:14PM +0800, Miaohe Lin wrote: > -unsigned long reclaim_pages(struct list_head *page_list) > +static inline unsigned int reclaim_page_list(struct list_head *page_list, struct pglist_data *pgdat) > { > - int nid = NUMA_NO_NODE; > - unsigned int nr_reclaimed = 0; > - LIST_HEAD(node_page_list); > struct reclaim_stat dummy_stat; > + unsigned int nr_reclaimed; > struct page *page; > - unsigned int noreclaim_flag; > struct scan_control sc = { > .gfp_mask = GFP_KERNEL, > .may_writepage = 1, > @@ -2529,6 +2526,24 @@ unsigned long reclaim_pages(struct list_head *page_list) > .no_demotion = 1, > }; > > + nr_reclaimed = shrink_page_list(page_list, pgdat, &sc, &dummy_stat, false); > + while (!list_empty(page_list)) { > + page = lru_to_page(page_list); > + list_del(&page->lru); > + putback_lru_page(page); Why wouldn't you use a folio here?
> +static inline unsigned int reclaim_page_list(struct list_head *page_list, struct pglist_data *pgdat)
This is completely unreadable with the crazy long lines.
Also why would you force inline this?
On 2022/3/29 22:00, Christoph Hellwig wrote: >> +static inline unsigned int reclaim_page_list(struct list_head *page_list, struct pglist_data *pgdat) > > This is completely unreadable with the crazy long lines. Will break this long lines. > > Also why would you force inline this? It seems inline is indeed not needed. Will change this too. > . > Many thanks for your comment.
On 2022/3/29 21:50, Matthew Wilcox wrote: > On Tue, Mar 29, 2022 at 09:26:14PM +0800, Miaohe Lin wrote: >> -unsigned long reclaim_pages(struct list_head *page_list) >> +static inline unsigned int reclaim_page_list(struct list_head *page_list, struct pglist_data *pgdat) >> { >> - int nid = NUMA_NO_NODE; >> - unsigned int nr_reclaimed = 0; >> - LIST_HEAD(node_page_list); >> struct reclaim_stat dummy_stat; >> + unsigned int nr_reclaimed; >> struct page *page; >> - unsigned int noreclaim_flag; >> struct scan_control sc = { >> .gfp_mask = GFP_KERNEL, >> .may_writepage = 1, >> @@ -2529,6 +2526,24 @@ unsigned long reclaim_pages(struct list_head *page_list) >> .no_demotion = 1, >> }; >> >> + nr_reclaimed = shrink_page_list(page_list, pgdat, &sc, &dummy_stat, false); >> + while (!list_empty(page_list)) { >> + page = lru_to_page(page_list); >> + list_del(&page->lru); >> + putback_lru_page(page); > > Why wouldn't you use a folio here? > I was just wanting to keep the code consistent with the previous one. Am I supposed to use a folio here ? If so, will do it in V2. Thanks. > > . >
On Wed, Mar 30, 2022 at 10:04:54AM +0800, Miaohe Lin wrote: > On 2022/3/29 21:50, Matthew Wilcox wrote: > >> + while (!list_empty(page_list)) { > >> + page = lru_to_page(page_list); > >> + list_del(&page->lru); > >> + putback_lru_page(page); > > > > Why wouldn't you use a folio here? > > > > I was just wanting to keep the code consistent with the previous one. Am I supposed to > use a folio here ? If so, will do it in V2. The distinction we're trying to make (and obviously we have a long way to go before we're finished) is between the structure that describes PAGE_SIZE bytes of memory and the structure that manages the memory allocation. This function is clearly handling the entire memory allocation, so it should be using a folio. Eventually, page->lru should disappear, but there's 180 places to figure out what we need to do instead.
On 2022/3/31 0:23, Matthew Wilcox wrote: > On Wed, Mar 30, 2022 at 10:04:54AM +0800, Miaohe Lin wrote: >> On 2022/3/29 21:50, Matthew Wilcox wrote: >>>> + while (!list_empty(page_list)) { >>>> + page = lru_to_page(page_list); >>>> + list_del(&page->lru); >>>> + putback_lru_page(page); >>> >>> Why wouldn't you use a folio here? >>> >> >> I was just wanting to keep the code consistent with the previous one. Am I supposed to >> use a folio here ? If so, will do it in V2. > > The distinction we're trying to make (and obviously we have a long way > to go before we're finished) is between the structure that describes > PAGE_SIZE bytes of memory and the structure that manages the memory > allocation. This function is clearly handling the entire memory > allocation, so it should be using a folio. Eventually, page->lru I see. Thanks for clarifying. > should disappear, but there's 180 places to figure out what we need to > do instead. > > > . >
diff --git a/mm/vmscan.c b/mm/vmscan.c index 09b452c4d256..a6e60c78d058 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -2513,14 +2513,11 @@ static void shrink_active_list(unsigned long nr_to_scan, nr_deactivate, nr_rotated, sc->priority, file); } -unsigned long reclaim_pages(struct list_head *page_list) +static inline unsigned int reclaim_page_list(struct list_head *page_list, struct pglist_data *pgdat) { - int nid = NUMA_NO_NODE; - unsigned int nr_reclaimed = 0; - LIST_HEAD(node_page_list); struct reclaim_stat dummy_stat; + unsigned int nr_reclaimed; struct page *page; - unsigned int noreclaim_flag; struct scan_control sc = { .gfp_mask = GFP_KERNEL, .may_writepage = 1, @@ -2529,6 +2526,24 @@ unsigned long reclaim_pages(struct list_head *page_list) .no_demotion = 1, }; + nr_reclaimed = shrink_page_list(page_list, pgdat, &sc, &dummy_stat, false); + while (!list_empty(page_list)) { + page = lru_to_page(page_list); + list_del(&page->lru); + putback_lru_page(page); + } + + return nr_reclaimed; +} + +unsigned long reclaim_pages(struct list_head *page_list) +{ + int nid = NUMA_NO_NODE; + unsigned int nr_reclaimed = 0; + LIST_HEAD(node_page_list); + struct page *page; + unsigned int noreclaim_flag; + noreclaim_flag = memalloc_noreclaim_save(); while (!list_empty(page_list)) { @@ -2544,28 +2559,12 @@ unsigned long reclaim_pages(struct list_head *page_list) continue; } - nr_reclaimed += shrink_page_list(&node_page_list, - NODE_DATA(nid), - &sc, &dummy_stat, false); - while (!list_empty(&node_page_list)) { - page = lru_to_page(&node_page_list); - list_del(&page->lru); - putback_lru_page(page); - } - + nr_reclaimed += reclaim_page_list(&node_page_list, NODE_DATA(nid)); nid = NUMA_NO_NODE; } - if (!list_empty(&node_page_list)) { - nr_reclaimed += shrink_page_list(&node_page_list, - NODE_DATA(nid), - &sc, &dummy_stat, false); - while (!list_empty(&node_page_list)) { - page = lru_to_page(&node_page_list); - list_del(&page->lru); - putback_lru_page(page); - } - } + if (!list_empty(&node_page_list)) + nr_reclaimed += reclaim_page_list(&node_page_list, NODE_DATA(nid)); memalloc_noreclaim_restore(noreclaim_flag);
Introduce helper function reclaim_page_list() to eliminate the duplicated code of doing shrink_page_list() and putback_lru_page. Also We can separate node reclaim from node page list operation this way. No functional change intended. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/vmscan.c | 47 +++++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 24 deletions(-)