| Message ID | 20230609162907.111756-1-tsahu@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] mm/folio: Avoid special handling for order value 0 in folio_set_order | expand |
On 06/09/23 21:59, Tarun Sahu wrote: > folio_set_order(folio, 0) is used in kernel at two places > __destroy_compound_gigantic_folio and __prep_compound_gigantic_folio. > Currently, It is called to clear out the folio->_folio_nr_pages and > folio->_folio_order. > > For __destroy_compound_gigantic_folio: > In past, folio_set_order(folio, 0) was needed because page->mapping used > to overlap with _folio_nr_pages and _folio_order. So if these fields were > left uncleared during freeing gigantic hugepages, they were causing > "BUG: bad page state" due to non-zero page->mapping. Now, After > Commit a01f43901cfb ("hugetlb: be sure to free demoted CMA pages to > CMA") page->mapping has explicitly been cleared out for tail pages. Also, > _folio_order and _folio_nr_pages no longer overlaps with page->mapping. > > So, folio_set_order(folio, 0) can be removed from freeing gigantic > folio path (__destroy_compound_gigantic_folio). > > Another place, folio_set_order(folio, 0) is called inside > __prep_compound_gigantic_folio during error path. Here, > folio_set_order(folio, 0) can also be removed if we move > folio_set_order(folio, order) after for loop. > > The patch also moves _folio_set_head call in __prep_compound_gigantic_folio() > such that we avoid clearing them in the error path. > > Also, as Mike pointed out: > "It would actually be better to move the calls _folio_set_head and > folio_set_order in __prep_compound_gigantic_folio() as suggested here. Why? > In the current code, the ref count on the 'head page' is still 1 (or more) > while those calls are made. So, someone could take a speculative ref on the > page BEFORE the tail pages are set up." > > This way, folio_set_order(folio, 0) is no more needed. And it will also > helps removing the confusion of folio order being set to 0 (as _folio_order > field is part of first tail page). > > Testing: I have run LTP tests, which all passes. and also I have written > the test in LTP which tests the bug caused by compound_nr and page->mapping > overlapping. > > https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/mem/hugetlb/hugemmap/hugemmap32.c > > Running on older kernel ( < 5.10-rc7) with the above bug this fails while > on newer kernel and, also with this patch it passes. > > Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com> > --- > v2->v3 > - removed the copy of page/folio definition from commit msg > v1->v2 > - Reword the commit message > > mm/hugetlb.c | 9 +++------ > mm/internal.h | 8 ++------ > 2 files changed, 5 insertions(+), 12 deletions(-) Thanks for answering all the questions along the way! Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
On Fri, 9 Jun 2023 21:59:07 +0530 Tarun Sahu <tsahu@linux.ibm.com> wrote: > folio_set_order(folio, 0) is used in kernel at two places > __destroy_compound_gigantic_folio and __prep_compound_gigantic_folio. > Currently, It is called to clear out the folio->_folio_nr_pages and > folio->_folio_order. > > For __destroy_compound_gigantic_folio: > In past, folio_set_order(folio, 0) was needed because page->mapping used > to overlap with _folio_nr_pages and _folio_order. So if these fields were > left uncleared during freeing gigantic hugepages, they were causing > "BUG: bad page state" due to non-zero page->mapping. Now, After > Commit a01f43901cfb ("hugetlb: be sure to free demoted CMA pages to > CMA") page->mapping has explicitly been cleared out for tail pages. Also, > _folio_order and _folio_nr_pages no longer overlaps with page->mapping. > > So, folio_set_order(folio, 0) can be removed from freeing gigantic > folio path (__destroy_compound_gigantic_folio). The above appears to be a code cleanup only? > Another place, folio_set_order(folio, 0) is called inside > __prep_compound_gigantic_folio during error path. Here, > folio_set_order(folio, 0) can also be removed if we move > folio_set_order(folio, order) after for loop. > > The patch also moves _folio_set_head call in __prep_compound_gigantic_folio() > such that we avoid clearing them in the error path. And the above also sounds like a code cleanup. > Also, as Mike pointed out: > "It would actually be better to move the calls _folio_set_head and > folio_set_order in __prep_compound_gigantic_folio() as suggested here. Why? > In the current code, the ref count on the 'head page' is still 1 (or more) > while those calls are made. So, someone could take a speculative ref on the > page BEFORE the tail pages are set up." > > This way, folio_set_order(folio, 0) is no more needed. And it will also > helps removing the confusion of folio order being set to 0 (as _folio_order > field is part of first tail page). > > Testing: I have run LTP tests, which all passes. and also I have written > the test in LTP which tests the bug caused by compound_nr and page->mapping > overlapping. What bug? Please describe the end-user visible effects of any bug. And if a bug is indeed fixed, please let's try to identify a Fixes: target and let's decide whether a -stable backport is needed. Thanks. > https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/mem/hugetlb/hugemmap/hugemmap32.c > > Running on older kernel ( < 5.10-rc7) with the above bug this fails while > on newer kernel and, also with this patch it passes. >
Hi Andrew, TLDR: It is not bug fix, it is just cleanup. Andrew Morton <akpm@linux-foundation.org> writes: > On Fri, 9 Jun 2023 21:59:07 +0530 Tarun Sahu <tsahu@linux.ibm.com> wrote: > >> folio_set_order(folio, 0) is used in kernel at two places >> __destroy_compound_gigantic_folio and __prep_compound_gigantic_folio. >> Currently, It is called to clear out the folio->_folio_nr_pages and >> folio->_folio_order. >> >> For __destroy_compound_gigantic_folio: >> In past, folio_set_order(folio, 0) was needed because page->mapping used >> to overlap with _folio_nr_pages and _folio_order. So if these fields were >> left uncleared during freeing gigantic hugepages, they were causing >> "BUG: bad page state" due to non-zero page->mapping. Now, After >> Commit a01f43901cfb ("hugetlb: be sure to free demoted CMA pages to >> CMA") page->mapping has explicitly been cleared out for tail pages. Also, >> _folio_order and _folio_nr_pages no longer overlaps with page->mapping. >> >> So, folio_set_order(folio, 0) can be removed from freeing gigantic >> folio path (__destroy_compound_gigantic_folio). > > The above appears to be a code cleanup only? yes, > >> Another place, folio_set_order(folio, 0) is called inside >> __prep_compound_gigantic_folio during error path. Here, >> folio_set_order(folio, 0) can also be removed if we move >> folio_set_order(folio, order) after for loop. >> >> The patch also moves _folio_set_head call in __prep_compound_gigantic_folio() >> such that we avoid clearing them in the error path. > > And the above also sounds like a code cleanup. yes > >> Also, as Mike pointed out: >> "It would actually be better to move the calls _folio_set_head and >> folio_set_order in __prep_compound_gigantic_folio() as suggested here. Why? >> In the current code, the ref count on the 'head page' is still 1 (or more) >> while those calls are made. So, someone could take a speculative ref on the >> page BEFORE the tail pages are set up." >> >> This way, folio_set_order(folio, 0) is no more needed. And it will also >> helps removing the confusion of folio order being set to 0 (as _folio_order >> field is part of first tail page). >> >> Testing: I have run LTP tests, which all passes. and also I have written >> the test in LTP which tests the bug caused by compound_nr and page->mapping >> overlapping. > > What bug? Please describe the end-user visible effects of any bug. > > And if a bug is indeed fixed, please let's try to identify a Fixes: > target and let's decide whether a -stable backport is needed. > > Thanks. > No bug fixed here, The above cleanup modifies the code which touches the code path that a past patch had added to resolve the bug. The above test just check if the resolution is not affected. >> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/mem/hugetlb/hugemmap/hugemmap32.c >> >> Running on older kernel ( < 5.10-rc7) with the above bug this fails while >> on newer kernel and, also with this patch it passes. >>
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index f154019e6b84..607553445855 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -1489,7 +1489,6 @@ static void __destroy_compound_gigantic_folio(struct folio *folio, set_page_refcounted(p); } - folio_set_order(folio, 0); __folio_clear_head(folio); } @@ -1951,9 +1950,6 @@ static bool __prep_compound_gigantic_folio(struct folio *folio, struct page *p; __folio_clear_reserved(folio); - __folio_set_head(folio); - /* we rely on prep_new_hugetlb_folio to set the destructor */ - folio_set_order(folio, order); for (i = 0; i < nr_pages; i++) { p = folio_page(folio, i); @@ -1999,6 +1995,9 @@ static bool __prep_compound_gigantic_folio(struct folio *folio, if (i != 0) set_compound_head(p, &folio->page); } + __folio_set_head(folio); + /* we rely on prep_new_hugetlb_folio to set the destructor */ + folio_set_order(folio, order); atomic_set(&folio->_entire_mapcount, -1); atomic_set(&folio->_nr_pages_mapped, 0); atomic_set(&folio->_pincount, 0); @@ -2017,8 +2016,6 @@ static bool __prep_compound_gigantic_folio(struct folio *folio, p = folio_page(folio, j); __ClearPageReserved(p); } - folio_set_order(folio, 0); - __folio_clear_head(folio); return false; } diff --git a/mm/internal.h b/mm/internal.h index 68410c6d97ac..c59fe08c5b39 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -425,16 +425,12 @@ int split_free_page(struct page *free_page, */ static inline void folio_set_order(struct folio *folio, unsigned int order) { - if (WARN_ON_ONCE(!folio_test_large(folio))) + if (WARN_ON_ONCE(!order || !folio_test_large(folio))) return; folio->_folio_order = order; #ifdef CONFIG_64BIT - /* - * When hugetlb dissolves a folio, we need to clear the tail - * page, rather than setting nr_pages to 1. - */ - folio->_folio_nr_pages = order ? 1U << order : 0; + folio->_folio_nr_pages = 1U << order; #endif }
folio_set_order(folio, 0) is used in kernel at two places __destroy_compound_gigantic_folio and __prep_compound_gigantic_folio. Currently, It is called to clear out the folio->_folio_nr_pages and folio->_folio_order. For __destroy_compound_gigantic_folio: In past, folio_set_order(folio, 0) was needed because page->mapping used to overlap with _folio_nr_pages and _folio_order. So if these fields were left uncleared during freeing gigantic hugepages, they were causing "BUG: bad page state" due to non-zero page->mapping. Now, After Commit a01f43901cfb ("hugetlb: be sure to free demoted CMA pages to CMA") page->mapping has explicitly been cleared out for tail pages. Also, _folio_order and _folio_nr_pages no longer overlaps with page->mapping. So, folio_set_order(folio, 0) can be removed from freeing gigantic folio path (__destroy_compound_gigantic_folio). Another place, folio_set_order(folio, 0) is called inside __prep_compound_gigantic_folio during error path. Here, folio_set_order(folio, 0) can also be removed if we move folio_set_order(folio, order) after for loop. The patch also moves _folio_set_head call in __prep_compound_gigantic_folio() such that we avoid clearing them in the error path. Also, as Mike pointed out: "It would actually be better to move the calls _folio_set_head and folio_set_order in __prep_compound_gigantic_folio() as suggested here. Why? In the current code, the ref count on the 'head page' is still 1 (or more) while those calls are made. So, someone could take a speculative ref on the page BEFORE the tail pages are set up." This way, folio_set_order(folio, 0) is no more needed. And it will also helps removing the confusion of folio order being set to 0 (as _folio_order field is part of first tail page). Testing: I have run LTP tests, which all passes. and also I have written the test in LTP which tests the bug caused by compound_nr and page->mapping overlapping. https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/mem/hugetlb/hugemmap/hugemmap32.c Running on older kernel ( < 5.10-rc7) with the above bug this fails while on newer kernel and, also with this patch it passes. Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com> --- v2->v3 - removed the copy of page/folio definition from commit msg v1->v2 - Reword the commit message mm/hugetlb.c | 9 +++------ mm/internal.h | 8 ++------ 2 files changed, 5 insertions(+), 12 deletions(-)