From: David Hildenbrand <david@redhat.com>
To: Michal Hocko <mhocko@kernel.org>, linux-mm@kvack.org
Cc: Andrew Morton <akpm@linux-foundation.org>,
Oscar Salvador <OSalvador@suse.com>,
Pavel Tatashin <pasha.tatashin@oracle.com>,
LKML <linux-kernel@vger.kernel.org>,
Michal Hocko <mhocko@suse.com>
Subject: Re: [RFC PATCH 2/3] mm, memory_hotplug: deobfuscate migration part of offlining
Date: Tue, 20 Nov 2018 15:26:43 +0100 [thread overview]
Message-ID: <f25bfa30-96cf-799c-6885-86a3a537a977@redhat.com> (raw)
In-Reply-To: <20181120134323.13007-3-mhocko@kernel.org>
On 20.11.18 14:43, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> Memory migration might fail during offlining and we keep retrying in
> that case. This is currently obfuscate by goto retry loop. The code
> is hard to follow and as a result it is even suboptimal becase each
> retry round scans the full range from start_pfn even though we have
> successfully scanned/migrated [start_pfn, pfn] range already. This
> is all only because check_pages_isolated failure has to rescan the full
> range again.
>
> De-obfuscate the migration retry loop by promoting it to a real for
> loop. In fact remove the goto altogether by making it a proper double
> loop (yeah, gotos are nasty in this specific case). In the end we
> will get a slightly more optimal code which is better readable.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/memory_hotplug.c | 60 +++++++++++++++++++++++----------------------
> 1 file changed, 31 insertions(+), 29 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 6263c8cd4491..9cd161db3061 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1591,38 +1591,40 @@ static int __ref __offline_pages(unsigned long start_pfn,
> goto failed_removal_isolated;
> }
>
> - pfn = start_pfn;
> -repeat:
> - /* start memory hot removal */
> - ret = -EINTR;
> - if (signal_pending(current)) {
> - reason = "signal backoff";
> - goto failed_removal_isolated;
> - }
> + do {
> + for (pfn = start_pfn; pfn;)
> + {
{ on a new line looks weird.
> + /* start memory hot removal */
> + ret = -EINTR;
I think we can move that into the "if (signal_pending(current))"
(if my eyes are not wrong, this will not be touched otherwise)
> + if (signal_pending(current)) {
> + reason = "signal backoff";
> + goto failed_removal_isolated;
> + }
>
> - cond_resched();
> - lru_add_drain_all();
> - drain_all_pages(zone);
> + cond_resched();
> + lru_add_drain_all();
> + drain_all_pages(zone);
>
> - pfn = scan_movable_pages(start_pfn, end_pfn);
> - if (pfn) { /* We have movable pages */
> - ret = do_migrate_range(pfn, end_pfn);
> - goto repeat;
> - }
> + pfn = scan_movable_pages(pfn, end_pfn);
> + if (pfn) {
> + /* TODO fatal migration failures should bail out */
> + do_migrate_range(pfn, end_pfn);
Right, that return value was always ignored.
> + }
> + }
> +
> + /*
> + * dissolve free hugepages in the memory block before doing offlining
> + * actually in order to make hugetlbfs's object counting consistent.
> + */
> + ret = dissolve_free_huge_pages(start_pfn, end_pfn);
> + if (ret) {
> + reason = "failure to dissolve huge pages";
> + goto failed_removal_isolated;
> + }
> + /* check again */
> + offlined_pages = check_pages_isolated(start_pfn, end_pfn);
> + } while (offlined_pages < 0);
>
> - /*
> - * dissolve free hugepages in the memory block before doing offlining
> - * actually in order to make hugetlbfs's object counting consistent.
> - */
> - ret = dissolve_free_huge_pages(start_pfn, end_pfn);
> - if (ret) {
> - reason = "failure to dissolve huge pages";
> - goto failed_removal_isolated;
> - }
> - /* check again */
> - offlined_pages = check_pages_isolated(start_pfn, end_pfn);
> - if (offlined_pages < 0)
> - goto repeat;
> pr_info("Offlined Pages %ld\n", offlined_pages);
> /* Ok, all of our target is isolated.
> We cannot do rollback at this point. */
>
Looks much better to me.
--
Thanks,
David / dhildenb
next prev parent reply other threads:[~2018-11-20 14:26 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-20 13:43 [RFC PATCH 0/3] few memory offlining enhancements Michal Hocko
2018-11-20 13:43 ` [RFC PATCH 1/3] mm, memory_hotplug: try to migrate full section worth of pages Michal Hocko
2018-11-20 14:18 ` David Hildenbrand
2018-11-20 14:25 ` Michal Hocko
2018-11-20 14:27 ` David Hildenbrand
2018-11-20 14:33 ` Pavel Tatashin
2018-11-20 14:51 ` osalvador
2018-11-20 15:00 ` Michal Hocko
2018-11-20 13:43 ` [RFC PATCH 2/3] mm, memory_hotplug: deobfuscate migration part of offlining Michal Hocko
2018-11-20 14:26 ` David Hildenbrand [this message]
2018-11-20 14:34 ` Michal Hocko
2018-11-20 14:34 ` David Hildenbrand
2018-11-20 15:13 ` osalvador
2018-11-20 15:18 ` Michal Hocko
2018-11-20 13:43 ` [RFC PATCH 3/3] mm, fault_around: do not take a reference to a locked page Michal Hocko
2018-11-20 14:07 ` Kirill A. Shutemov
2018-11-20 14:12 ` Michal Hocko
2018-11-20 14:17 ` Kirill A. Shutemov
2018-11-20 14:25 ` Michal Hocko
2018-11-21 4:51 ` William Kucharski
2018-11-21 7:07 ` Michal Hocko
2018-11-20 14:33 ` David Hildenbrand
2018-11-21 1:47 ` Hugh Dickins
2018-11-21 7:11 ` Michal Hocko
2018-11-22 2:27 ` Hugh Dickins
2018-11-22 9:05 ` Michal Hocko
2018-11-23 20:22 ` Hugh Dickins
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=f25bfa30-96cf-799c-6885-86a3a537a977@redhat.com \
--to=david@redhat.com \
--cc=OSalvador@suse.com \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=mhocko@suse.com \
--cc=pasha.tatashin@oracle.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).