| Message ID | 20221229122503.149083-1-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: huge_memory: convert split_huge_pages_all() to use a folio | expand |
On Thu, 29 Dec 2022 20:25:03 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > Straightforwardly convert split_huge_pages_all() to use a folio. > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > mm/huge_memory.c | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 266c4b557946..c8cbe7f62eaa 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -2932,6 +2932,7 @@ static void split_huge_pages_all(void) > { > struct zone *zone; > struct page *page; > + struct folio *folio; > unsigned long pfn, max_zone_pfn; > unsigned long total = 0, split = 0; > > @@ -2944,24 +2945,32 @@ static void split_huge_pages_all(void) > int nr_pages; > > page = pfn_to_online_page(pfn); > - if (!page || !get_page_unless_zero(page)) > + if (!page || PageTail(page)) > + continue; Why is the PageTail() test added? > + folio = page_folio(page); > + if (!folio_try_get(folio)) > continue; > > - if (zone != page_zone(page)) > + if (unlikely(page_folio(page) != folio)) And this? > + goto next; > + > + if (zone != folio_zone(folio)) > goto next; > > - if (!PageHead(page) || PageHuge(page) || !PageLRU(page)) > + if (!folio_test_large(folio) > + || folio_test_hugetlb(folio) > + || !folio_test_lru(folio)) > goto next; > > total++; > - lock_page(page); > - nr_pages = thp_nr_pages(page); > - if (!split_huge_page(page)) > + folio_lock(folio); > + nr_pages = folio_nr_pages(folio); > + if (!split_folio(folio)) > split++; > pfn += nr_pages - 1; > - unlock_page(page); > + folio_unlock(folio); > next: > - put_page(page); > + folio_put(folio); > cond_resched(); > } > }
On 2022/12/30 7:28, Andrew Morton wrote: > On Thu, 29 Dec 2022 20:25:03 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> Straightforwardly convert split_huge_pages_all() to use a folio. >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> mm/huge_memory.c | 25 +++++++++++++++++-------- >> 1 file changed, 17 insertions(+), 8 deletions(-) >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index 266c4b557946..c8cbe7f62eaa 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -2932,6 +2932,7 @@ static void split_huge_pages_all(void) >> { >> struct zone *zone; >> struct page *page; >> + struct folio *folio; >> unsigned long pfn, max_zone_pfn; >> unsigned long total = 0, split = 0; >> >> @@ -2944,24 +2945,32 @@ static void split_huge_pages_all(void) >> int nr_pages; >> >> page = pfn_to_online_page(pfn); >> - if (!page || !get_page_unless_zero(page)) >> + if (!page || PageTail(page)) >> + continue; > > Why is the PageTail() test added? This function is trying to split huge pages, it traverse all the pfn, the huge page already split when we met Head page, most importantly, get_page_unless_zero() will do nothing on Tail page too. > >> + folio = page_folio(page); >> + if (!folio_try_get(folio)) >> continue; >> >> - if (zone != page_zone(page)) >> + if (unlikely(page_folio(page) != folio)) > > And this? I think this is a double check in case of page is already changed by someone else, suggested by Matthew[1] [1] https://lore.kernel.org/linux-mm/20221227195004.2809-1-sj@kernel.org/T/#m33047c152f6793bfebaa55cb1f4662fed73508d2
On Thu, Dec 29, 2022 at 08:25:03PM +0800, Kefeng Wang wrote: > - if (!PageHead(page) || PageHuge(page) || !PageLRU(page)) > + if (!folio_test_large(folio) > + || folio_test_hugetlb(folio) > + || !folio_test_lru(folio)) > goto next; That is a completely illegible way of indenting this code! There's no visual cue when the condition stops and when the next statement begins. Try one of these: if (!folio_test_large(folio) || folio_test_hugetlb(folio) || !folio_test_lru(folio)) goto next; if (!folio_test_large(folio) || folio_test_hugetlb(folio) || !folio_test_lru(folio)) goto next;
On 2022/12/30 16:14, Matthew Wilcox wrote: > On Thu, Dec 29, 2022 at 08:25:03PM +0800, Kefeng Wang wrote: >> - if (!PageHead(page) || PageHuge(page) || !PageLRU(page)) >> + if (!folio_test_large(folio) >> + || folio_test_hugetlb(folio) >> + || !folio_test_lru(folio)) >> goto next; > > That is a completely illegible way of indenting this code! There's > no visual cue when the condition stops and when the next statement > begins. Try one of these: > sorry about the bad indenting, > if (!folio_test_large(folio) || > folio_test_hugetlb(folio) || > !folio_test_lru(folio)) > goto next; > will resend with this one, thanks for your review.
diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 266c4b557946..c8cbe7f62eaa 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -2932,6 +2932,7 @@ static void split_huge_pages_all(void) { struct zone *zone; struct page *page; + struct folio *folio; unsigned long pfn, max_zone_pfn; unsigned long total = 0, split = 0; @@ -2944,24 +2945,32 @@ static void split_huge_pages_all(void) int nr_pages; page = pfn_to_online_page(pfn); - if (!page || !get_page_unless_zero(page)) + if (!page || PageTail(page)) + continue; + folio = page_folio(page); + if (!folio_try_get(folio)) continue; - if (zone != page_zone(page)) + if (unlikely(page_folio(page) != folio)) + goto next; + + if (zone != folio_zone(folio)) goto next; - if (!PageHead(page) || PageHuge(page) || !PageLRU(page)) + if (!folio_test_large(folio) + || folio_test_hugetlb(folio) + || !folio_test_lru(folio)) goto next; total++; - lock_page(page); - nr_pages = thp_nr_pages(page); - if (!split_huge_page(page)) + folio_lock(folio); + nr_pages = folio_nr_pages(folio); + if (!split_folio(folio)) split++; pfn += nr_pages - 1; - unlock_page(page); + folio_unlock(folio); next: - put_page(page); + folio_put(folio); cond_resched(); } }
Straightforwardly convert split_huge_pages_all() to use a folio. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- mm/huge_memory.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)