From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 13F1760764 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932581AbeFFJcq (ORCPT + 25 others); Wed, 6 Jun 2018 05:32:46 -0400 Received: from mx2.suse.de ([195.135.220.15]:47454 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932492AbeFFJcS (ORCPT ); Wed, 6 Jun 2018 05:32:18 -0400 Date: Wed, 6 Jun 2018 11:32:14 +0200 From: Michal Hocko To: Mike Kravetz Cc: Vlastimil Babka , linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, Reinette Chatre , Christopher Lameter , Guy Shattah , Anshuman Khandual , Michal Nazarewicz , David Nellans , Laura Abbott , Pavel Machek , Dave Hansen , Andrew Morton Subject: Re: [PATCH 2/3] mm: add find_alloc_contig_pages() interface Message-ID: <20180606093214.GE32433@dhcp22.suse.cz> References: <20180417020915.11786-1-mike.kravetz@oracle.com> <20180417020915.11786-3-mike.kravetz@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.5 (2018-04-13) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed 02-05-18 14:13:32, Mike Kravetz wrote: > On 04/21/2018 09:16 AM, Vlastimil Babka wrote: > > On 04/17/2018 04:09 AM, Mike Kravetz wrote: > >> find_alloc_contig_pages() is a new interface that attempts to locate > >> and allocate a contiguous range of pages. It is provided as a more > >> convenient interface than alloc_contig_range() which is currently > >> used by CMA and gigantic huge pages. > >> > >> When attempting to allocate a range of pages, migration is employed > >> if possible. There is no guarantee that the routine will succeed. > >> So, the user must be prepared for failure and have a fall back plan. > >> > >> Signed-off-by: Mike Kravetz > > > > Hi, just two quick observations, maybe discussion pointers for the > > LSF/MM session: > > - it's weird that find_alloc_contig_pages() takes an order, and > > free_contig_pages() takes a nr_pages. I suspect the interface would be > > more future-proof with both using nr_pages? Perhaps also minimum > > alignment for the allocation side? Order is fine for hugetlb, but what > > about other potential users? > > Agreed, and I am changing this to nr_pages and adding alignment. > > > - contig_alloc_migratetype_ok() says that MIGRATE_CMA blocks are OK to > > allocate from. This silently assumes that everything allocated by this > > will be migratable itself, or it might eat CMA reserves. Is it the case? > > Also you then call alloc_contig_range() with MIGRATE_MOVABLE, so it will > > skip/fail on MIGRATE_CMA anyway IIRC. > > When looking closer at the code, alloc_contig_range currently has comments > saying migratetype must be MIGRATE_MOVABLE or MIGRATE_CMA. However, this > is not checked/enforced anywhere in the code (that I can see). The > migratetype passed to alloc_contig_range() will be used to set the migrate > type of all pageblocks in the range. If there is an error, one side effect > is that some pageblocks may have their migrate type changed to migratetype. > Depending on how far we got before hitting the error, the number of pageblocks > changed is unknown. This actually can happen at the lower level routine > start_isolate_page_range(). > > My first thought was to make start_isolate_page_range/set_migratetype_isolate > check that the migrate type of a pageblock was migratetype before isolating. > This would work for CMA, and I could make it work for the new allocator. > However, offline_pages also calls start_isolate_page_range and I believe we > do not want to enforce such a rule (all pageblocks must be of the same migrate > type) for memory hotplug/offline? > > Should we be concerned at all about this potential changing of migrate type > on error? The only way I can think to avoid this is to save the original > migrate type before isolation. This is more a question to Vlastimil, Joonsoo. But my understanding is that it doesn't matter. MIGRATE_MOVABLE will not block other allocations. So we seem to need it only for MIGRATE_CMA. The later should die sooner or later hopefully so this awful kludge should just die with it. -- Michal Hocko SUSE Labs