On Mon, Dec 17, 2018 at 01:21:32PM +0100, Michal Hocko wrote:
>On Fri 14-12-18 15:17:56, Wei Yang wrote:
>> On Thu, Dec 13, 2018 at 07:57:12PM -0800, Andrew Morton wrote:
>> >On Fri, 14 Dec 2018 10:39:12 +0800 Wei Yang <richard.weiyang@xxxxxxxxx> wrote:
>> >
>> >> Below is a brief call flow for __offline_pages()
>> >
>> >Offtopic...
>> >
>> >set_migratetype_isolate() has the comment
>> >
>> > /*
>> > * immobile means "not-on-lru" pages. If immobile is larger than
>> > * removable-by-driver pages reported by notifier, we'll fail.
>> > */
>> >
>> >what the heck does that mean? It used to talk about unmovable pages,
>> >but this was mysteriously changed to use the unique term "immobile" by
>> >Minchan's ee6f509c32 ("mm: factor out memory isolate functions").
>> >Could someone please take a look?
>> >
>> >
>> >> and
>> >> alloc_contig_range():
>> >>
>> >> __offline_pages()/alloc_contig_range()
>> >> start_isolate_page_range()
>> >> set_migratetype_isolate()
>> >> drain_all_pages()
>> >> drain_all_pages()
>> >>
>> >> Since set_migratetype_isolate() is only used in
>> >> start_isolate_page_range(), which is just used in __offline_pages() and
>> >> alloc_contig_range(). And both of them call drain_all_pages() if every
>> >> check looks good. This means it is not necessary call drain_all_pages()
>> >> in each iteration of set_migratetype_isolate().
>> >>
>> >> By doing so, the logic seems a little bit clearer.
>> >> set_migratetype_isolate() handles pages in Buddy, while
>> >> drain_all_pages() takes care of pages in pcp.
>> >
>> >Well. drain_all_pages() moves pages from pcp to buddy so I'm not sure
>> >that argument holds water.
>> >
>> >Can we step back a bit and ask ourselves what all these draining
>> >operations are actually for? What is the intent behind each callsite?
>> >Figuring that out (and perhaps even documenting it!) would help us
>> >decide the most appropriate places from which to perform the drain.
>>
>> With some rethinking we even could take drain_all_pages() out of the
>> repeat loop. Because after isolation, the page in this range will not be
>> put to pcp pageset. So we just need to drain pages once.
>>
>> The change may look like this.
>
>No, this is incorrect. Draining pcp lists before scan_movable_pages is
>most likely sub-optimal, because scan_movable_pages will simply ignore
>pages being on the pcp lists. But we definitely want to drain before we
>terminate the offlining phase because we do not want to have isolated
>pages on those lists before we allow the final hotremove.
>
>The way how we retry the migration loop until there is no page in use
>just guarantees that drain_all_pages is called. If you put it out of the
>loop then you just break that assumption. Moving drain_all_pages down
>after the migration is done should work well AFAICS but I didn't really
>think through all potential side effects nor have time to do so now.
>
After discussion, do you agree with this proposal?
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 6910e0eea074..120e9fdfd055 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1590,6 +1590,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
>> if (ret)
>> goto failed_removal;
>>
>> + drain_all_pages(zone);
>> pfn = start_pfn;
>> repeat:
>> /* start memory hot removal */
>> @@ -1599,7 +1600,6 @@ static int __ref __offline_pages(unsigned long start_pfn,
>>
>> cond_resched();
>> lru_add_drain_all();
>> - drain_all_pages(zone);
>>
>> pfn = scan_movable_pages(start_pfn, end_pfn);
>> if (pfn) { /* We have movable pages */
>>
>> --
>> Wei Yang
>> Help you, Help me
>
>--
>Michal Hocko
>SUSE Labs
--
Wei Yang
Help you, Help me
