From: Mike Kravetz <mike.kravetz@oracle.com>
To: Tarun Sahu <tsahu@linux.ibm.com>
Cc: linux-mm@kvack.org, akpm@linux-foundation.org,
muchun.song@linux.dev, aneesh.kumar@linux.ibm.com,
willy@infradead.org, sidhartha.kumar@oracle.com,
gerald.schaefer@linux.ibm.com, linux-kernel@vger.kernel.org,
jaypatel@linux.ibm.com
Subject: Re: [PATCH v3] mm/folio: Avoid special handling for order value 0 in folio_set_order
Date: Fri, 9 Jun 2023 11:10:36 -0700 [thread overview]
Message-ID: <20230609181036.GB139487@monkey> (raw)
In-Reply-To: <20230609162907.111756-1-tsahu@linux.ibm.com>
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
next prev parent reply other threads:[~2023-06-09 18:16 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2023-06-09 18:29 ` Andrew Morton
2023-06-10 6:50 ` Tarun Sahu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230609181036.GB139487@monkey \
--to=mike.kravetz@oracle.com \
--cc=akpm@linux-foundation.org \
--cc=aneesh.kumar@linux.ibm.com \
--cc=gerald.schaefer@linux.ibm.com \
--cc=jaypatel@linux.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=muchun.song@linux.dev \
--cc=sidhartha.kumar@oracle.com \
--cc=tsahu@linux.ibm.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).