LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mike Kravetz <mike.kravetz@oracle.com>
To: Vlastimil Babka <vbabka@suse.cz>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	linux-api@vger.kernel.org
Cc: Reinette Chatre <reinette.chatre@intel.com>,
	Michal Hocko <mhocko@kernel.org>,
	Christopher Lameter <cl@linux.com>,
	Guy Shattah <sguy@mellanox.com>,
	Anshuman Khandual <khandual@linux.vnet.ibm.com>,
	Michal Nazarewicz <mina86@mina86.com>,
	David Nellans <dnellans@nvidia.com>,
	Laura Abbott <labbott@redhat.com>, Pavel Machek <pavel@ucw.cz>,
	Dave Hansen <dave.hansen@intel.com>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 2/3] mm: add find_alloc_contig_pages() interface
Date: Wed, 2 May 2018 14:13:32 -0700	[thread overview]
Message-ID: <af56d281-fea1-2bd7-aff1-108fa1d9f3be@oracle.com> (raw)
In-Reply-To: <deb9dd1d-84bf-75c7-2880-a7bcec880d47@suse.cz>

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 <mike.kravetz@oracle.com>
> 
> 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.

-- 
Mike Kravetz

  reply	other threads:[~2018-05-02 21:14 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-17  2:09 [PATCH 0/3] Interface for higher order contiguous allocations Mike Kravetz
2018-04-17  2:09 ` [PATCH 1/3] mm: change type of free_contig_range(nr_pages) to unsigned long Mike Kravetz
2018-04-17  2:09 ` [PATCH 2/3] mm: add find_alloc_contig_pages() interface Mike Kravetz
2018-04-17 12:10   ` kbuild test robot
2018-04-18  1:39     ` Mike Kravetz
2018-04-17 14:19   ` kbuild test robot
2018-04-21 16:16   ` Vlastimil Babka
2018-05-02 21:13     ` Mike Kravetz [this message]
2018-06-06  9:32       ` Michal Hocko
2018-04-23  0:09   ` Michal Hocko
2018-04-23  4:22     ` Mike Kravetz
2018-04-24 13:37       ` Michal Hocko
2018-04-17  2:09 ` [PATCH 3/3] mm/hugetlb: use find_alloc_contig_pages() to allocate gigantic pages Mike Kravetz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=af56d281-fea1-2bd7-aff1-108fa1d9f3be@oracle.com \
    --to=mike.kravetz@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=cl@linux.com \
    --cc=dave.hansen@intel.com \
    --cc=dnellans@nvidia.com \
    --cc=khandual@linux.vnet.ibm.com \
    --cc=labbott@redhat.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@kernel.org \
    --cc=mina86@mina86.com \
    --cc=pavel@ucw.cz \
    --cc=reinette.chatre@intel.com \
    --cc=sguy@mellanox.com \
    --cc=vbabka@suse.cz \
    --subject='Re: [PATCH 2/3] mm: add find_alloc_contig_pages() interface' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).