[PATCH 0/2] Make alloc_contig_range handle Hugetlb pages

[PATCH 0/2] Make alloc_contig_range handle Hugetlb pages

[Oscar Salvador]

Hi, 

this is the new version of [1].

 RFC -> v1:
 - Drop RFC
 - Addressed feedback from David and Mike
 - Fence off gigantic pages as there is a cyclic dependency between
   them and alloc_contig_range
 - Re-organize the code to make race-window smaller and to put
   all details in hugetlb code
 - Drop nodemask initialization. First a node will be tried and then we
   will back to other nodes containing memory (N_MEMORY). Details in
   patch#1's changelog
 - Count new page as surplus in case we failed to dissolve the old page
   and the new one. Details in patch#1.

I already have a patch that uses the new alloc_and_dissolve() from hotplug
code, but I decided to leave that patch out until this patchset is settled.

[1] https://patchwork.kernel.org/project/linux-mm/list/?series=429833

Oscar Salvador (2):
  mm: Make alloc_contig_range handle free hugetlb pages
  mm: Make alloc_contig_range handle in-use hugetlb pages

 include/linux/hugetlb.h |  7 ++++
 mm/compaction.c         | 21 +++++++++++
 mm/hugetlb.c            | 93 +++++++++++++++++++++++++++++++++++++++++++++++++
 mm/vmscan.c             |  5 +--
 4 files changed, 124 insertions(+), 2 deletions(-)

-- 
2.16.3

[PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Oscar Salvador]

Free hugetlb pages 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 hugetlb page, and if it
succeeds, it will dissolve the old one.

With regard to the allocation, since we do not know whether the old page
was allocated on a specific node on request, the node the old page belongs
to will be tried first, and then we will fallback to all nodes containing
memory (N_MEMORY).

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>
---
 include/linux/hugetlb.h |  6 ++++
 mm/compaction.c         | 12 +++++++
 mm/hugetlb.c            | 91 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index b5807f23caf8..72352d718829 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -505,6 +505,7 @@ struct huge_bootmem_page {
 	struct hstate *hstate;
 };
 
+bool isolate_or_dissolve_huge_page(struct page *page);
 struct page *alloc_huge_page(struct vm_area_struct *vma,
 				unsigned long addr, int avoid_reserve);
 struct page *alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
@@ -775,6 +776,11 @@ void set_page_huge_active(struct page *page);
 #else	/* CONFIG_HUGETLB_PAGE */
 struct hstate {};
 
+static inline bool isolate_or_dissolve_huge_page(struct page *page)
+{
+	return false;
+}
+
 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 190ccdaa6c19..d52506ed9db7 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -905,6 +905,18 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 			valid_page = page;
 		}
 
+		if (PageHuge(page) && cc->alloc_contig) {
+			if (!isolate_or_dissolve_huge_page(page))
+				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
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 4bdb58ab14cb..b78926bca60a 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2294,6 +2294,97 @@ static void restore_reserve_on_error(struct hstate *h,
 	}
 }
 
+static bool alloc_and_dissolve_huge_page(struct hstate *h, struct page *page)
+{
+	gfp_t gfp_mask = htlb_alloc_mask(h);
+	nodemask_t *nmask = &node_states[N_MEMORY];
+	struct page *new_page;
+	bool ret = false;
+	int nid;
+
+	spin_lock(&hugetlb_lock);
+	/*
+	 * Check one more time to make race-window smaller.
+	 */
+	if (!PageHuge(page)) {
+		/*
+		 * Dissolved from under our feet.
+		 */
+		spin_unlock(&hugetlb_lock);
+		return true;
+	}
+
+	nid = page_to_nid(page);
+	spin_unlock(&hugetlb_lock);
+
+	/*
+	 * Before dissolving the page, we need to allocate a new one,
+	 * so the pool remains stable.
+	 */
+	new_page = alloc_fresh_huge_page(h, gfp_mask, nid, nmask, NULL);
+	if (new_page) {
+		/*
+		 * Ok, we got a new free hugepage to replace this one. Try to
+		 * dissolve the old page.
+		 */
+		if (!dissolve_free_huge_page(page)) {
+			ret = true;
+		} else if (dissolve_free_huge_page(new_page)) {
+			/*
+			 * Seems the old page could not be dissolved, so try to
+			 * dissolve the freshly allocated page. If that fails
+			 * too, let us count the new page as a surplus. Doing so
+			 * allows the pool to be re-balanced when pages are freed
+			 * instead of enqueued again.
+			 */
+			spin_lock(&hugetlb_lock);
+			h->surplus_huge_pages++;
+			h->surplus_huge_pages_node[nid]++;
+			spin_unlock(&hugetlb_lock);
+		}
+		/*
+		 * Free it into the hugepage allocator
+		 */
+		put_page(new_page);
+	}
+
+	return ret;
+}
+
+bool isolate_or_dissolve_huge_page(struct page *page)
+{
+	struct hstate *h = NULL;
+	struct page *head;
+	bool ret = false;
+
+	spin_lock(&hugetlb_lock);
+	if (PageHuge(page)) {
+		head = compound_head(page);
+		h = page_hstate(head);
+	}
+	spin_unlock(&hugetlb_lock);
+
+	if (!h)
+		/*
+		 * The page might have been dissolved from under our feet.
+		 * If that is the case, return success as if we dissolved it
+		 * ourselves.
+		 */
+		return true;
+
+	if (hstate_is_gigantic(h))
+		/*
+		 * Fence off gigantic pages as there is a cyclic dependency
+		 * between alloc_contig_range and them.
+		 */
+		return ret;
+
+	if(!page_count(head) && alloc_and_dissolve_huge_page(h, head))
+		ret = true;
+
+	return ret;
+}
+
 struct page *alloc_huge_page(struct vm_area_struct *vma,
 				    unsigned long addr, int avoid_reserve)
 {
-- 
2.16.3

[PATCH 2/2] mm: Make alloc_contig_range handle in-use hugetlb pages

[Oscar Salvador]

In-use hugetlb pages can be migrated as any other page (LRU
and Movable), so let alloc_contig_range handle them.

All we need is to succesfully isolate such page.

Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 include/linux/hugetlb.h |  5 +++--
 mm/compaction.c         | 11 ++++++++++-
 mm/hugetlb.c            |  6 ++++--
 mm/vmscan.c             |  5 +++--
 4 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 72352d718829..8c17d0dbc87c 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -505,7 +505,7 @@ struct huge_bootmem_page {
 	struct hstate *hstate;
 };
 
-bool isolate_or_dissolve_huge_page(struct page *page);
+bool isolate_or_dissolve_huge_page(struct page *page, struct list_head *list);
 struct page *alloc_huge_page(struct vm_area_struct *vma,
 				unsigned long addr, int avoid_reserve);
 struct page *alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
@@ -776,7 +776,8 @@ void set_page_huge_active(struct page *page);
 #else	/* CONFIG_HUGETLB_PAGE */
 struct hstate {};
 
-static inline bool isolate_or_dissolve_huge_page(struct page *page)
+static inline bool isolate_or_dissolve_huge_page(struct page *page,
+						 struct list_head *list)
 {
 	return false;
 }
diff --git a/mm/compaction.c b/mm/compaction.c
index d52506ed9db7..55a41a9228a9 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -906,9 +906,17 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 		}
 
 		if (PageHuge(page) && cc->alloc_contig) {
-			if (!isolate_or_dissolve_huge_page(page))
+			if (!isolate_or_dissolve_huge_page(page, &cc->migratepages))
 				goto isolate_fail;
 
+			if (PageHuge(page)) {
+				/*
+				 * Hugepage was succesfully isolated.
+				 */
+				low_pfn += compound_nr(page) - 1;
+				goto isolate_success_no_list;
+			}
+
 			/*
 			 * Ok, the hugepage was dissolved. Now these pages are
 			 * Buddy and cannot be re-allocated because they are
@@ -1053,6 +1061,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
 
 isolate_success:
 		list_add(&page->lru, &cc->migratepages);
+isolate_success_no_list:
 		cc->nr_migratepages += compound_nr(page);
 		nr_isolated += compound_nr(page);
 
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index b78926bca60a..9fa678d13c68 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -2351,7 +2351,7 @@ static bool alloc_and_dissolve_huge_page(struct hstate *h, struct page *page)
 	return ret;
 }
 
-bool isolate_or_dissolve_huge_page(struct page *page)
+bool isolate_or_dissolve_huge_page(struct page *page, struct list_head *list)
 {
 	struct hstate *h = NULL;
 	struct page *head;
@@ -2379,7 +2379,9 @@ bool isolate_or_dissolve_huge_page(struct page *page)
 		 */
 		return ret;
 
-	if(!page_count(head) && alloc_and_dissolve_huge_page(h, head))
+	if (page_count(head) && isolate_huge_page(head, list))
+		ret = true;
+	else if(!page_count(head) && alloc_and_dissolve_huge_page(h, head))
 		ret = true;
 
 	return ret;
diff --git a/mm/vmscan.c b/mm/vmscan.c
index b1b574ad199d..0803adca4469 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1506,8 +1506,9 @@ unsigned int reclaim_clean_pages_from_list(struct zone *zone,
 	LIST_HEAD(clean_pages);
 
 	list_for_each_entry_safe(page, next, page_list, lru) {
-		if (page_is_file_lru(page) && !PageDirty(page) &&
-		    !__PageMovable(page) && !PageUnevictable(page)) {
+		if (!PageHuge(page) && page_is_file_lru(page) &&
+		    !PageDirty(page) && !__PageMovable(page) &&
+		    !PageUnevictable(page)) {
 			ClearPageActive(page);
 			list_move(&page->lru, &clean_pages);
 		}
-- 
2.16.3

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Michal Hocko]

On Wed 17-02-21 11:08:15, Oscar Salvador wrote:
> Free hugetlb pages 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 hugetlb page, and if it
> succeeds, it will dissolve the old one.
> 
> With regard to the allocation, since we do not know whether the old page
> was allocated on a specific node on request, the node the old page belongs
> to will be tried first, and then we will fallback to all nodes containing
> memory (N_MEMORY).

I do not think fallback to a different zone is ok. If yes then this
really requires a very good reasoning. alloc_contig_range is an
optimistic allocation interface at best and it shouldn't break carefully
node aware preallocation done by administrator.

> Note that gigantic hugetlb pages are fenced off since there is a cyclic
> dependency between them and alloc_contig_range.

Why do we need/want to do all this in the first place?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] mm: Make alloc_contig_range handle in-use hugetlb pages

[Michal Hocko]

On Wed 17-02-21 11:08:16, Oscar Salvador wrote:
> In-use hugetlb pages can be migrated as any other page (LRU
> and Movable), so let alloc_contig_range handle them.
> 
> All we need is to succesfully isolate such page.

Again, this is missing a problem statement and a justification why we
want/need this.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[David Hildenbrand]

On 17.02.21 14:30, Michal Hocko wrote:
> On Wed 17-02-21 11:08:15, Oscar Salvador wrote:
>> Free hugetlb pages 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 hugetlb page, and if it
>> succeeds, it will dissolve the old one.
>>
>> With regard to the allocation, since we do not know whether the old page
>> was allocated on a specific node on request, the node the old page belongs
>> to will be tried first, and then we will fallback to all nodes containing
>> memory (N_MEMORY).
> 
> I do not think fallback to a different zone is ok. If yes then this
> really requires a very good reasoning. alloc_contig_range is an
> optimistic allocation interface at best and it shouldn't break carefully
> node aware preallocation done by administrator.

What does memory offlining do when migrating in-use hugetlbfs pages? 
Does it always keep the node?

I think keeping the node is the easiest/simplest approach for now.

> 
>> Note that gigantic hugetlb pages are fenced off since there is a cyclic
>> dependency between them and alloc_contig_range.
> 
> Why do we need/want to do all this in the first place?

cma and virtio-mem (especially on ZONE_MOVABLE) really want to handle 
hugetlbfs pages.

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Oscar Salvador]

On Wed, Feb 17, 2021 at 02:30:43PM +0100, Michal Hocko wrote:
> On Wed 17-02-21 11:08:15, Oscar Salvador wrote:
> I do not think fallback to a different zone is ok. If yes then this
> really requires a very good reasoning. alloc_contig_range is an
> optimistic allocation interface at best and it shouldn't break carefully
> node aware preallocation done by administrator.

Yeah, previous version (RFC) was more careful with that.
I somehow thought that it might be ok to fallback to other nodes in case
we failed to allocate on the preferred nid.

I will get RFC handling back wrt. allocation once I gather more feedback.

> 
> > Note that gigantic hugetlb pages are fenced off since there is a cyclic
> > dependency between them and alloc_contig_range.
> 
> Why do we need/want to do all this in the first place?

When trying to allocate a memory chunk with alloc_contig_range, it will fail
if it ever sees a Hugetlb page because isolate_migratepages_range() does not
"recognize" those for migration (or to put it different, does not know about
them).

Given that HugeTLB pages can be migrated or dissolved if free, it makes sense
to enable isolate_migratepages_range() to recognize HugeTLB pages in order
to handle them, as it currently does with LRU and Movable pages.

-- 
Oscar Salvador
SUSE L3

Re: [PATCH 2/2] mm: Make alloc_contig_range handle in-use hugetlb pages

[Oscar Salvador]

On Wed, Feb 17, 2021 at 02:36:31PM +0100, Michal Hocko wrote:
> On Wed 17-02-21 11:08:16, Oscar Salvador wrote:
> > In-use hugetlb pages can be migrated as any other page (LRU
> > and Movable), so let alloc_contig_range handle them.
> > 
> > All we need is to succesfully isolate such page.
> 
> Again, this is missing a problem statement and a justification why we
> want/need this.

Heh, I was poor in words.

"alloc_contig_range() will fail miserably if it finds a HugeTLB page within
 the range without a chance to handle them. Since HugeTLB pages can be migrated
 as any other page (LRU and Movable), it does not make sense to bail out.
 Enable the interface to recognize in-use HugeTLB pages and have a chance
 to migrate them"

What about something along those lines?


-- 
Oscar Salvador
SUSE L3

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Michal Hocko]

On Wed 17-02-21 14:36:47, David Hildenbrand wrote:
> On 17.02.21 14:30, Michal Hocko wrote:
> > On Wed 17-02-21 11:08:15, Oscar Salvador wrote:
> > > Free hugetlb pages 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 hugetlb page, and if it
> > > succeeds, it will dissolve the old one.
> > > 
> > > With regard to the allocation, since we do not know whether the old page
> > > was allocated on a specific node on request, the node the old page belongs
> > > to will be tried first, and then we will fallback to all nodes containing
> > > memory (N_MEMORY).
> > 
> > I do not think fallback to a different zone is ok. If yes then this
> > really requires a very good reasoning. alloc_contig_range is an
> > optimistic allocation interface at best and it shouldn't break carefully
> > node aware preallocation done by administrator.
> 
> What does memory offlining do when migrating in-use hugetlbfs pages? Does it
> always keep the node?

No it will break the node pool. The reasoning behind that is that
offlining is an explicit request from the userspace and it is expected
to break affinities because it is a destructive action from the memory
capacity point of view. It is impossible to have former affinity while
you are cutting the memory off under its user.

> I think keeping the node is the easiest/simplest approach for now.
> 
> > 
> > > Note that gigantic hugetlb pages are fenced off since there is a cyclic
> > > dependency between them and alloc_contig_range.
> > 
> > Why do we need/want to do all this in the first place?
> 
> cma and virtio-mem (especially on ZONE_MOVABLE) really want to handle
> hugetlbfs pages.

Do we have any real life examples? Or does this fall more into, let's
optimize an existing implementation category.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[David Hildenbrand]

On 17.02.21 14:50, Michal Hocko wrote:
> On Wed 17-02-21 14:36:47, David Hildenbrand wrote:
>> On 17.02.21 14:30, Michal Hocko wrote:
>>> On Wed 17-02-21 11:08:15, Oscar Salvador wrote:
>>>> Free hugetlb pages 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 hugetlb page, and if it
>>>> succeeds, it will dissolve the old one.
>>>>
>>>> With regard to the allocation, since we do not know whether the old page
>>>> was allocated on a specific node on request, the node the old page belongs
>>>> to will be tried first, and then we will fallback to all nodes containing
>>>> memory (N_MEMORY).
>>>
>>> I do not think fallback to a different zone is ok. If yes then this
>>> really requires a very good reasoning. alloc_contig_range is an
>>> optimistic allocation interface at best and it shouldn't break carefully
>>> node aware preallocation done by administrator.
>>
>> What does memory offlining do when migrating in-use hugetlbfs pages? Does it
>> always keep the node?
> 
> No it will break the node pool. The reasoning behind that is that
> offlining is an explicit request from the userspace and it is expected

userspace? in 99,9996% it's the hardware that triggers the unplug of a DIMM.

> 
>> I think keeping the node is the easiest/simplest approach for now.
>>
>>>
>>>> Note that gigantic hugetlb pages are fenced off since there is a cyclic
>>>> dependency between them and alloc_contig_range.
>>>
>>> Why do we need/want to do all this in the first place?
>>
>> cma and virtio-mem (especially on ZONE_MOVABLE) really want to handle
>> hugetlbfs pages.
> 
> Do we have any real life examples? Or does this fall more into, let's
> optimize an existing implementation category.
> 

It's a big TODO item I have on my list and I am happy that Oscar is 
looking into it. So yes, I noticed it while working on virtio-mem. It's 
real.

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/2] mm: Make alloc_contig_range handle in-use hugetlb pages

[Michal Hocko]

On Wed 17-02-21 14:46:49, Oscar Salvador wrote:
> On Wed, Feb 17, 2021 at 02:36:31PM +0100, Michal Hocko wrote:
> > On Wed 17-02-21 11:08:16, Oscar Salvador wrote:
> > > In-use hugetlb pages can be migrated as any other page (LRU
> > > and Movable), so let alloc_contig_range handle them.
> > > 
> > > All we need is to succesfully isolate such page.
> > 
> > Again, this is missing a problem statement and a justification why we
> > want/need this.
> 
> Heh, I was poor in words.
> 
> "alloc_contig_range() will fail miserably if it finds a HugeTLB page within
>  the range without a chance to handle them. Since HugeTLB pages can be migrated
>  as any other page (LRU and Movable), it does not make sense to bail out.
>  Enable the interface to recognize in-use HugeTLB pages and have a chance
>  to migrate them"
> 
> What about something along those lines?

Is this a real life problem? I know we _can_ but I do not see any
reasoning _why_ should we care all that much.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Michal Hocko]

On Wed 17-02-21 14:53:37, David Hildenbrand wrote:
> On 17.02.21 14:50, Michal Hocko wrote:
[...]
> > Do we have any real life examples? Or does this fall more into, let's
> > optimize an existing implementation category.
> > 
> 
> It's a big TODO item I have on my list and I am happy that Oscar is looking
> into it. So yes, I noticed it while working on virtio-mem. It's real.

Do not take me wrong, I am not opposing to the functionality. I am
asking for the specific usecase.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[David Hildenbrand]

On 17.02.21 14:59, Michal Hocko wrote:
> On Wed 17-02-21 14:53:37, David Hildenbrand wrote:
>> On 17.02.21 14:50, Michal Hocko wrote:
> [...]
>>> Do we have any real life examples? Or does this fall more into, let's
>>> optimize an existing implementation category.
>>>
>>
>> It's a big TODO item I have on my list and I am happy that Oscar is looking
>> into it. So yes, I noticed it while working on virtio-mem. It's real.
> 
> Do not take me wrong, I am not opposing to the functionality. I am
> asking for the specific usecase.

Makes sense, and a proper motivation should be included in the 
patches/cover letter. So here comes a quick-n-dirty example:


Start a VM with 4G. Hotplug 1G via virtio-mem and online it to 
ZONE_MOVABLE. Allocate 512 huge pages.

[root@localhost ~]# cat /proc/meminfo
MemTotal:        5061512 kB
MemFree:         3319396 kB
MemAvailable:    3457144 kB
...
HugePages_Total:     512
HugePages_Free:      512
HugePages_Rsvd:        0
HugePages_Surp:        0
Hugepagesize:       2048 kB


The huge pages get partially allocate from ZONE_MOVABLE. Try unplugging 
1G via virtio-mem (remember, all ZONE_MOVABLE). Inside the guest:

[  180.058992] alloc_contig_range: [1b8000, 1c0000) PFNs busy
[  180.060531] alloc_contig_range: [1b8000, 1c0000) PFNs busy
[  180.061972] alloc_contig_range: [1b8000, 1c0000) PFNs busy
[  180.063413] alloc_contig_range: [1b8000, 1c0000) PFNs busy
[  180.064838] alloc_contig_range: [1b8000, 1c0000) PFNs busy
[  180.065848] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
[  180.066794] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
[  180.067738] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
[  180.068669] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
[  180.069598] alloc_contig_range: [1bfc00, 1c0000) PFNs busy


I succeed in unplugging 540MB - 484 MB remain blocked by huge pages 
("which did not end up there by pure luck"). These pages are movable 
(and even free!) and can easily be reallocated.

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Michal Hocko]

On Wed 17-02-21 15:08:04, David Hildenbrand wrote:
> On 17.02.21 14:59, Michal Hocko wrote:
> > On Wed 17-02-21 14:53:37, David Hildenbrand wrote:
> > > On 17.02.21 14:50, Michal Hocko wrote:
> > [...]
> > > > Do we have any real life examples? Or does this fall more into, let's
> > > > optimize an existing implementation category.
> > > > 
> > > 
> > > It's a big TODO item I have on my list and I am happy that Oscar is looking
> > > into it. So yes, I noticed it while working on virtio-mem. It's real.
> > 
> > Do not take me wrong, I am not opposing to the functionality. I am
> > asking for the specific usecase.
> 
> Makes sense, and a proper motivation should be included in the patches/cover
> letter. So here comes a quick-n-dirty example:
> 
> 
> Start a VM with 4G. Hotplug 1G via virtio-mem and online it to ZONE_MOVABLE.
> Allocate 512 huge pages.
> 
> [root@localhost ~]# cat /proc/meminfo
> MemTotal:        5061512 kB
> MemFree:         3319396 kB
> MemAvailable:    3457144 kB
> ...
> HugePages_Total:     512
> HugePages_Free:      512
> HugePages_Rsvd:        0
> HugePages_Surp:        0
> Hugepagesize:       2048 kB
> 
> 
> The huge pages get partially allocate from ZONE_MOVABLE. Try unplugging 1G
> via virtio-mem (remember, all ZONE_MOVABLE). Inside the guest:
> 
> [  180.058992] alloc_contig_range: [1b8000, 1c0000) PFNs busy
> [  180.060531] alloc_contig_range: [1b8000, 1c0000) PFNs busy
> [  180.061972] alloc_contig_range: [1b8000, 1c0000) PFNs busy
> [  180.063413] alloc_contig_range: [1b8000, 1c0000) PFNs busy
> [  180.064838] alloc_contig_range: [1b8000, 1c0000) PFNs busy
> [  180.065848] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
> [  180.066794] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
> [  180.067738] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
> [  180.068669] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
> [  180.069598] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
> 
> 
> I succeed in unplugging 540MB - 484 MB remain blocked by huge pages ("which
> did not end up there by pure luck"). These pages are movable (and even
> free!) and can easily be reallocated.

OK, this sounds reasonable.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Oscar Salvador]

On Wed, Feb 17, 2021 at 03:08:04PM +0100, David Hildenbrand wrote:
> On 17.02.21 14:59, Michal Hocko wrote:
> > On Wed 17-02-21 14:53:37, David Hildenbrand wrote:
> > > On 17.02.21 14:50, Michal Hocko wrote:
> > [...]
> > > > Do we have any real life examples? Or does this fall more into, let's
> > > > optimize an existing implementation category.
> > > > 
> > > 
> > > It's a big TODO item I have on my list and I am happy that Oscar is looking
> > > into it. So yes, I noticed it while working on virtio-mem. It's real.
> > 
> > Do not take me wrong, I am not opposing to the functionality. I am
> > asking for the specific usecase.
> 
> Makes sense, and a proper motivation should be included in the patches/cover
> letter. So here comes a quick-n-dirty example:

Definitely. I took it for granted that the problem was obvious.

> Start a VM with 4G. Hotplug 1G via virtio-mem and online it to ZONE_MOVABLE.
> Allocate 512 huge pages.
> 
> [root@localhost ~]# cat /proc/meminfo
> MemTotal:        5061512 kB
> MemFree:         3319396 kB
> MemAvailable:    3457144 kB
> ...
> HugePages_Total:     512
> HugePages_Free:      512
> HugePages_Rsvd:        0
> HugePages_Surp:        0
> Hugepagesize:       2048 kB
> 
> 
> The huge pages get partially allocate from ZONE_MOVABLE. Try unplugging 1G
> via virtio-mem (remember, all ZONE_MOVABLE). Inside the guest:
> 
> [  180.058992] alloc_contig_range: [1b8000, 1c0000) PFNs busy
> [  180.060531] alloc_contig_range: [1b8000, 1c0000) PFNs busy
> [  180.061972] alloc_contig_range: [1b8000, 1c0000) PFNs busy
> [  180.063413] alloc_contig_range: [1b8000, 1c0000) PFNs busy
> [  180.064838] alloc_contig_range: [1b8000, 1c0000) PFNs busy
> [  180.065848] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
> [  180.066794] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
> [  180.067738] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
> [  180.068669] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
> [  180.069598] alloc_contig_range: [1bfc00, 1c0000) PFNs busy
> 
> 
> I succeed in unplugging 540MB - 484 MB remain blocked by huge pages ("which
> did not end up there by pure luck"). These pages are movable (and even
> free!) and can easily be reallocated.

Thanks for providing such a detailed case David.
I will gather the bits and prepare a v2 once I gather more feedback.

-- 
Oscar Salvador
SUSE L3

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Michal Hocko]

On Wed 17-02-21 11:08:15, Oscar Salvador wrote:
[...]
> +static bool alloc_and_dissolve_huge_page(struct hstate *h, struct page *page)
> +{
> +	gfp_t gfp_mask = htlb_alloc_mask(h);
> +	nodemask_t *nmask = &node_states[N_MEMORY];
> +	struct page *new_page;
> +	bool ret = false;
> +	int nid;
> +
> +	spin_lock(&hugetlb_lock);
> +	/*
> +	 * Check one more time to make race-window smaller.
> +	 */
> +	if (!PageHuge(page)) {
> +		/*
> +		 * Dissolved from under our feet.
> +		 */
> +		spin_unlock(&hugetlb_lock);
> +		return true;
> +	}

Is this really necessary? dissolve_free_huge_page will take care of this
and the race windown you are covering is really tiny.

> +
> +	nid = page_to_nid(page);
> +	spin_unlock(&hugetlb_lock);
> +
> +	/*
> +	 * Before dissolving the page, we need to allocate a new one,
> +	 * so the pool remains stable.
> +	 */
> +	new_page = alloc_fresh_huge_page(h, gfp_mask, nid, nmask, NULL);

wrt. fallback to other zones, I haven't realized that the primary
usecase is a form of memory offlining (from virt-mem). I am not yet sure
what the proper behavior is in that case but if breaking hugetlb pools,
similar to the normal hotplug operation, is viable then this needs a
special mode. We do not want a random alloc_contig_range user to do the
same. So for starter I would go with __GFP_THISNODE here.

> +	if (new_page) {
> +		/*
> +		 * Ok, we got a new free hugepage to replace this one. Try to
> +		 * dissolve the old page.
> +		 */
> +		if (!dissolve_free_huge_page(page)) {
> +			ret = true;
> +		} else if (dissolve_free_huge_page(new_page)) {
> +			/*
> +			 * Seems the old page could not be dissolved, so try to
> +			 * dissolve the freshly allocated page. If that fails
> +			 * too, let us count the new page as a surplus. Doing so
> +			 * allows the pool to be re-balanced when pages are freed
> +			 * instead of enqueued again.
> +			 */
> +			spin_lock(&hugetlb_lock);
> +			h->surplus_huge_pages++;
> +			h->surplus_huge_pages_node[nid]++;
> +			spin_unlock(&hugetlb_lock);
> +		}
> +		/*
> +		 * Free it into the hugepage allocator
> +		 */
> +		put_page(new_page);
> +	}
> +
> +	return ret;
> +}
> +
> +bool isolate_or_dissolve_huge_page(struct page *page)
> +{
> +	struct hstate *h = NULL;
> +	struct page *head;
> +	bool ret = false;
> +
> +	spin_lock(&hugetlb_lock);
> +	if (PageHuge(page)) {
> +		head = compound_head(page);
> +		h = page_hstate(head);
> +	}
> +	spin_unlock(&hugetlb_lock);
> +
> +	if (!h)
> +		/*
> +		 * The page might have been dissolved from under our feet.
> +		 * If that is the case, return success as if we dissolved it
> +		 * ourselves.
> +		 */
> +		return true;

nit I would put the comment above the conditin for both cases. It reads
more easily that way. At least without { }.

> +
> +	if (hstate_is_gigantic(h))
> +		/*
> +		 * Fence off gigantic pages as there is a cyclic dependency
> +		 * between alloc_contig_range and them.
> +		 */
> +		return ret;
> +
> +	if(!page_count(head) && alloc_and_dissolve_huge_page(h, head))
> +		ret = true;
> +
> +	return ret;
> +}
> +
>  struct page *alloc_huge_page(struct vm_area_struct *vma,
>  				    unsigned long addr, int avoid_reserve)
>  {

Other than that I haven't noticed any surprises.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] mm: Make alloc_contig_range handle in-use hugetlb pages

[Michal Hocko]

On Wed 17-02-21 11:08:16, Oscar Salvador wrote:
> In-use hugetlb pages can be migrated as any other page (LRU
> and Movable), so let alloc_contig_range handle them.
> 
> All we need is to succesfully isolate such page.
> 
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
>  include/linux/hugetlb.h |  5 +++--
>  mm/compaction.c         | 11 ++++++++++-
>  mm/hugetlb.c            |  6 ++++--
>  mm/vmscan.c             |  5 +++--
>  4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 72352d718829..8c17d0dbc87c 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -505,7 +505,7 @@ struct huge_bootmem_page {
>  	struct hstate *hstate;
>  };
>  
> -bool isolate_or_dissolve_huge_page(struct page *page);
> +bool isolate_or_dissolve_huge_page(struct page *page, struct list_head *list);
>  struct page *alloc_huge_page(struct vm_area_struct *vma,
>  				unsigned long addr, int avoid_reserve);
>  struct page *alloc_huge_page_nodemask(struct hstate *h, int preferred_nid,
> @@ -776,7 +776,8 @@ void set_page_huge_active(struct page *page);
>  #else	/* CONFIG_HUGETLB_PAGE */
>  struct hstate {};
>  
> -static inline bool isolate_or_dissolve_huge_page(struct page *page)
> +static inline bool isolate_or_dissolve_huge_page(struct page *page,
> +						 struct list_head *list)
>  {
>  	return false;
>  }
> diff --git a/mm/compaction.c b/mm/compaction.c
> index d52506ed9db7..55a41a9228a9 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -906,9 +906,17 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  		}
>  
>  		if (PageHuge(page) && cc->alloc_contig) {
> -			if (!isolate_or_dissolve_huge_page(page))
> +			if (!isolate_or_dissolve_huge_page(page, &cc->migratepages))
>  				goto isolate_fail;
>  
> +			if (PageHuge(page)) {
> +				/*
> +				 * Hugepage was succesfully isolated.
> +				 */
> +				low_pfn += compound_nr(page) - 1;
> +				goto isolate_success_no_list;
> +			}
> +
>  			/*
>  			 * Ok, the hugepage was dissolved. Now these pages are
>  			 * Buddy and cannot be re-allocated because they are
> @@ -1053,6 +1061,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>  
>  isolate_success:
>  		list_add(&page->lru, &cc->migratepages);
> +isolate_success_no_list:
>  		cc->nr_migratepages += compound_nr(page);

I do not follow. You have successfully isolated huge pages so why don't
you add that page to the migratepages list to be migrated?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] mm: Make alloc_contig_range handle in-use hugetlb pages

[Oscar Salvador]

On 2021-02-17 16:06, Michal Hocko wrote:
> I do not follow. You have successfully isolated huge pages so why don't
> you add that page to the migratepages list to be migrated?

It is added. Note that I pass de list (cc->migratepages) to 
isolate_or_dissolve_huge_page().
It is done this way because we have to pass the list to 
isolate_huge_page() directly.
That is why I jump to isolate_success_no_list instead of isolate_sucess.

I know it is subtle, but I could not come up with a better name at that 
time.

-- 
Oscar Salvador
SUSE L3

Re: [PATCH 2/2] mm: Make alloc_contig_range handle in-use hugetlb pages

[Michal Hocko]

On Wed 17-02-21 16:27:27, Oscar Salvador wrote:
> On 2021-02-17 16:06, Michal Hocko wrote:
> > I do not follow. You have successfully isolated huge pages so why don't
> > you add that page to the migratepages list to be migrated?
> 
> It is added. Note that I pass de list (cc->migratepages) to
> isolate_or_dissolve_huge_page().
> It is done this way because we have to pass the list to isolate_huge_page()
> directly.
> That is why I jump to isolate_success_no_list instead of isolate_sucess.
> 
> I know it is subtle, but I could not come up with a better name at that
> time.

OK, I have missed that. Maybe just extend the comment. 

	/*
	 * Hugepage was succesfully isolated and on the tmigratepages
	 * list 
	 */
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] mm: Make alloc_contig_range handle in-use hugetlb pages

[Oscar Salvador]

On Wed, Feb 17, 2021 at 04:33:18PM +0100, Michal Hocko wrote:
> OK, I have missed that. Maybe just extend the comment. 
> 
> 	/*
> 	 * Hugepage was succesfully isolated and on the tmigratepages
> 	 * list 
> 	 */

Sure, I will improve it.

Thanks 

-- 
Oscar Salvador
SUSE L3

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Oscar Salvador]

On Wed, Feb 17, 2021 at 04:00:11PM +0100, Michal Hocko wrote:
> Is this really necessary? dissolve_free_huge_page will take care of this
> and the race windown you are covering is really tiny.

Probably not, I was trying to shrink to race window as much as possible
but the call to dissolve_free_huge_page might be enough.

> > +	nid = page_to_nid(page);
> > +	spin_unlock(&hugetlb_lock);
> > +
> > +	/*
> > +	 * Before dissolving the page, we need to allocate a new one,
> > +	 * so the pool remains stable.
> > +	 */
> > +	new_page = alloc_fresh_huge_page(h, gfp_mask, nid, nmask, NULL);
> 
> wrt. fallback to other zones, I haven't realized that the primary
> usecase is a form of memory offlining (from virt-mem). I am not yet sure
> what the proper behavior is in that case but if breaking hugetlb pools,
> similar to the normal hotplug operation, is viable then this needs a
> special mode. We do not want a random alloc_contig_range user to do the
> same. So for starter I would go with __GFP_THISNODE here.

Ok, makes sense.
__GFP_THISNODE will not allow fallback to other node's zones.
Since we only allow the nid the page belongs to, nodemask should be
NULL, right?

> > +	if (!h)
> > +		/*
> > +		 * The page might have been dissolved from under our feet.
> > +		 * If that is the case, return success as if we dissolved it
> > +		 * ourselves.
> > +		 */
> > +		return true;
> 
> nit I would put the comment above the conditin for both cases. It reads
> more easily that way. At least without { }.

Yes, makes sense.

> Other than that I haven't noticed any surprises.

I did. The 'put_page' call should be placed above, right after getting
the page. Otherwise, refcount == 1 and we will fail to dissolve the
new page if we need to (in case old page fails to be dissolved).
I already fixed that locally.

-- 
Oscar Salvador
SUSE L3

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Michal Hocko]

On Thu 18-02-21 11:09:17, Oscar Salvador wrote:
> On Wed, Feb 17, 2021 at 04:00:11PM +0100, Michal Hocko wrote:
> > Is this really necessary? dissolve_free_huge_page will take care of this
> > and the race windown you are covering is really tiny.
> 
> Probably not, I was trying to shrink to race window as much as possible
> but the call to dissolve_free_huge_page might be enough.
> 
> > > +	nid = page_to_nid(page);
> > > +	spin_unlock(&hugetlb_lock);
> > > +
> > > +	/*
> > > +	 * Before dissolving the page, we need to allocate a new one,
> > > +	 * so the pool remains stable.
> > > +	 */
> > > +	new_page = alloc_fresh_huge_page(h, gfp_mask, nid, nmask, NULL);
> > 
> > wrt. fallback to other zones, I haven't realized that the primary
> > usecase is a form of memory offlining (from virt-mem). I am not yet sure
> > what the proper behavior is in that case but if breaking hugetlb pools,
> > similar to the normal hotplug operation, is viable then this needs a
> > special mode. We do not want a random alloc_contig_range user to do the
> > same. So for starter I would go with __GFP_THISNODE here.
> 
> Ok, makes sense.
> __GFP_THISNODE will not allow fallback to other node's zones.
> Since we only allow the nid the page belongs to, nodemask should be
> NULL, right?

I would have to double check because hugetlb has a slightly different
expectations from nodemask than the page allocator. The later translates
that to all possible nodes but hugetlb API tries to dereference nodes.
Maybe THIS node special cases it somewhere.

> > > +	if (!h)
> > > +		/*
> > > +		 * The page might have been dissolved from under our feet.
> > > +		 * If that is the case, return success as if we dissolved it
> > > +		 * ourselves.
> > > +		 */
> > > +		return true;
> > 
> > nit I would put the comment above the conditin for both cases. It reads
> > more easily that way. At least without { }.
> 
> Yes, makes sense.
> 
> > Other than that I haven't noticed any surprises.
> 
> I did. The 'put_page' call should be placed above, right after getting
> the page. Otherwise, refcount == 1 and we will fail to dissolve the
> new page if we need to (in case old page fails to be dissolved).
> I already fixed that locally.

I am not sure I follow. newly allocated pages is unreferenced
unconditionally and the old page is not referenced by this path.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Oscar Salvador]

On Thu, Feb 18, 2021 at 01:52:38PM +0100, Michal Hocko wrote:
> > Ok, makes sense.
> > __GFP_THISNODE will not allow fallback to other node's zones.
> > Since we only allow the nid the page belongs to, nodemask should be
> > NULL, right?
> 
> I would have to double check because hugetlb has a slightly different
> expectations from nodemask than the page allocator. The later translates
> that to all possible nodes but hugetlb API tries to dereference nodes.
> Maybe THIS node special cases it somewhere.

Uhm, I do not quite follow here.
AFAICS, alloc_fresh_huge_page->alloc_buddy_huge_page does nothing
with the nodemask, bur rather with nodes_retry mask. That is done
to not retry on a node we failed to allocate a page.

Now, alloc_buddy_huge_page calls __alloc_pages_nodemask directly.
If my understanding is correct, it is ok to have a null nodemask
as __next_zones_zonelist() will go through our own zonelist,
since __GFP_THISNODE made us take ZONELIST_NOFALLBACK.

Actually, I do not see how passing a non-null nodemask migth have
helped there, unless we allow to specify more nodes.

> > I did. The 'put_page' call should be placed above, right after getting
> > the page. Otherwise, refcount == 1 and we will fail to dissolve the
> > new page if we need to (in case old page fails to be dissolved).
> > I already fixed that locally.
> 
> I am not sure I follow. newly allocated pages is unreferenced
> unconditionally and the old page is not referenced by this path.

Current code is:

 allocate_a_new_page (new_page's refcount = 1)
 dissolve_old_page
  : if fail
     dissolve_new_page (we cannot dissolve it refcount != 0)
 put_page(new_page);

It should be:

 allocate_a_new_page (new_page's refcount = 1)
 put_page(new_page); (new_page's refcount = 0)
 dissolve_old_page
  : if fail
     dissolve_new_page (we can dissolve it as refcount == 0)

I hope this clarifies it .

-- 
Oscar Salvador
SUSE L3

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Michal Hocko]

On Thu 18-02-21 14:32:50, Oscar Salvador wrote:
> On Thu, Feb 18, 2021 at 01:52:38PM +0100, Michal Hocko wrote:
> > > Ok, makes sense.
> > > __GFP_THISNODE will not allow fallback to other node's zones.
> > > Since we only allow the nid the page belongs to, nodemask should be
> > > NULL, right?
> > 
> > I would have to double check because hugetlb has a slightly different
> > expectations from nodemask than the page allocator. The later translates
> > that to all possible nodes but hugetlb API tries to dereference nodes.
> > Maybe THIS node special cases it somewhere.
> 
> Uhm, I do not quite follow here.
> AFAICS, alloc_fresh_huge_page->alloc_buddy_huge_page does nothing
> with the nodemask, bur rather with nodes_retry mask. That is done
> to not retry on a node we failed to allocate a page.
> 
> Now, alloc_buddy_huge_page calls __alloc_pages_nodemask directly.
> If my understanding is correct, it is ok to have a null nodemask
> as __next_zones_zonelist() will go through our own zonelist,
> since __GFP_THISNODE made us take ZONELIST_NOFALLBACK.

As I've said. Page allocator can cope with NULL nodemask just fine.
I have checked the code and now remember the tricky part. It is
alloc_gigantic_page which cannot work with NULL nodemask because it
relies on for_each_node_mask and that, unlike zonelist iterator, cannot
cope with NULL node mask. This is the case only for !GFP_THISNODE.

> Actually, I do not see how passing a non-null nodemask migth have
> helped there, unless we allow to specify more nodes.

No, nodemask is doesn't make any difference.
 
> > > I did. The 'put_page' call should be placed above, right after getting
> > > the page. Otherwise, refcount == 1 and we will fail to dissolve the
> > > new page if we need to (in case old page fails to be dissolved).
> > > I already fixed that locally.
> > 
> > I am not sure I follow. newly allocated pages is unreferenced
> > unconditionally and the old page is not referenced by this path.
> 
> Current code is:
> 
>  allocate_a_new_page (new_page's refcount = 1)
>  dissolve_old_page
>   : if fail
>      dissolve_new_page (we cannot dissolve it refcount != 0)
>  put_page(new_page);

OK, new_page would go to the pool rather than get freed as this is
neither a surplus nnor a temporary page.

> It should be:
> 
>  allocate_a_new_page (new_page's refcount = 1)
>  put_page(new_page); (new_page's refcount = 0)
>  dissolve_old_page
>   : if fail
>      dissolve_new_page (we can dissolve it as refcount == 0)
> 
> I hope this clarifies it .

OK, I see the problem now. And your above solution is not really
optimal either. Your put_page would add the page to the pool and so it
could be used by somebody. One way around it would be either directly
manipulating reference count which is fugly or you can make it a
temporal page (alloc_migrate_huge_page) or maybe even better not special
case this here but rather allow migrating free hugetlb pages in the
migrate_page path.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Oscar Salvador]

On 2021-02-18 14:59, Michal Hocko wrote:
> As I've said. Page allocator can cope with NULL nodemask just fine.
> I have checked the code and now remember the tricky part. It is
> alloc_gigantic_page which cannot work with NULL nodemask because it
> relies on for_each_node_mask and that, unlike zonelist iterator, cannot
> cope with NULL node mask. This is the case only for !GFP_THISNODE.

Ok, thanks for the clarification.

> OK, I see the problem now. And your above solution is not really
> optimal either. Your put_page would add the page to the pool and so it
> could be used by somebody. One

Yeah, that is right.

> way around it would be either directly
> manipulating reference count which is fugly or you can make it a
> temporal page (alloc_migrate_huge_page) or maybe even better not 
> special
> case this here but rather allow migrating free hugetlb pages in the
> migrate_page path.

I will have a look into it to see how what would look.

Thanks for the feedback

-- 
Oscar Salvador
SUSE L3

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Oscar Salvador]

On Thu, Feb 18, 2021 at 02:59:40PM +0100, Michal Hocko wrote:
> > It should be:
> > 
> >  allocate_a_new_page (new_page's refcount = 1)
> >  put_page(new_page); (new_page's refcount = 0)
> >  dissolve_old_page
> >   : if fail
> >      dissolve_new_page (we can dissolve it as refcount == 0)
> > 
> > I hope this clarifies it .
> 
> OK, I see the problem now. And your above solution is not really
> optimal either. Your put_page would add the page to the pool and so it
> could be used by somebody. One way around it would be either directly
> manipulating reference count which is fugly or you can make it a
> temporal page (alloc_migrate_huge_page) or maybe even better not special
> case this here but rather allow migrating free hugetlb pages in the
> migrate_page path.

I have been weighting up this option because it seemed the most clean way to
proceed. Having the hability to migrate free hugepages directly from migrate_page
would spare us this function.
But there is a problem. migrate_pages needs the pages to be on a list (by
page->lru). That is no problem for used pages, but for freehugepages we would
have to remove the page from hstate->hugepage_freelists, meaning that if userspace
comes along and tries to get a hugepage (a hugepage he previously allocated by
e.g: /proc/sys/.../nr_hugepages), it will fail.

So I am not really sure we can go this way. Unless we are willing to accept
that temporary userspace can get ENOMEM if it tries to use a hugepage, which
I do not think it is a good idea.
Another way to go would be to make up for the free hugepages to be migrated and
allocate the same amount, but that starts to go down a rabbit hole.

I yet need to give it some more spins, but all in all, I think the easiest way
forward way might be to do something like:

alloc_and_dissolve_huge_page {

   new_page = alloc_fresh_huge_page(h, gfp_mask, nid, NULL, NULL);
   if (new_page) {
           /*
            * Put the page in the freelist hugepage pool.
            * We might race with someone coming by and grabbing the page,
            * but that is fine since we mark the page as Temporary in case
            * both old and new_page fail to be dissolved, so new_page
            * will be freed when its last reference is gone.
            */
           put_page(new_page);
      
           if (!dissolve_free_huge_page(page)) {
                   /*
                    * Old page could be dissolved.
                    */
                   ret = true;
           } else if (dissolve_free_huge_page(new_page)) {
                  /*
                   * Page might have been dissolved by admin by doing
                   * "echo 0 > /proc/../nr_hugepages". Check it before marking
                   * the page.
                   */
                  spin_lock(&hugetlb_lock);
                  /* Mark the page Temporary in case we fail to dissolve both
                   * the old page and new_page. It will be freed when the last
                   * user drops it.
                   */
                  if (PageHuge(new_page))
                          SetPageHugeTemporary(new_page);
                  spin_unlock(&hugetlb_lock);
           }
   }

There is one more thing to cover though.
Between the dissolve_free_huge_page(new_page) and the PageHuge check, the
page might have been freed, and so enqueued (because we did not mark it as 
Temporary yet). If that is the case, we would later mark the page as Temporary
being in the freepool, not nice.

One way out would be to pass a boolean parameter to dissolve_free_huge_page(),
that tells whether the page should be marked Temporary in case the operation fails.
That would ease things a lot because __everything__ is being done under the lock,
which means the page cannot go away in the meantime.

Just my thoughts.

What do you think?


-- 
Oscar Salvador
SUSE L3

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Michal Hocko]

On Fri 19-02-21 10:05:53, Oscar Salvador wrote:
> On Thu, Feb 18, 2021 at 02:59:40PM +0100, Michal Hocko wrote:
> > > It should be:
> > > 
> > >  allocate_a_new_page (new_page's refcount = 1)
> > >  put_page(new_page); (new_page's refcount = 0)
> > >  dissolve_old_page
> > >   : if fail
> > >      dissolve_new_page (we can dissolve it as refcount == 0)
> > > 
> > > I hope this clarifies it .
> > 
> > OK, I see the problem now. And your above solution is not really
> > optimal either. Your put_page would add the page to the pool and so it
> > could be used by somebody. One way around it would be either directly
> > manipulating reference count which is fugly or you can make it a
> > temporal page (alloc_migrate_huge_page) or maybe even better not special
> > case this here but rather allow migrating free hugetlb pages in the
> > migrate_page path.
> 
> I have been weighting up this option because it seemed the most clean way to
> proceed. Having the hability to migrate free hugepages directly from migrate_page
> would spare us this function.
> But there is a problem. migrate_pages needs the pages to be on a list (by
> page->lru). That is no problem for used pages, but for freehugepages we would
> have to remove the page from hstate->hugepage_freelists, meaning that if userspace
> comes along and tries to get a hugepage (a hugepage he previously allocated by
> e.g: /proc/sys/.../nr_hugepages), it will fail.

Good point. I should have realized that.
 
> So I am not really sure we can go this way. Unless we are willing to accept
> that temporary userspace can get ENOMEM if it tries to use a hugepage, which
> I do not think it is a good idea.

No, this is not acceptable.

> Another way to go would be to make up for the free hugepages to be migrated and
> allocate the same amount, but that starts to go down a rabbit hole.
> 
> I yet need to give it some more spins, but all in all, I think the easiest way
> forward way might be to do something like:
> 
> alloc_and_dissolve_huge_page {
> 
>    new_page = alloc_fresh_huge_page(h, gfp_mask, nid, NULL, NULL);
>    if (new_page) {
>            /*
>             * Put the page in the freelist hugepage pool.
>             * We might race with someone coming by and grabbing the page,
>             * but that is fine since we mark the page as Temporary in case
>             * both old and new_page fail to be dissolved, so new_page
>             * will be freed when its last reference is gone.
>             */
>            put_page(new_page);
>       
>            if (!dissolve_free_huge_page(page)) {
>                    /*
>                     * Old page could be dissolved.
>                     */
>                    ret = true;
>            } else if (dissolve_free_huge_page(new_page)) {
>                   /*
>                    * Page might have been dissolved by admin by doing
>                    * "echo 0 > /proc/../nr_hugepages". Check it before marking
>                    * the page.
>                    */
>                   spin_lock(&hugetlb_lock);
>                   /* Mark the page Temporary in case we fail to dissolve both
>                    * the old page and new_page. It will be freed when the last
>                    * user drops it.
>                    */
>                   if (PageHuge(new_page))
>                           SetPageHugeTemporary(new_page);
>                   spin_unlock(&hugetlb_lock);
>            }
>    }

OK, this should work but I am really wondering whether it wouldn't be
just simpler to replace the old page by a new one in the free list
directly. Or is there any reason we have to go through the generic
helpers path? I mean something like this

	new_page = alloc_fresh_huge_page();
	if (!new_page)
		goto fail;
	spin_lock(hugetlb_lock);
	if (!PageHuge(old_page)) {
		/* freed from under us, nothing to do */ 
		__update_and_free_page(new_page);
		goto unlock;
	}
	list_del(&old_page->lru);
	__update_and_free_page(old_page);
	__enqueue_huge_page(new_page);
unlock:
	spin_unlock(hugetlb_lock);

This will require to split update_and_free_page and enqueue_huge_page to
counters independent parts but that shouldn't be a big deal. But it will
also protect from any races. Not an act of beauty but seems less hackish
to me.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Oscar Salvador]

On Fri, Feb 19, 2021 at 10:56:42AM +0100, Michal Hocko wrote:
> OK, this should work but I am really wondering whether it wouldn't be
> just simpler to replace the old page by a new one in the free list
> directly. Or is there any reason we have to go through the generic
> helpers path? I mean something like this
> 
> 	new_page = alloc_fresh_huge_page();
> 	if (!new_page)
> 		goto fail;
> 	spin_lock(hugetlb_lock);
> 	if (!PageHuge(old_page)) {
> 		/* freed from under us, nothing to do */ 
> 		__update_and_free_page(new_page);
> 		goto unlock;
> 	}
> 	list_del(&old_page->lru);
> 	__update_and_free_page(old_page);
> 	__enqueue_huge_page(new_page);
> unlock:
> 	spin_unlock(hugetlb_lock);
> 
> This will require to split update_and_free_page and enqueue_huge_page to
> counters independent parts but that shouldn't be a big deal. But it will
> also protect from any races. Not an act of beauty but seems less hackish
> to me.

Yes, I think this would to the trick, and it is race-free.
Let me play with it a bit and see what I can come up with.

Thanks for the valuable insight.

-- 
Oscar Salvador
SUSE L3

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Oscar Salvador]

On Fri, Feb 19, 2021 at 10:56:42AM +0100, Michal Hocko wrote:
> OK, this should work but I am really wondering whether it wouldn't be
> just simpler to replace the old page by a new one in the free list
> directly. Or is there any reason we have to go through the generic
> helpers path? I mean something like this
> 
> 	new_page = alloc_fresh_huge_page();
> 	if (!new_page)
> 		goto fail;
> 	spin_lock(hugetlb_lock);
> 	if (!PageHuge(old_page)) {
> 		/* freed from under us, nothing to do */ 
> 		__update_and_free_page(new_page);
> 		goto unlock;
> 	}
> 	list_del(&old_page->lru);
> 	__update_and_free_page(old_page);
> 	__enqueue_huge_page(new_page);
> unlock:
> 	spin_unlock(hugetlb_lock);
> 
> This will require to split update_and_free_page and enqueue_huge_page to
> counters independent parts but that shouldn't be a big deal. But it will
> also protect from any races. Not an act of beauty but seems less hackish
> to me.

On a closer look, do we really need to decouple update_and_free_page and
enqueue_huge_page? These two functions do not handle the lock, but rather
the functions that call them (as would be in our case).
Only update_and_free_page drops the lock during the freeing of a gigantic page
and then it takes it again, as the caller is who took the lock.

am I missing anything obvious here?

-- 
Oscar Salvador
SUSE L3

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Michal Hocko]

On Fri 19-02-21 11:40:30, Oscar Salvador wrote:
> On Fri, Feb 19, 2021 at 10:56:42AM +0100, Michal Hocko wrote:
> > OK, this should work but I am really wondering whether it wouldn't be
> > just simpler to replace the old page by a new one in the free list
> > directly. Or is there any reason we have to go through the generic
> > helpers path? I mean something like this
> > 
> > 	new_page = alloc_fresh_huge_page();
> > 	if (!new_page)
> > 		goto fail;
> > 	spin_lock(hugetlb_lock);
> > 	if (!PageHuge(old_page)) {
> > 		/* freed from under us, nothing to do */ 
> > 		__update_and_free_page(new_page);
> > 		goto unlock;
> > 	}
> > 	list_del(&old_page->lru);
> > 	__update_and_free_page(old_page);
> > 	__enqueue_huge_page(new_page);
> > unlock:
> > 	spin_unlock(hugetlb_lock);
> > 
> > This will require to split update_and_free_page and enqueue_huge_page to
> > counters independent parts but that shouldn't be a big deal. But it will
> > also protect from any races. Not an act of beauty but seems less hackish
> > to me.
> 
> On a closer look, do we really need to decouple update_and_free_page and
> enqueue_huge_page? These two functions do not handle the lock, but rather
> the functions that call them (as would be in our case).
> Only update_and_free_page drops the lock during the freeing of a gigantic page
> and then it takes it again, as the caller is who took the lock.
> 
> am I missing anything obvious here?

It is not the lock that I care about but more about counters. The
intention was that there is a single place to handle both enqueing and
dequeing. As not all places require counters to be updated. E.g. the
migration which just replaces one page by another.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Oscar Salvador]

On Fri, Feb 19, 2021 at 11:55:00AM +0100, Michal Hocko wrote:
> It is not the lock that I care about but more about counters. The
> intention was that there is a single place to handle both enqueing and
> dequeing. As not all places require counters to be updated. E.g. the
> migration which just replaces one page by another.

I see.
alloc_fresh_huge_page->prep_new_huge_page increments h->nr_huge_pages{_node}
counters.
Which means:

>       new_page = alloc_fresh_huge_page();
>       if (!new_page)
>               goto fail;
>       spin_lock(hugetlb_lock);
>       if (!PageHuge(old_page)) {
>               /* freed from under us, nothing to do */ 
>               __update_and_free_page(new_page);

Here we need update_and_free_page, otherwise we would be leaving a stale value
in h->nr_huge_pages{_node}. 

>               goto unlock;
>       }
>       list_del(&old_page->lru);
>       __update_and_free_page(old_page);

Same here.

>       __enqueue_huge_page(new_page);

This is ok since h->free_huge_pages{_node} do not need to be updated.

-- 
Oscar Salvador
SUSE L3

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Michal Hocko]

On Fri 19-02-21 12:17:11, Oscar Salvador wrote:
> On Fri, Feb 19, 2021 at 11:55:00AM +0100, Michal Hocko wrote:
> > It is not the lock that I care about but more about counters. The
> > intention was that there is a single place to handle both enqueing and
> > dequeing. As not all places require counters to be updated. E.g. the
> > migration which just replaces one page by another.
> 
> I see.
> alloc_fresh_huge_page->prep_new_huge_page increments h->nr_huge_pages{_node}
> counters.
> Which means:
> 
> >       new_page = alloc_fresh_huge_page();
> >       if (!new_page)
> >               goto fail;
> >       spin_lock(hugetlb_lock);
> >       if (!PageHuge(old_page)) {
> >               /* freed from under us, nothing to do */ 
> >               __update_and_free_page(new_page);
> 
> Here we need update_and_free_page, otherwise we would be leaving a stale value
> in h->nr_huge_pages{_node}. 
> 
> >               goto unlock;
> >       }
> >       list_del(&old_page->lru);
> >       __update_and_free_page(old_page);
> 
> Same here.
> 
> >       __enqueue_huge_page(new_page);
> 
> This is ok since h->free_huge_pages{_node} do not need to be updated.

Fair enough. I didn't get to think this through obviously, but you
should get the idea ;)
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: Make alloc_contig_range handle free hugetlb pages

[Mike Kravetz]

On 2/19/21 2:14 AM, Oscar Salvador wrote:
> On Fri, Feb 19, 2021 at 10:56:42AM +0100, Michal Hocko wrote:
>> OK, this should work but I am really wondering whether it wouldn't be
>> just simpler to replace the old page by a new one in the free list
>> directly. Or is there any reason we have to go through the generic
>> helpers path? I mean something like this
>>
>> 	new_page = alloc_fresh_huge_page();
>> 	if (!new_page)
>> 		goto fail;
>> 	spin_lock(hugetlb_lock);
>> 	if (!PageHuge(old_page)) {

Yes, something like this should work.  I'll let Oscar work out the details.
One thing to note is that you also need to check for old_page not on the
free list here.  It could have been allocated and in use.  In addition,
make sure to check the new flag HPageFreed to ensure page is on free list
before doing any type of update_and_free_page call.
-- 
Mike Kravetz

>> 		/* freed from under us, nothing to do */ 
>> 		__update_and_free_page(new_page);
>> 		goto unlock;
>> 	}
>> 	list_del(&old_page->lru);
>> 	__update_and_free_page(old_page);
>> 	__enqueue_huge_page(new_page);
>> unlock:
>> 	spin_unlock(hugetlb_lock);
>>
>> This will require to split update_and_free_page and enqueue_huge_page to
>> counters independent parts but that shouldn't be a big deal. But it will
>> also protect from any races. Not an act of beauty but seems less hackish
>> to me.
> 
> Yes, I think this would to the trick, and it is race-free.
> Let me play with it a bit and see what I can come up with.
> 
> Thanks for the valuable insight.
>