LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vlastimil Babka <vbabka@suse.cz>
To: Mike Kravetz <mike.kravetz@oracle.com>,
	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>,
	Joonsoo Kim <iamjoonsoo.kim@lge.com>
Subject: Re: [PATCH v2 2/4] mm: check for proper migrate type during isolation
Date: Tue, 22 May 2018 09:07:56 +0200	[thread overview]
Message-ID: <a9816b88-015e-7e44-cbb8-a7ff04453870@suse.cz> (raw)
In-Reply-To: <f50d6814-8bc6-80cd-c0e5-b2cfa4f9e576@oracle.com>

On 05/22/2018 01:10 AM, Mike Kravetz wrote:
> On 05/18/2018 03:32 AM, Vlastimil Babka wrote:
>> On 05/04/2018 01:29 AM, Mike Kravetz wrote:
>>> The routine start_isolate_page_range and alloc_contig_range have
>>> comments saying that migratetype must be either MIGRATE_MOVABLE or
>>> MIGRATE_CMA.  However, this is not enforced.
>>
>> Enforced, no. But if the pageblocks really were as such, it used to
>> shortcut has_unmovable_pages(). This was wrong and removed in
>> d7b236e10ced ("mm: drop migrate type checks from has_unmovable_pages")
>> plus 4da2ce250f98 ("mm: distinguish CMA and MOVABLE isolation in
>> has_unmovable_pages()").
>>
>>
>>   What is important is
>>> that that all pageblocks in the range are of type migratetype.
>>                                                the same
>>> This is because blocks will be set to migratetype on error.
>>
>> Strictly speaking this is true only for the CMA case. For other cases,
>> the best thing actually would be to employ the same heuristics as page
>> allocation migratetype fallbacks, and count how many pages are free and
>> how many appear to be movable, see how steal_suitable_fallback() uses
>> the last parameter of move_freepages_block().
>>
>>> Add a boolean argument enforce_migratetype to the routine
>>> start_isolate_page_range.  If set, it will check that all pageblocks
>>> in the range have the passed migratetype.  Return -EINVAL is pageblock
>>                                                             if
>>> is wrong type is found in range.
>>   of
>>>
>>> A boolean is used for enforce_migratetype as there are two primary
>>> users.  Contiguous range allocation which wants to enforce migration
>>> type checking.  Memory offline (hotplug) which is not concerned about
>>> type checking.
>>
>> This is missing some high-level result. The end change is that CMA is
>> now enforcing. So we are making it more robust when it's called on
>> non-CMA pageblocks by mistake? (BTW I still do hope we can remove
>> MIGRATE_CMA soon after Joonsoo's ZONE_MOVABLE CMA conversion. Combined
>> with my suggestion above we could hopefully get rid of the migratetype
>> parameter completely instead of enforcing it?). Is this also a
>> preparation for introducing find_alloc_contig_pages() which will be
>> enforcing? (I guess, and will find out shortly, but it should be stated
>> here)
> 
> Thank you for looking at these patches Vlastimil.
> 
> My primary motivation for this patch was the 'error recovery' in
> start_isolate_page_range.  It takes a range and attempts to set
> all pageblocks to MIGRATE_ISOLATE.  If it encounters an error after
> setting some blocks to isolate, it will 'clean up' by setting the
> migrate type of previously modified blocks to the passed migratetype.

Right.

> So, one possible side effect of an error in start_isolate_page_range
> is that the migrate type of some pageblocks could be modified.  Thinking
> about it more now, that may be OK.

It would be definitely OK if the migratetype was changed similarly as
steal_suitable_fallback() does it, as I've said above.

> It just does not seem like the
> right thing to do, especially with comments saying "migratetype must
> be either MIGRATE_MOVABLE or MIGRATE_CMA".  I'm fine with leaving the
> code as is and just cleaning up the comments if you think that may
> be better.

That's also possible, especially when the code is restructured as I've
suggested in the other reply, which should significantly reduce the
amount of error recoveries.

  reply	other threads:[~2018-05-22  7:08 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-03 23:29 [PATCH v2 0/4] Interface for higher order contiguous allocations Mike Kravetz
2018-05-03 23:29 ` [PATCH v2 1/4] mm: change type of free_contig_range(nr_pages) to unsigned long Mike Kravetz
2018-05-18  9:12   ` Vlastimil Babka
2018-05-18 22:01     ` Mike Kravetz
2018-05-03 23:29 ` [PATCH v2 2/4] mm: check for proper migrate type during isolation Mike Kravetz
2018-05-18 10:32   ` Vlastimil Babka
2018-05-21 23:10     ` Mike Kravetz
2018-05-22  7:07       ` Vlastimil Babka [this message]
2018-05-03 23:29 ` [PATCH v2 3/4] mm: add find_alloc_contig_pages() interface Mike Kravetz
2018-05-21  8:54   ` Vlastimil Babka
2018-05-21 23:48     ` Mike Kravetz
2018-05-22 16:41       ` Reinette Chatre
2018-05-22 20:35         ` Mike Kravetz
2018-05-23 11:18         ` Vlastimil Babka
2018-05-23 18:07           ` Reinette Chatre
2018-05-28 13:12             ` Vlastimil Babka
2018-05-03 23:29 ` [PATCH v2 4/4] mm/hugetlb: use find_alloc_contig_pages() to allocate gigantic pages Mike Kravetz
2018-05-21 12:00 ` [PATCH v2 0/4] Interface for higher order contiguous allocations Vlastimil Babka
2018-05-22  0:15   ` 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=a9816b88-015e-7e44-cbb8-a7ff04453870@suse.cz \
    --to=vbabka@suse.cz \
    --cc=akpm@linux-foundation.org \
    --cc=cl@linux.com \
    --cc=dave.hansen@intel.com \
    --cc=dnellans@nvidia.com \
    --cc=iamjoonsoo.kim@lge.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=mike.kravetz@oracle.com \
    --cc=mina86@mina86.com \
    --cc=pavel@ucw.cz \
    --cc=reinette.chatre@intel.com \
    --cc=sguy@mellanox.com \
    --subject='Re: [PATCH v2 2/4] mm: check for proper migrate type during isolation' \
    /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).