* [PATCH v3] mm/folio: Avoid special handling for order value 0 in folio_set_order
@ 2023-06-09 16:29 Tarun Sahu
2023-06-09 18:10 ` Mike Kravetz
2023-06-09 18:29 ` Andrew Morton
0 siblings, 2 replies; 4+ messages in thread
From: Tarun Sahu @ 2023-06-09 16:29 UTC (permalink / raw)
To: linux-mm
Cc: akpm, muchun.song, mike.kravetz, aneesh.kumar, willy,
sidhartha.kumar, gerald.schaefer, linux-kernel, jaypatel, tsahu
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(-)
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
}
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] mm/folio: Avoid special handling for order value 0 in folio_set_order
2023-06-09 16:29 [PATCH v3] mm/folio: Avoid special handling for order value 0 in folio_set_order Tarun Sahu
@ 2023-06-09 18:10 ` Mike Kravetz
2023-06-09 18:29 ` Andrew Morton
1 sibling, 0 replies; 4+ messages in thread
From: Mike Kravetz @ 2023-06-09 18:10 UTC (permalink / raw)
To: Tarun Sahu
Cc: linux-mm, akpm, muchun.song, aneesh.kumar, willy, sidhartha.kumar,
gerald.schaefer, linux-kernel, jaypatel
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>
--
Mike Kravetz
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] mm/folio: Avoid special handling for order value 0 in folio_set_order
2023-06-09 16:29 [PATCH v3] mm/folio: Avoid special handling for order value 0 in folio_set_order Tarun Sahu
2023-06-09 18:10 ` Mike Kravetz
@ 2023-06-09 18:29 ` Andrew Morton
2023-06-10 6:50 ` Tarun Sahu
1 sibling, 1 reply; 4+ messages in thread
From: Andrew Morton @ 2023-06-09 18:29 UTC (permalink / raw)
To: Tarun Sahu
Cc: linux-mm, muchun.song, mike.kravetz, aneesh.kumar, willy,
sidhartha.kumar, gerald.schaefer, linux-kernel, jaypatel
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.
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] mm/folio: Avoid special handling for order value 0 in folio_set_order
2023-06-09 18:29 ` Andrew Morton
@ 2023-06-10 6:50 ` Tarun Sahu
0 siblings, 0 replies; 4+ messages in thread
From: Tarun Sahu @ 2023-06-10 6:50 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, muchun.song, mike.kravetz, aneesh.kumar, willy,
sidhartha.kumar, gerald.schaefer, linux-kernel, jaypatel
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.
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-10 6:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-09 16:29 [PATCH v3] mm/folio: Avoid special handling for order value 0 in folio_set_order Tarun Sahu
2023-06-09 18:10 ` Mike Kravetz
2023-06-09 18:29 ` Andrew Morton
2023-06-10 6:50 ` Tarun Sahu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).