Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 24 Aug 2020 22:10:49 -0700
From:   Andrew Morton <akpm@linux-foundation.org>
To:     js1304@gmail.com
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Michal Hocko <mhocko@kernel.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>,
        kernel-team@lge.com, Joonsoo Kim <iamjoonsoo.kim@lge.com>
Subject: Re: [PATCH for v5.9] mm/page_alloc: handle a missing case for
 memalloc_nocma_{save/restore} APIs
Message-Id: <20200824221049.edb3c540bbfc859a6806600d@linux-foundation.org>
In-Reply-To: <1598331582-19923-1-git-send-email-iamjoonsoo.kim@lge.com>
References: <1598331582-19923-1-git-send-email-iamjoonsoo.kim@lge.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 25 Aug 2020 13:59:42 +0900 js1304@gmail.com wrote:

> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> memalloc_nocma_{save/restore} APIs can be used to skip page allocation
> on CMA area, but, there is a missing case and the page on CMA area could
> be allocated even if APIs are used. This patch handles this case to fix
> the potential issue.
> 
> Missing case is an allocation from the pcplist. MIGRATE_MOVABLE pcplist
> could have the pages on CMA area so we need to skip it if ALLOC_CMA isn't
> specified.
> 
> This patch implements this behaviour by checking allocated page from
> the pcplist rather than skipping an allocation from the pcplist entirely.
> Skipping the pcplist entirely would result in a mismatch between watermark
> check and actual page allocation. And, it requires to break current code
> layering that order-0 page is always handled by the pcplist. I'd prefer
> to avoid it so this patch uses different way to skip CMA page allocation
> from the pcplist.
> 
> ...
>
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3341,6 +3341,22 @@ static struct page *rmqueue_pcplist(struct zone *preferred_zone,
>  	pcp = &this_cpu_ptr(zone->pageset)->pcp;
>  	list = &pcp->lists[migratetype];
>  	page = __rmqueue_pcplist(zone,  migratetype, alloc_flags, pcp, list);
> +#ifdef CONFIG_CMA
> +	if (page) {
> +		int mt = get_pcppage_migratetype(page);
> +
> +		/*
> +		 * pcp could have the pages on CMA area and we need to skip it
> +		 * when !ALLOC_CMA. Free all pcplist and retry allocation.
> +		 */
> +		if (is_migrate_cma(mt) && !(alloc_flags & ALLOC_CMA)) {
> +			list_add(&page->lru, &pcp->lists[migratetype]);
> +			pcp->count++;
> +			free_pcppages_bulk(zone, pcp->count, pcp);
> +			page = __rmqueue_pcplist(zone, migratetype, alloc_flags, pcp, list);
> +		}
> +	}
> +#endif
>  	if (page) {
>  		__count_zid_vm_events(PGALLOC, page_zonenum(page), 1);
>  		zone_statistics(preferred_zone, zone);

That's a bunch more code on a very hot path to serve an obscure feature
which has a single obscure callsite.

Can we instead put the burden on that callsite rather than upon
everyone?  For (dumb) example, teach __gup_longterm_locked() to put the
page back if it's CMA and go get another one?