Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Mon, 16 Oct 2023 16:26:29 -0400
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     Zi Yan <ziy@nvidia.com>
Cc:     David Hildenbrand <david@redhat.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Mel Gorman <mgorman@techsingularity.net>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Kefeng Wang <wangkefeng.wang@huawei.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2 0/6] mm: page_alloc: freelist migratetype hygiene
Message-ID: <20231016202629.GA1042487@cmpxchg.org>
References: <0D2BD71D-5E65-4175-8872-5E70278C57DA@nvidia.com>
 <329AB331-DDC1-4074-A85E-AB5CF866CE84@nvidia.com>
 <20231010211200.GA129823@cmpxchg.org>
 <D6A142AB-08F6-4335-8D08-1743DFAAD10C@nvidia.com>
 <43350F6E-E56D-41C8-8441-A96D83B7D7C1@nvidia.com>
 <ED43DACB-C77F-40D3-8806-D3F26AD95E8D@nvidia.com>
 <20231016143717.GH470544@cmpxchg.org>
 <5B61B70F-E85D-4E6F-8856-17A90B899F98@nvidia.com>
 <20231016185113.GI470544@cmpxchg.org>
 <6F2C52B6-2031-4694-A124-0DFDE9F88E88@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6F2C52B6-2031-4694-A124-0DFDE9F88E88@nvidia.com>
Precedence: bulk

On Mon, Oct 16, 2023 at 03:49:49PM -0400, Zi Yan wrote:
> On 16 Oct 2023, at 14:51, Johannes Weiner wrote:
> 
> > On Mon, Oct 16, 2023 at 11:00:33AM -0400, Zi Yan wrote:
> >> On 16 Oct 2023, at 10:37, Johannes Weiner wrote:
> >>
> >>> On Mon, Oct 16, 2023 at 09:35:34AM -0400, Zi Yan wrote:
> >>>>> The attached patch has all the suggested changes, let me know how it
> >>>>> looks to you. Thanks.
> >>>>
> >>>> The one I sent has free page accounting issues. The attached one fixes them.
> >>>
> >>> Do you still have the warnings? I wonder what went wrong.
> >>
> >> No warnings. But something with the code:
> >>
> >> 1. in your version, split_free_page() is called without changing any pageblock
> >> migratetypes, then split_free_page() is just a no-op, since the page is
> >> just deleted from the free list, then freed via different orders. Buddy allocator
> >> will merge them back.
> >
> > Hm not quite.
> >
> > If it's the tail block of a buddy, I update its type before
> > splitting. The splitting loop looks up the type of each block for
> > sorting it onto freelists.
> >
> > If it's the head block, yes I split it first according to its old
> > type. But then I let it fall through to scanning the block, which will
> > find that buddy, update its type and move it.
> 
> That is the issue, since split_free_page() assumes the pageblocks of
> that free page have different types. It basically just free the page
> with different small orders summed up to the original free page order.
> If all pageblocks of the free page have the same migratetype, __free_one_page()
> will merge these small order pages back to the original order free page.

duh, of course, you're right. Thanks for patiently explaining this.

> >> 2. in my version, I set pageblock migratetype to new_mt before split_free_page(),
> >> but it causes free page accounting issues, since in the case of head, free pages
> >> are deleted from new_mt when they are in old_mt free list and the accounting
> >> decreases new_mt free page number instead of old_mt one.
> >
> > Right, that makes sense.
> >
> >> Basically, split_free_page() is awkward as it relies on preset migratetypes,
> >> which changes migratetypes without deleting the free pages from the list first.
> >> That is why I came up with the new split_free_page() below.
> >
> > Yeah, the in-between thing is bad. Either it fixes the migratetype
> > before deletion, or it doesn't do the deletion. I'm thinking it would
> > be simpler to move the deletion out instead.
> 
> Yes and no. After deletion, a free page no longer has PageBuddy set and
> has buddy_order information cleared. Either we reset PageBuddy and order
> to the deleted free page, or split_free_page() needs to be changed to
> accept pages without the information (basically remove the PageBuddy
> and order check code).

Good point, that requires extra care.

It's correct in the code now, but it deserves a comment, especially
because of the "buddy" naming in the new split function.

> >> Hmm, if CONFIG_ARCH_FORCE_MAX_ORDER can make a buddy have more than one
> >> pageblock and in turn makes an in-use page have more than one pageblock,
> >> we will have problems. Since in isolate_single_pageblock(), an in-use page
> >> can have part of its pageblock set to a different migratetype and be freed,
> >> causing the free page with unmatched migratetypes. We might need to
> >> free pages at pageblock_order if their orders are bigger than pageblock_order.
> >
> > Is this a practical issue? You mentioned that right now only gigantic
> > pages can be larger than a pageblock, and those are freed in order-0
> > chunks.
> 
> Only if the system allocates a page (non hugetlb pages) with >pageblock_order
> and frees it with the same order. I just do not know if such pages exist on
> other arch than x86. Maybe I just think too much.

Hm, I removed LRU pages from the handling (and added the warning) but
I left in PageMovable(). The only users are z3fold, zsmalloc and
memory ballooning. AFAICS none of them can be bigger than a pageblock.
Let me remove that and add a warning for that case as well.

This way, we only attempt to migrate hugetlb, where we know the free
path - and get warnings for anything else that's larger than expected.

This seems like the safest option. On the off chance that there is a
regression, it won't jeopardize anybody's systems, while the warning
provides all the information we need to debug what's going on.

> > From a0460ad30a24cf73816ac40b262af0ba3723a242 Mon Sep 17 00:00:00 2001
> > From: Johannes Weiner <hannes@cmpxchg.org>
> > Date: Mon, 16 Oct 2023 12:32:21 -0400
> > Subject: [PATCH] mm: page_isolation: prepare for hygienic freelists
> >
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

> It looks good to me. Thanks.
> 
> Reviewed-by: Zi Yan <ziy@nvidia.com>

Thank you for all your help!