Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Tarun Sahu <tsahu@linux.ibm.com>
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     linux-mm@kvack.org, muchun.song@linux.dev, mike.kravetz@oracle.com,
        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
In-Reply-To: <20230609112944.fc08936beb29a18f7bfb5ae3@linux-foundation.org>
References: <20230609162907.111756-1-tsahu@linux.ibm.com>
 <20230609112944.fc08936beb29a18f7bfb5ae3@linux-foundation.org>
Date:   Sat, 10 Jun 2023 12:20:20 +0530
Message-ID: <87ilbverpv.fsf@linux.ibm.com>
Content-Type: text/plain
MIME-Version: 1.0
Precedence: bulk

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.
>>