From: David Hildenbrand <david@redhat.com>
To: Oscar Salvador <osalvador@suse.de>,
Andrew Morton <akpm@linux-foundation.org>
Cc: Mike Kravetz <mike.kravetz@oracle.com>,
Vlastimil Babka <vbabka@suse.cz>,
Michal Hocko <mhocko@kernel.org>,
Muchun Song <songmuchun@bytedance.com>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 5/7] mm: Make alloc_contig_range handle free hugetlb pages
Date: Wed, 14 Apr 2021 14:26:21 +0200 [thread overview]
Message-ID: <71cce295-91d3-21d8-5075-04a2e828d0f2@redhat.com> (raw)
In-Reply-To: <20210413104747.12177-6-osalvador@suse.de>
> +static inline int isolate_or_dissolve_huge_page(struct page *page)
> +{
> + return -ENOMEM;
Without CONFIG_HUGETLB_PAGE, there is no way someone could possible pass
in something valid. Although it doesn't matter too much, -EINVAL or
similar sounds a bit better.
> +}
> +
> static inline struct page *alloc_huge_page(struct vm_area_struct *vma,
> unsigned long addr,
> int avoid_reserve)
> diff --git a/mm/compaction.c b/mm/compaction.c
> index eeba4668c22c..89426b6d1ea3 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -788,7 +788,7 @@ static bool too_many_isolated(pg_data_t *pgdat)
> * Isolate all pages that can be migrated from the range specified by
> * [low_pfn, end_pfn). The range is expected to be within same pageblock.
> * Returns errno, like -EAGAIN or -EINTR in case e.g signal pending or congestion,
> - * or 0.
> + * -ENOMEM in case we could not allocate a page, or 0.
> * cc->migrate_pfn will contain the next pfn to scan (which may be both less,
> * equal to or more that end_pfn).
> *
> @@ -809,6 +809,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> bool skip_on_failure = false;
> unsigned long next_skip_pfn = 0;
> bool skip_updated = false;
> + bool fatal_error = false;
Can't we use "ret == -ENOMEM" instead of fatal_error?
> int ret = 0;
>
> cc->migrate_pfn = low_pfn;
> @@ -907,6 +908,33 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> valid_page = page;
> }
>
> + if (PageHuge(page) && cc->alloc_contig) {
> + ret = isolate_or_dissolve_huge_page(page);
> +
> + /*
> + * Fail isolation in case isolate_or_dissolve_huge_page
> + * reports an error. In case of -ENOMEM, abort right away.
> + */
> + if (ret < 0) {
> + /*
> + * Do not report -EBUSY down the chain.
> + */
> + if (ret == -ENOMEM)
> + fatal_error = true;
> + else
> + ret = 0;
> + low_pfn += (1UL << compound_order(page)) - 1;
> + goto isolate_fail;
> + }
> +
> + /*
> + * Ok, the hugepage was dissolved. Now these pages are
> + * Buddy and cannot be re-allocated because they are
> + * isolated. Fall-through as the check below handles
> + * Buddy pages.
> + */
> + }
> +
> /*
> * Skip if free. We read page order here without zone lock
> * which is generally unsafe, but the race window is small and
> @@ -1066,7 +1094,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> put_page(page);
>
> isolate_fail:
> - if (!skip_on_failure)
> + if (!skip_on_failure && !fatal_error)
> continue;
>
> /*
> @@ -1092,6 +1120,9 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> */
> next_skip_pfn += 1UL << cc->order;
> }
> +
> + if (fatal_error)
> + break;
> }
>
> /*
> @@ -1145,7 +1176,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
> * @end_pfn: The one-past-last PFN.
> *
> * Returns errno, like -EAGAIN or -EINTR in case e.g signal pending or congestion,
> - * or 0.
> + * -ENOMEM in case we could not allocate a page, or 0.
> */
> int
> isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn,
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 0607b2b71ac6..4a664d6e82c1 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2266,6 +2266,121 @@ static void restore_reserve_on_error(struct hstate *h,
> }
> }
>
> +/*
> + * alloc_and_dissolve_huge_page - Allocate a new page and dissolve the old one
> + * @h: struct hstate old page belongs to
> + * @old_page: Old page to dissolve
> + * Returns 0 on success, otherwise negated error.
> + */
> +
> +static int alloc_and_dissolve_huge_page(struct hstate *h, struct page *old_page)
> +{
> + gfp_t gfp_mask = htlb_alloc_mask(h) | __GFP_THISNODE;
> + int nid = page_to_nid(old_page);
> + struct page *new_page;
> + int ret = 0;
> +
> + /*
> + * Before dissolving the page, we need to allocate a new one for the
> + * pool to remain stable. Using alloc_buddy_huge_page() allows us to
> + * not having to deal with prep_new_page() and avoids dealing of any
> + * counters. This simplifies and let us do the whole thing under the
> + * lock.
> + */
> + new_page = alloc_buddy_huge_page(h, gfp_mask, nid, NULL, NULL);
> + if (!new_page)
> + return -ENOMEM;
> +
> +retry:
> + spin_lock_irq(&hugetlb_lock);
> + if (!PageHuge(old_page)) {
> + /*
> + * Freed from under us. Drop new_page too.
> + */
> + goto free_new;
> + } else if (page_count(old_page)) {
> + /*
> + * Someone has grabbed the page, fail for now.
> + */
> + ret = -EBUSY;
> + goto free_new;
> + } else if (!HPageFreed(old_page)) {
> + /*
> + * Page's refcount is 0 but it has not been enqueued in the
> + * freelist yet. Race window is small, so we can succeed here if
> + * we retry.
> + */
> + spin_unlock_irq(&hugetlb_lock);
> + cond_resched();
> + goto retry;
> + } else {
> + /*
> + * Ok, old_page is still a genuine free hugepage. Remove it from
> + * the freelist and decrease the counters. These will be
> + * incremented again when calling __prep_account_new_huge_page()
> + * and enqueue_huge_page() for new_page. The counters will remain
> + * stable since this happens under the lock.
> + */
> + remove_hugetlb_page(h, old_page, false);
> +
> + /*
> + * Call __prep_new_huge_page() to construct the hugetlb page, and
> + * enqueue it then to place it in the freelists. After this,
> + * counters are back on track. Free hugepages have a refcount of 0,
> + * so we need to decrease new_page's count as well.
> + */
> + __prep_new_huge_page(new_page);
> + __prep_account_new_huge_page(h, nid);
> + page_ref_dec(new_page);
> + enqueue_huge_page(h, new_page);
> +
> + /*
> + * Pages have been replaced, we can safely free the old one.
> + */
> + spin_unlock_irq(&hugetlb_lock);
> + update_and_free_page(h, old_page);
> + }
> +
> + return ret;
> +
> +free_new:
> + spin_unlock_irq(&hugetlb_lock);
> + __free_pages(new_page, huge_page_order(h));
> +
> + return ret;
> +}
> +
> +int isolate_or_dissolve_huge_page(struct page *page)
> +{
> + struct hstate *h;
> + struct page *head;
> +
> + /*
> + * The page might have been dissolved from under our feet, so make sure
> + * to carefully check the state under the lock.
> + * Return success when racing as if we dissolved the page ourselves.
> + */
> + spin_lock_irq(&hugetlb_lock);
> + if (PageHuge(page)) {
> + head = compound_head(page);
> + h = page_hstate(head);
> + } else {
> + spin_unlock(&hugetlb_lock);
> + return 0;
> + }
> + spin_unlock_irq(&hugetlb_lock);
> +
> + /*
> + * Fence off gigantic pages as there is a cyclic dependency between
> + * alloc_contig_range and them. Return -ENOME as this has the effect
s/-ENOME/-ENOMEM/
> + * of bailing out right away without further retrying.
> + */
> + if (hstate_is_gigantic(h))
> + return -ENOMEM;
> +
> + return alloc_and_dissolve_huge_page(h, head);
> +}
> +
> struct page *alloc_huge_page(struct vm_area_struct *vma,
> unsigned long addr, int avoid_reserve)
> {
>
Complicated stuff, but looks good to me.
--
Thanks,
David / dhildenb
next prev parent reply other threads:[~2021-04-14 12:26 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-13 10:47 [PATCH v7 0/7] Make alloc_contig_range handle Hugetlb pages Oscar Salvador
2021-04-13 10:47 ` [PATCH v7 1/7] mm,page_alloc: Bail out earlier on -ENOMEM in alloc_contig_migrate_range Oscar Salvador
2021-04-13 17:00 ` Mike Kravetz
2021-04-13 10:47 ` [PATCH v7 2/7] mm,compaction: Let isolate_migratepages_{range,block} return error codes Oscar Salvador
2021-04-13 17:52 ` Mike Kravetz
2021-04-14 11:54 ` David Hildenbrand
2021-04-15 8:42 ` Oscar Salvador
2021-04-13 10:47 ` [PATCH v7 3/7] mm,hugetlb: Clear HPageFreed outside of the lock Oscar Salvador
2021-04-13 13:23 ` Michal Hocko
2021-04-13 21:19 ` Mike Kravetz
2021-04-14 6:04 ` Michal Hocko
2021-04-14 7:41 ` Oscar Salvador
2021-04-14 8:28 ` Michal Hocko
2021-04-14 10:01 ` Oscar Salvador
2021-04-14 10:03 ` Oscar Salvador
2021-04-14 10:32 ` Michal Hocko
2021-04-14 10:49 ` Oscar Salvador
2021-04-14 11:09 ` Michal Hocko
2021-04-14 12:02 ` David Hildenbrand
2021-04-14 16:45 ` Mike Kravetz
2021-04-13 10:47 ` [PATCH v7 4/7] mm,hugetlb: Split prep_new_huge_page functionality Oscar Salvador
2021-04-13 13:24 ` Michal Hocko
2021-04-13 13:26 ` Michal Hocko
2021-04-13 21:33 ` Mike Kravetz
2021-04-14 4:59 ` Oscar Salvador
2021-04-14 12:15 ` David Hildenbrand
2021-04-14 17:03 ` Mike Kravetz
2021-04-13 10:47 ` [PATCH v7 5/7] mm: Make alloc_contig_range handle free hugetlb pages Oscar Salvador
2021-04-13 13:40 ` Michal Hocko
2021-04-15 8:29 ` Oscar Salvador
2021-04-13 22:29 ` Mike Kravetz
2021-04-14 4:54 ` Oscar Salvador
2021-04-14 12:26 ` David Hildenbrand [this message]
2021-04-15 8:28 ` Oscar Salvador
2021-04-13 10:47 ` [PATCH v7 6/7] mm: Make alloc_contig_range handle in-use " Oscar Salvador
2021-04-13 22:48 ` Mike Kravetz
2021-04-14 4:52 ` Oscar Salvador
2021-04-14 17:07 ` Mike Kravetz
2021-04-13 10:47 ` [PATCH v7 7/7] mm,page_alloc: Drop unnecessary checks from pfn_range_valid_contig Oscar Salvador
2021-04-13 22:53 ` 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=71cce295-91d3-21d8-5075-04a2e828d0f2@redhat.com \
--to=david@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=mike.kravetz@oracle.com \
--cc=osalvador@suse.de \
--cc=songmuchun@bytedance.com \
--cc=vbabka@suse.cz \
/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).