Received-SPF: pass (google.com: domain of linux-kernel+bounces-133372-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 5 Apr 2024 12:56:32 -0400
From: Johannes Weiner <hannes@cmpxchg.org>
To: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Vlastimil Babka <vbabka@suse.cz>,
	Mel Gorman <mgorman@techsingularity.net>, Zi Yan <ziy@nvidia.com>,
	"Huang, Ying" <ying.huang@intel.com>,
	David Hildenbrand <david@redhat.com>, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 06/10] mm: page_alloc: fix freelist movement during block
 conversion
Message-ID: <20240405165632.GA870124@cmpxchg.org>
References: <20240320180429.678181-1-hannes@cmpxchg.org>
 <20240320180429.678181-7-hannes@cmpxchg.org>
 <a97697e0-45b0-4f71-b087-fdc7a1d43c0e@linux.alibaba.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a97697e0-45b0-4f71-b087-fdc7a1d43c0e@linux.alibaba.com>

Hi Baolin,

On Fri, Apr 05, 2024 at 08:11:47PM +0800, Baolin Wang wrote:
> On 2024/3/21 02:02, Johannes Weiner wrote:
> > @@ -2127,15 +2165,14 @@ __rmqueue(struct zone *zone, unsigned int order, int migratetype,
> >   				return page;
> >   		}
> >   	}
> > -retry:
> > +
> >   	page = __rmqueue_smallest(zone, order, migratetype);
> >   	if (unlikely(!page)) {
> >   		if (alloc_flags & ALLOC_CMA)
> >   			page = __rmqueue_cma_fallback(zone, order);
> > -
> > -		if (!page && __rmqueue_fallback(zone, order, migratetype,
> > -								alloc_flags))
> > -			goto retry;
> > +		else
> > +			page = __rmqueue_fallback(zone, order, migratetype,
> > +						  alloc_flags);
> 
> (Sorry for chim in late.)
> 
> I have some doubts about the changes here. The original code logic was 
> that if the 'migratetype' type allocation is failed, it would first try 
> CMA page allocation and then attempt to fallback to other migratetype 
> allocations. Now it has been changed so that if CMA allocation fails, it 
> will directly return. This change has caused a regression when I running 
> the thpcompact benchmark, resulting in a significant reduction in the 
> percentage of THPs like below:
> 
> thpcompact Percentage Faults Huge
>                           K6.9-rc2                K6.9-rc2 + this patch
> Percentage huge-1        78.18 (   0.00%)       42.49 ( -45.65%)
> Percentage huge-3        86.70 (   0.00%)       35.13 ( -59.49%)
> Percentage huge-5        90.26 (   0.00%)       52.35 ( -42.00%)
> Percentage huge-7        92.34 (   0.00%)       31.84 ( -65.52%)
> Percentage huge-12       91.18 (   0.00%)       45.85 ( -49.72%)
> Percentage huge-18       89.00 (   0.00%)       29.18 ( -67.22%)
> Percentage huge-24       90.52 (   0.00%)       46.68 ( -48.43%)
> Percentage huge-30       94.44 (   0.00%)       38.35 ( -59.39%)
> Percentage huge-32       93.09 (   0.00%)       39.37 ( -57.70%)

Ouch, sorry about that! I changed that specific part around later
during development and didn't retest with CMA. I'll be sure to
re-enable it again in my config.

> After making the following modifications, the regression is gone.
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index ce67dc6777fa..a7cfe65e45c1 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2139,7 +2139,8 @@ __rmqueue(struct zone *zone, unsigned int order, 
> int migratetype,
>          if (unlikely(!page)) {
>                  if (alloc_flags & ALLOC_CMA)
>                          page = __rmqueue_cma_fallback(zone, order);
> -               else
> +
> +               if (!page)
>                          page = __rmqueue_fallback(zone, order, migratetype,
>                                                    alloc_flags);
>          }
> 
> But I am not sure your original change is intentional? IIUC, we still 
> need try fallbacking even though CMA allocation is failed, please 
> correct me if I misunderstand your code. Thanks.

No, this was accidental. I missed that CMA dependency when changing
things around for the new return type of __rmqueue_fallback(). Your
fix is good: just because the request qualifies for CMA doesn't mean
it will succeed from that region. We need the fallback for those.

Andrew, could you please pick up Baolin's change for this patch?

[baolin.wang@linux.alibaba.com: fix allocation failures with CONFIG_CMA]

Thanks for debugging this and the fix, Baolin.