LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Michal Hocko <mhocko@suse.com>
To: Oscar Salvador <osalvador@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	David Hildenbrand <david@redhat.com>,
	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: Tue, 13 Apr 2021 15:40:18 +0200
Message-ID: <YHWfQjegLi4xekhM@dhcp22.suse.cz> (raw)
In-Reply-To: <20210413104747.12177-6-osalvador@suse.de>

On Tue 13-04-21 12:47:45, Oscar Salvador wrote:
> alloc_contig_range will fail if it ever sees a HugeTLB page within the
> range we are trying to allocate, even when that page is free and can be
> easily reallocated.
> This has proved to be problematic for some users of alloc_contic_range,
> e.g: CMA and virtio-mem, where those would fail the call even when those
> pages lay in ZONE_MOVABLE and are free.
> 
> We can do better by trying to replace such page.
> 
> Free hugepages are tricky to handle so as to no userspace application
> notices disruption, we need to replace the current free hugepage with
> a new one.
> 
> In order to do that, a new function called alloc_and_dissolve_huge_page
> is introduced.
> This function will first try to get a new fresh hugepage, and if it
> succeeds, it will replace the old one in the free hugepage pool.
> 
> The free page replacement is done under hugetlb_lock, so no external
> users of hugetlb will notice the change.
> To allocate the new huge page, we use alloc_buddy_huge_page(), so we
> do not have to deal with any counters, and prep_new_huge_page() is not
> called. This is valulable because in case we need to free the new page,
> we only need to call __free_pages().
> 
> Once we know that the page to be replaced is a genuine 0-refcounted
> huge page, we remove the old page from the freelist by remove_hugetlb_page().
> Then, we can call __prep_new_huge_page() and __prep_account_new_huge_page()
> for the new huge page to properly initialize it and increment the
> hstate->nr_huge_pages counter (previously decremented by
> remove_hugetlb_page()).
> Once done, the page is enqueued by enqueue_huge_page() and it is ready
> to be used.
> 
> There is one tricky case when
> page's refcount is 0 because it is in the process of being released.
> A missing PageHugeFreed bit will tell us that freeing is in flight so
> we retry after dropping the hugetlb_lock. The race window should be
> small and the next retry should make a forward progress.
> 
> E.g:
> 
> CPU0				CPU1
> free_huge_page()		isolate_or_dissolve_huge_page
> 				  PageHuge() == T
> 				  alloc_and_dissolve_huge_page
> 				    alloc_buddy_huge_page()
> 				    spin_lock_irq(hugetlb_lock)
> 				    // PageHuge() && !PageHugeFreed &&
> 				    // !PageCount()
> 				    spin_unlock_irq(hugetlb_lock)
>   spin_lock_irq(hugetlb_lock)
>   1) update_and_free_page
>        PageHuge() == F
>        __free_pages()
>   2) enqueue_huge_page
>        SetPageHugeFreed()
>   spin_unlock(&hugetlb_lock)
> 				  spin_lock_irq(hugetlb_lock)
>                                    1) PageHuge() == F (freed by case#1 from CPU0)
> 				   2) PageHuge() == T
>                                        PageHugeFreed() == T
>                                        - proceed with replacing the page
> 
> In the case above we retry as the window race is quite small and we have high
> chances to succeed next time.
> 
> With regard to the allocation, we restrict it to the node the page belongs
> to with __GFP_THISNODE, meaning we do not fallback on other node's zones.
> 
> Note that gigantic hugetlb pages are fenced off since there is a cyclic
> dependency between them and alloc_contig_range.
> 
> Signed-off-by: Oscar Salvador <osalvador@suse.de>

Acked-by: Michal Hocko <mhocko@suse.com>

One minor nit below
[...]

> +		/*
> +		 * 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);

I think it would help to put something like the following into the
comment above this really strange construct.

		/*
		 * new_page needs to be initialized with the standard
		 * hugetlb state. This is normally done by
		 * prep_new_huge_page but that takes hugetlb_lock which
		 * is already held so we need to open code it here.
		 * Reference count trick is needed because allocator
		 * gives us referenced page but the pool requires pages
		 * with 0 refcount.
		 */

> +		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);

-- 
Michal Hocko
SUSE Labs

  reply index

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 [this message]
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
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=YHWfQjegLi4xekhM@dhcp22.suse.cz \
    --to=mhocko@suse.com \
    --cc=akpm@linux-foundation.org \
    --cc=david@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lore.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git