From: osalvador@suse.de
To: Michal Hocko <mhocko@kernel.org>
Cc: akpm@linux-foundation.org, david@redhat.com,
pasha.tatashin@soleen.com, dan.j.williams@gmail.com,
linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH] mm, memory_hotplug: Don't bail out in do_migrate_range prematurely
Date: Tue, 11 Dec 2018 13:22:27 +0100 [thread overview]
Message-ID: <6009dea8a638aaa5b88088a117297edf@suse.de> (raw)
In-Reply-To: <20181211101818.GE1286@dhcp22.suse.cz>
On 2018-12-11 11:18, Michal Hocko wrote:
>> Currently, if we fail to isolate a single page, we put all already
>> isolated pages back to their LRU and we bail out from the function.
>> This is quite suboptimal, as this will force us to start over again
>> because scan_movable_pages will give us the same range.
>> If there is no chance that we can isolate that page, we will loop here
>> forever.
>
> This is true but reorganizing the code will not help the underlying
> issue. Because the permanently failing page will be still there for
> scan_movable_pages to encounter.
Well, it would only help in case the page is neither LRU nor
non-movable page, then we would fail to isolate it in do_migrate_range
and we will start over.
Letting do_migrate_range do some work, would mean that at some point
the permanently failing page will not be within a range but the first
one
of a range, and so scan_movable_pages will skip it.
>
>> Issue debugged in 4d0c7db96 ("hwpoison, memory_hotplug: allow
>> hwpoisoned
>> pages to be offlined") has proved that.
>
> I assume that 4d0c7db96 is a sha1 from the linux-next. Please note that
> this is not going to be the case when merged upstream. So I would use a
> link.
I will replace the sha1 with the link in the next version.
>> Although this patch has proved to be useful when dealing with
>> 4d0c7db96 because it allows us to move forward as long as the
>> page is not in LRU, we still need 4d0c7db96
>> ("hwpoison, memory_hotplug: allow hwpoisoned pages to be offlined")
>> to handle the LRU case and the unmapping of the page if needed.
>> So, this is just a follow-up cleanup.
>
> I suspect the above paragraph is adding more confusion than necessary.
> I
> would just drop it.
Fair enough, I will drop it.
> The main question here is. Do we want to migrate as much as possible or
> do we want to be conservative and bail out early. The later could be an
> advantage if the next attempt could fail the whole operation because
> the
> impact of the failed operation would be somehow reduced. The former
> should be better for throughput because easily done stuff is done
> first.
>
> I would go with the throuput because our failure mode is to bail out
> much earlier - even before we try to migrate. Even though the detection
> is not perfect it works reasonably well for most usecases.
I agree here.
I think it is better to do as much work as possible at once.
> you really want to keep this branch. You just do not want to bail out.
> We want to know about pages which fail to isolate and you definitely do
> not want to keep the reference elevated behind. not_managed stuff can
> go
> away.
Yeah, I just realized when I sent it.
I will fix it up in v2.
Thanks
next prev parent reply other threads:[~2018-12-11 12:22 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-11 8:50 [PATCH] mm, memory_hotplug: Don't bail out in do_migrate_range prematurely Oscar Salvador
2018-12-11 8:57 ` osalvador
2018-12-11 9:37 ` David Hildenbrand
2018-12-11 9:35 ` David Hildenbrand
2018-12-11 10:20 ` Michal Hocko
2018-12-11 10:35 ` David Hildenbrand
2018-12-11 10:18 ` Michal Hocko
2018-12-11 12:22 ` osalvador [this message]
2018-12-11 12:52 ` Michal Hocko
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=6009dea8a638aaa5b88088a117297edf@suse.de \
--to=osalvador@suse.de \
--cc=akpm@linux-foundation.org \
--cc=dan.j.williams@gmail.com \
--cc=david@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=pasha.tatashin@soleen.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).