| Message ID | 20220329132619.18689-3-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few cleanup and fixup patches for migration | expand |
Miaohe Lin <linmiaohe@huawei.com> writes: > can_split_huge_page is introduced via commit b8f593cd0896 ("mm, THP, swap: > check whether THP can be split firstly") to avoid deleting the THP from > the swap cache and freeing the swap cluster when the THP cannot be split. > But since commit bd4c82c22c36 ("mm, THP, swap: delay splitting THP after > swapped out"), splitting THP is delayed until THP is swapped out. There's > no need to delete the THP from the swap cache and free the swap cluster > anymore. Thus we can remove this unneeded can_split_huge_page check now to > simplify the code logic. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/vmscan.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 7c1a9713bfc9..09b452c4d256 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -1691,9 +1691,6 @@ static unsigned int shrink_page_list(struct list_head *page_list, > if (folio_maybe_dma_pinned(folio)) > goto keep_locked; > if (PageTransHuge(page)) { > - /* cannot split THP, skip it */ > - if (!can_split_folio(folio, NULL)) > - goto activate_locked; > /* > * Split pages without a PMD map right > * away. Chances are some or all of the I'm OK with the change itself. But THP still needs to be split after being swapped out. The reason we don't need to check can_split_folio() is that folio_maybe_dma_pinned() is checked before. Which will avoid the long term pinned pages to be swapped out. And we can live with short term pinned pages. Without can_split_folio() checking we can simplify the code as follows, /* * Split pages without a PMD map right * away. Chances are some or all of the * tail pages can be freed without IO. */ if (PageTransHuge(page) && !compound_mapcount(page) && split_huge_page_to_list(page, page_list)) goto keep_locked; activate_locked can be changed to keep_locked too, because it's just short term pinning. Best Regards, Huang, Ying
On 2022/3/31 16:38, Huang, Ying wrote: > Miaohe Lin <linmiaohe@huawei.com> writes: > >> can_split_huge_page is introduced via commit b8f593cd0896 ("mm, THP, swap: >> check whether THP can be split firstly") to avoid deleting the THP from >> the swap cache and freeing the swap cluster when the THP cannot be split. >> But since commit bd4c82c22c36 ("mm, THP, swap: delay splitting THP after >> swapped out"), splitting THP is delayed until THP is swapped out. There's >> no need to delete the THP from the swap cache and free the swap cluster >> anymore. Thus we can remove this unneeded can_split_huge_page check now to >> simplify the code logic. >> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> >> --- >> mm/vmscan.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/mm/vmscan.c b/mm/vmscan.c >> index 7c1a9713bfc9..09b452c4d256 100644 >> --- a/mm/vmscan.c >> +++ b/mm/vmscan.c >> @@ -1691,9 +1691,6 @@ static unsigned int shrink_page_list(struct list_head *page_list, >> if (folio_maybe_dma_pinned(folio)) >> goto keep_locked; >> if (PageTransHuge(page)) { >> - /* cannot split THP, skip it */ >> - if (!can_split_folio(folio, NULL)) >> - goto activate_locked; >> /* >> * Split pages without a PMD map right >> * away. Chances are some or all of the > > I'm OK with the change itself. But THP still needs to be split after > being swapped out. The reason we don't need to check can_split_folio() Could you please explain the relevant code path more detailed slightly? IIUC, if THP is swapped out, it will be freed via destroy_compound_page after __remove_mapping in shrink_page_list. So THP can be freed without split. Or am I miss something ? > is that folio_maybe_dma_pinned() is checked before. Which will avoid > the long term pinned pages to be swapped out. And we can live with > short term pinned pages. Without can_split_folio() checking we can > simplify the code as follows, > > /* > * Split pages without a PMD map right > * away. Chances are some or all of the > * tail pages can be freed without IO. > */ > if (PageTransHuge(page) && !compound_mapcount(page) && > split_huge_page_to_list(page, page_list)) > goto keep_locked; > > activate_locked can be changed to keep_locked too, because it's just > short term pinning. > The change above looks good to me. Many thanks. Should I add a Suggested-by tag for you? > Best Regards, > Huang, Ying Thanks. > > . >
diff --git a/mm/vmscan.c b/mm/vmscan.c index 7c1a9713bfc9..09b452c4d256 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -1691,9 +1691,6 @@ static unsigned int shrink_page_list(struct list_head *page_list, if (folio_maybe_dma_pinned(folio)) goto keep_locked; if (PageTransHuge(page)) { - /* cannot split THP, skip it */ - if (!can_split_folio(folio, NULL)) - goto activate_locked; /* * Split pages without a PMD map right * away. Chances are some or all of the
can_split_huge_page is introduced via commit b8f593cd0896 ("mm, THP, swap: check whether THP can be split firstly") to avoid deleting the THP from the swap cache and freeing the swap cluster when the THP cannot be split. But since commit bd4c82c22c36 ("mm, THP, swap: delay splitting THP after swapped out"), splitting THP is delayed until THP is swapped out. There's no need to delete the THP from the swap cache and free the swap cluster anymore. Thus we can remove this unneeded can_split_huge_page check now to simplify the code logic. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/vmscan.c | 3 --- 1 file changed, 3 deletions(-)