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;
Date:   Fri, 14 Apr 2023 12:19:03 +0200
From:   Michal Hocko <mhocko@suse.com>
To:     Mel Gorman <mgorman@techsingularity.net>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        Oscar Salvador <OSalvador@suse.com>,
        Yuanxi Liu <y.liu@naruida.com>,
        David Hildenbrand <david@redhat.com>,
        Linux-MM <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mm: page_alloc: Assume huge tail pages are valid when
 allocating contiguous pages
Message-ID: <ZDkolzCWQNe0NmTD@dhcp22.suse.cz>
References: <20230414082222.idgw745cgcduzy37@techsingularity.net>
 <ZDkU6PuuDsUb82lr@dhcp22.suse.cz>
 <20230414095204.7fz6trkj5i4mzthz@techsingularity.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230414095204.7fz6trkj5i4mzthz@techsingularity.net>
Precedence: bulk

On Fri 14-04-23 10:52:04, Mel Gorman wrote:
> On Fri, Apr 14, 2023 at 10:55:04AM +0200, Michal Hocko wrote:
> > On Fri 14-04-23 09:22:22, Mel Gorman wrote:
> > [...]
> > > +
> > > +		/*
> > > +		 * Do not migrate huge pages that span the size of the region
> > > +		 * being allocated contiguous. e.g. Do not migrate a 1G page
> > > +		 * for a 1G allocation request. CMA is an exception as the
> > > +		 * region may be reserved for hardware that requires physical
> > > +		 * memory without a MMU or scatter/gather capability.
> > > +		 *
> > > +		 * Note that the compound check is race-prone versus
> > > +		 * free/split/collapse but it should be safe and result in
> > > +		 * a premature skip or a useless migration attempt.
> > > +		 */
> > > +		if (PageHuge(page) && compound_nr(page) >= nr_pages &&
> > > +		    !is_migrate_cma_page(page)) {
> > > +			return false;
> > 
> > Is the CMA check working as expected?
> 
> I didn't test it as I don't have a good simulator for CMA contraints which
> is still a mobile phone concern for devices like cameras.
> 
> > The function sounds quite generic
> > and I agree that it would make sense if it was generic but it is used
> > only for GB pages in fact and unless I am missing something it would
> > allow to migrate CMA pages and potentially allocate over that region
> > without any possibility to migrate GB page out so the CMA region would
> > be essentially unusable for CMA users.
> 
> It's used primarily for 1G pages but does have other users (debugging
> mostly, low priority). As it's advertised as a general API, I decided to
> treat it as such and that meant being nice to CMA if possible. If CMA pages
> migrate but can still use the target location then it should be fine. If a
> CMA can migrate to an usable location that breaks a device then that's a bug.
> 
> > GB pages already have their CMA
> > allocator path before we get to alloc_contig_pages. Or do I miss
> > something?
> 
> I don't think you missed anything. The CMA check is, at best, an effort
> to have a potentially useful semantic but it's very doubtful anyone will
> notice or care. I'm perfectly happy just to drop the CMA check because it's a
> straight-forward fix and more suitable as a -stable backport.  I'm also happy
> to just go with a PageHuge check and ignore any possibility that a 2M page
> could be migrated to satisfy a 1G allocation.  1G allocation requests after
> significant uptime is a crapshoot at best and relying on them succeeding is
> unwise. There is a non-zero possibility that the latency incurred migrating
> 2M pages and still failing a 1G allocation could itself be classed as a
> bug with users preferring fast-failure of 1G allocation attempts.

Yes, the simpler the better. If we encounter a real usecase where couple
of 2MB hugetlb pages stand in the way to GB pages then we can add the
check so I would just go with reintroducing the PageHuge check alone.

Thanks!
-- 
Michal Hocko
SUSE Labs