diff mbox series

[v2] mm: hugetlb_vmemmap: provide stronger vmemmap allocation guarantees

Message ID 20230412195939.1242462-1-pasha.tatashin@soleen.com
State New
Headers
Series [v2] mm: hugetlb_vmemmap: provide stronger vmemmap allocation guarantees |

Commit Message

Pasha Tatashin April 12, 2023, 7:59 p.m. UTC 
  HugeTLB pages have a struct page optimizations where struct pages for tail
pages are freed. However, when HugeTLB pages are destroyed, the memory for
struct pages (vmemmap) need to be allocated again.

Currently, __GFP_NORETRY flag is used to allocate the memory for vmemmap,
but given that this flag makes very little effort to actually reclaim
memory the returning of huge pages back to the system can be problem. Lets
use __GFP_RETRY_MAYFAIL instead. This flag is also performs graceful
reclaim without causing ooms, but at least it may perform a few retries,
and will fail only when there is genuinely little amount of unused memory
in the system.

Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
Suggested-by: David Rientjes <rientjes@google.com>
---
 mm/hugetlb_vmemmap.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Changelog:
v2
  - removed gfp_mask argument from alloc_vmemmap_page_list as suggested by
    David Rientjes.
  - Fixed spelling in the patch title.

Comments

Pasha Tatashin April 13, 2023, 2:59 p.m. UTC | #1 
On Wed, Apr 12, 2023 at 4:13 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> Lots of questions (ie, missing information!)
>
> On Wed, 12 Apr 2023 19:59:39 +0000 Pasha Tatashin <pasha.tatashin@soleen.com> wrote:
>
> > HugeTLB pages have a struct page optimizations where struct pages for tail
> > pages are freed. However, when HugeTLB pages are destroyed, the memory for
> > struct pages (vmemmap) need to be allocated again.
> >
> > Currently, __GFP_NORETRY flag is used to allocate the memory for vmemmap,
> > but given that this flag makes very little effort to actually reclaim
> > memory the returning of huge pages back to the system can be problem.
>
> Are there any reports of this happening in the real world?
>
> > Lets
> > use __GFP_RETRY_MAYFAIL instead. This flag is also performs graceful
> > reclaim without causing ooms, but at least it may perform a few retries,
> > and will fail only when there is genuinely little amount of unused memory
> > in the system.
>
> If so, does this change help?

It helps to avoid transient allocation problems. In general it is not
a good idea to fail because we are trying to free gigantic pages back
to the system.

>
> If the allocation attempt fails, what are the consequences?

The gigantic page is not going to be returned to the system. The use
will have to free some memory before returning them back to the
system.

>
> What are the potential downsides to this change?  Why did we choose
> __GFP_NORETRY in the first place?
>
> What happens if we try harder (eg, GFP_KERNEL)?

MIchal answered this question, that it won't do much difference due to
__GFP_THISNODE
Pasha Tatashin April 13, 2023, 3:05 p.m. UTC | #2 
On Wed, Apr 12, 2023 at 4:18 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Wed 12-04-23 13:13:02, Andrew Morton wrote:
> > Lots of questions (ie, missing information!)
> >
> > On Wed, 12 Apr 2023 19:59:39 +0000 Pasha Tatashin <pasha.tatashin@soleen.com> wrote:
> >
> > > HugeTLB pages have a struct page optimizations where struct pages for tail
> > > pages are freed. However, when HugeTLB pages are destroyed, the memory for
> > > struct pages (vmemmap) need to be allocated again.
> > >
> > > Currently, __GFP_NORETRY flag is used to allocate the memory for vmemmap,
> > > but given that this flag makes very little effort to actually reclaim
> > > memory the returning of huge pages back to the system can be problem.
> >
> > Are there any reports of this happening in the real world?
> >
> > > Lets
> > > use __GFP_RETRY_MAYFAIL instead. This flag is also performs graceful
> > > reclaim without causing ooms, but at least it may perform a few retries,
> > > and will fail only when there is genuinely little amount of unused memory
> > > in the system.
> >
> > If so, does this change help?
> >
> > If the allocation attempt fails, what are the consequences?
> >
> > What are the potential downsides to this change?  Why did we choose
> > __GFP_NORETRY in the first place?
> >
> > What happens if we try harder (eg, GFP_KERNEL)?
>
> Mike was generous enough to make me remember
> https://lore.kernel.org/linux-mm/YCafit5ruRJ+SL8I@dhcp22.suse.cz/.
> GFP_KERNEL wouldn't make much difference becauset this is
> __GFP_THISNODE. But I do agree that the changelog should go into more
> details about why do we want to try harder now. I can imagine that
> shrinking hugetlb pool by a large amount of hugetlb pages might become a
> problem but is this really happening or is this a theoretical concern?

This is a theoretical concern. Freeing a 1G page requires 16M of free
memory. A machine might need to be reconfigured from one task to
another, and release a large number of 1G pages back to the system if
allocating 16M fails, the release won't work.

In an ideal scenario we should guarantee that this never fails: that
we always can free HugeTLB pages back to the system. At the very least
we could steal the memory for vmemmap from the page that is being
released.

Pasha
Michal Hocko April 13, 2023, 3:25 p.m. UTC | #3 
On Thu 13-04-23 11:05:20, Pavel Tatashin wrote:
> On Wed, Apr 12, 2023 at 4:18 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Wed 12-04-23 13:13:02, Andrew Morton wrote:
> > > Lots of questions (ie, missing information!)
> > >
> > > On Wed, 12 Apr 2023 19:59:39 +0000 Pasha Tatashin <pasha.tatashin@soleen.com> wrote:
> > >
> > > > HugeTLB pages have a struct page optimizations where struct pages for tail
> > > > pages are freed. However, when HugeTLB pages are destroyed, the memory for
> > > > struct pages (vmemmap) need to be allocated again.
> > > >
> > > > Currently, __GFP_NORETRY flag is used to allocate the memory for vmemmap,
> > > > but given that this flag makes very little effort to actually reclaim
> > > > memory the returning of huge pages back to the system can be problem.
> > >
> > > Are there any reports of this happening in the real world?
> > >
> > > > Lets
> > > > use __GFP_RETRY_MAYFAIL instead. This flag is also performs graceful
> > > > reclaim without causing ooms, but at least it may perform a few retries,
> > > > and will fail only when there is genuinely little amount of unused memory
> > > > in the system.
> > >
> > > If so, does this change help?
> > >
> > > If the allocation attempt fails, what are the consequences?
> > >
> > > What are the potential downsides to this change?  Why did we choose
> > > __GFP_NORETRY in the first place?
> > >
> > > What happens if we try harder (eg, GFP_KERNEL)?
> >
> > Mike was generous enough to make me remember
> > https://lore.kernel.org/linux-mm/YCafit5ruRJ+SL8I@dhcp22.suse.cz/.
> > GFP_KERNEL wouldn't make much difference becauset this is
> > __GFP_THISNODE. But I do agree that the changelog should go into more
> > details about why do we want to try harder now. I can imagine that
> > shrinking hugetlb pool by a large amount of hugetlb pages might become a
> > problem but is this really happening or is this a theoretical concern?
> 
> This is a theoretical concern. Freeing a 1G page requires 16M of free
> memory. A machine might need to be reconfigured from one task to
> another, and release a large number of 1G pages back to the system if
> allocating 16M fails, the release won't work.

This is really an important "detail" changelog should mention. While I
am not really against that change I would much rather see that as a
result of a real world fix rather than a theoretical concern. Mostly
because a real life scenario would allow us to test the
__GFP_RETRY_MAYFAIL effectivness. As that request might fail as well we
just end up with a theoretical fix for a theoretical problem. Something
that is easy to introduce but much harder to get rid of should we ever
need to change __GFP_RETRY_MAYFAIL implementation for example.

> In an ideal scenario we should guarantee that this never fails: that
> we always can free HugeTLB pages back to the system. At the very least
> we could steal the memory for vmemmap from the page that is being
> released.

Yes, this really bothered me when the concept was introduced initially.
I am always concerned when you need to allocate in order to free memory.
Practically speaking we haven't heard about bug reports so maybe this is
not such a big deal as I thought.
Pasha Tatashin April 13, 2023, 5:11 p.m. UTC | #4 
On Thu, Apr 13, 2023 at 11:25 AM Michal Hocko <mhocko@suse.com> wrote:
>
> On Thu 13-04-23 11:05:20, Pavel Tatashin wrote:
> > On Wed, Apr 12, 2023 at 4:18 PM Michal Hocko <mhocko@suse.com> wrote:
> > >
> > > On Wed 12-04-23 13:13:02, Andrew Morton wrote:
> > > > Lots of questions (ie, missing information!)
> > > >
> > > > On Wed, 12 Apr 2023 19:59:39 +0000 Pasha Tatashin <pasha.tatashin@soleen.com> wrote:
> > > >
> > > > > HugeTLB pages have a struct page optimizations where struct pages for tail
> > > > > pages are freed. However, when HugeTLB pages are destroyed, the memory for
> > > > > struct pages (vmemmap) need to be allocated again.
> > > > >
> > > > > Currently, __GFP_NORETRY flag is used to allocate the memory for vmemmap,
> > > > > but given that this flag makes very little effort to actually reclaim
> > > > > memory the returning of huge pages back to the system can be problem.
> > > >
> > > > Are there any reports of this happening in the real world?
> > > >
> > > > > Lets
> > > > > use __GFP_RETRY_MAYFAIL instead. This flag is also performs graceful
> > > > > reclaim without causing ooms, but at least it may perform a few retries,
> > > > > and will fail only when there is genuinely little amount of unused memory
> > > > > in the system.
> > > >
> > > > If so, does this change help?
> > > >
> > > > If the allocation attempt fails, what are the consequences?
> > > >
> > > > What are the potential downsides to this change?  Why did we choose
> > > > __GFP_NORETRY in the first place?
> > > >
> > > > What happens if we try harder (eg, GFP_KERNEL)?
> > >
> > > Mike was generous enough to make me remember
> > > https://lore.kernel.org/linux-mm/YCafit5ruRJ+SL8I@dhcp22.suse.cz/.
> > > GFP_KERNEL wouldn't make much difference becauset this is
> > > __GFP_THISNODE. But I do agree that the changelog should go into more
> > > details about why do we want to try harder now. I can imagine that
> > > shrinking hugetlb pool by a large amount of hugetlb pages might become a
> > > problem but is this really happening or is this a theoretical concern?
> >
> > This is a theoretical concern. Freeing a 1G page requires 16M of free
> > memory. A machine might need to be reconfigured from one task to
> > another, and release a large number of 1G pages back to the system if
> > allocating 16M fails, the release won't work.
>
> This is really an important "detail" changelog should mention. While I
> am not really against that change I would much rather see that as a
> result of a real world fix rather than a theoretical concern. Mostly
> because a real life scenario would allow us to test the
> __GFP_RETRY_MAYFAIL effectivness. As that request might fail as well we
> just end up with a theoretical fix for a theoretical problem. Something
> that is easy to introduce but much harder to get rid of should we ever
> need to change __GFP_RETRY_MAYFAIL implementation for example.

I will add this to changelog in v3. If  __GFP_RETRY_MAYFAIL is
ineffective we will receive feedback once someone hits this problem.
Otherwise, we will never hear about it. I think overall it is safer to
keep this code with __GFP_RETRY_MAYFAIL flag.

>
> > In an ideal scenario we should guarantee that this never fails: that
> > we always can free HugeTLB pages back to the system. At the very least
> > we could steal the memory for vmemmap from the page that is being
> > released.
>
> Yes, this really bothered me when the concept was introduced initially.
> I am always concerned when you need to allocate in order to free memory.
> Practically speaking we haven't heard about bug reports so maybe this is
> not such a big deal as I thought.

I suspect this is because at the moment it is not that frequent when a
machine is reconfigured from having a lot of HugeTLB based workload to
non-HugeTLB workload.

Pasha
Michal Hocko April 13, 2023, 6:12 p.m. UTC | #5 
On Thu 13-04-23 13:11:39, Pavel Tatashin wrote:
> On Thu, Apr 13, 2023 at 11:25 AM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Thu 13-04-23 11:05:20, Pavel Tatashin wrote:
[...]
> > > This is a theoretical concern. Freeing a 1G page requires 16M of free
> > > memory. A machine might need to be reconfigured from one task to
> > > another, and release a large number of 1G pages back to the system if
> > > allocating 16M fails, the release won't work.
> >
> > This is really an important "detail" changelog should mention. While I
> > am not really against that change I would much rather see that as a
> > result of a real world fix rather than a theoretical concern. Mostly
> > because a real life scenario would allow us to test the
> > __GFP_RETRY_MAYFAIL effectivness. As that request might fail as well we
> > just end up with a theoretical fix for a theoretical problem. Something
> > that is easy to introduce but much harder to get rid of should we ever
> > need to change __GFP_RETRY_MAYFAIL implementation for example.
> 
> I will add this to changelog in v3. If  __GFP_RETRY_MAYFAIL is
> ineffective we will receive feedback once someone hits this problem.

I do not remember anybody hitting this with the current __GFP_NORETRY.
So arguably there is nothing to be fixed ATM.

> Otherwise, we will never hear about it. I think overall it is safer to
> keep this code with __GFP_RETRY_MAYFAIL flag.
> 
> >
> > > In an ideal scenario we should guarantee that this never fails: that
> > > we always can free HugeTLB pages back to the system. At the very least
> > > we could steal the memory for vmemmap from the page that is being
> > > released.
> >
> > Yes, this really bothered me when the concept was introduced initially.
> > I am always concerned when you need to allocate in order to free memory.
> > Practically speaking we haven't heard about bug reports so maybe this is
> > not such a big deal as I thought.
> 
> I suspect this is because at the moment it is not that frequent when a
> machine is reconfigured from having a lot of HugeTLB based workload to
> non-HugeTLB workload.

Yes, hugetlb workloads tend to be pretty static from my experience.
David Rientjes April 15, 2023, 12:47 a.m. UTC | #6 
On Thu, 13 Apr 2023, Michal Hocko wrote:

> [...]
> > > > This is a theoretical concern. Freeing a 1G page requires 16M of free
> > > > memory. A machine might need to be reconfigured from one task to
> > > > another, and release a large number of 1G pages back to the system if
> > > > allocating 16M fails, the release won't work.
> > >
> > > This is really an important "detail" changelog should mention. While I
> > > am not really against that change I would much rather see that as a
> > > result of a real world fix rather than a theoretical concern. Mostly
> > > because a real life scenario would allow us to test the
> > > __GFP_RETRY_MAYFAIL effectivness. As that request might fail as well we
> > > just end up with a theoretical fix for a theoretical problem. Something
> > > that is easy to introduce but much harder to get rid of should we ever
> > > need to change __GFP_RETRY_MAYFAIL implementation for example.
> > 
> > I will add this to changelog in v3. If  __GFP_RETRY_MAYFAIL is
> > ineffective we will receive feedback once someone hits this problem.
> 
> I do not remember anybody hitting this with the current __GFP_NORETRY.
> So arguably there is nothing to be fixed ATM.
> 

I think we should still at least clear __GFP_NORETRY in this allocation: 
to be able to free 1GB hugepages back to the system we'd like the page 
allocator to at least exercise its normal order-0 allocation logic rather 
than exempting it from retrying reclaim by opting into __GFP_NORETRY.

I'd agree with the analysis in 
https://lore.kernel.org/linux-mm/YCafit5ruRJ+SL8I@dhcp22.suse.cz/ that 
either a cleared __GFP_NORETRY or a __GFP_RETRY_MAYFAIL makes logical 
sense.

We really *do* want to free these hugepages back to the system and the 
amount of memory freeing will always be more than the allocation for 
struct page.  The net result is more free memory.

If the allocation fails, we can't free 1GB back to the system on a 
saturated node if our first reclaim attempt didn't allow these struct 
pages to be allocated.  Stranding 1GB in the hugetlb pool that no 
userspace on the system can make use of at the time isn't very useful.
Michal Hocko April 17, 2023, 8:33 a.m. UTC | #7 
On Fri 14-04-23 17:47:28, David Rientjes wrote:
> On Thu, 13 Apr 2023, Michal Hocko wrote:
> 
> > [...]
> > > > > This is a theoretical concern. Freeing a 1G page requires 16M of free
> > > > > memory. A machine might need to be reconfigured from one task to
> > > > > another, and release a large number of 1G pages back to the system if
> > > > > allocating 16M fails, the release won't work.
> > > >
> > > > This is really an important "detail" changelog should mention. While I
> > > > am not really against that change I would much rather see that as a
> > > > result of a real world fix rather than a theoretical concern. Mostly
> > > > because a real life scenario would allow us to test the
> > > > __GFP_RETRY_MAYFAIL effectivness. As that request might fail as well we
> > > > just end up with a theoretical fix for a theoretical problem. Something
> > > > that is easy to introduce but much harder to get rid of should we ever
> > > > need to change __GFP_RETRY_MAYFAIL implementation for example.
> > > 
> > > I will add this to changelog in v3. If  __GFP_RETRY_MAYFAIL is
> > > ineffective we will receive feedback once someone hits this problem.
> > 
> > I do not remember anybody hitting this with the current __GFP_NORETRY.
> > So arguably there is nothing to be fixed ATM.
> > 
> 
> I think we should still at least clear __GFP_NORETRY in this allocation: 
> to be able to free 1GB hugepages back to the system we'd like the page 
> allocator to at least exercise its normal order-0 allocation logic rather 
> than exempting it from retrying reclaim by opting into __GFP_NORETRY.
> 
> I'd agree with the analysis in 
> https://lore.kernel.org/linux-mm/YCafit5ruRJ+SL8I@dhcp22.suse.cz/ that 
> either a cleared __GFP_NORETRY or a __GFP_RETRY_MAYFAIL makes logical 
> sense.
> 
> We really *do* want to free these hugepages back to the system and the 
> amount of memory freeing will always be more than the allocation for 
> struct page.  The net result is more free memory.
> 
> If the allocation fails, we can't free 1GB back to the system on a 
> saturated node if our first reclaim attempt didn't allow these struct 
> pages to be allocated.  Stranding 1GB in the hugetlb pool that no 
> userspace on the system can make use of at the time isn't very useful.

I do not think there is any dispute in the theoretical concern. The question is
whether this is something that really needs a fix in practice. Have we
ever seen workloads which rely on GB pages to fail freeing them?
Mike Kravetz April 17, 2023, 6:51 p.m. UTC | #8 
On 04/17/23 10:33, Michal Hocko wrote:
> On Fri 14-04-23 17:47:28, David Rientjes wrote:
> > On Thu, 13 Apr 2023, Michal Hocko wrote:
> > 
> > > [...]
> > > > > > This is a theoretical concern. Freeing a 1G page requires 16M of free
> > > > > > memory. A machine might need to be reconfigured from one task to
> > > > > > another, and release a large number of 1G pages back to the system if
> > > > > > allocating 16M fails, the release won't work.
> > > > >
> > > > > This is really an important "detail" changelog should mention. While I
> > > > > am not really against that change I would much rather see that as a
> > > > > result of a real world fix rather than a theoretical concern. Mostly
> > > > > because a real life scenario would allow us to test the
> > > > > __GFP_RETRY_MAYFAIL effectivness. As that request might fail as well we
> > > > > just end up with a theoretical fix for a theoretical problem. Something
> > > > > that is easy to introduce but much harder to get rid of should we ever
> > > > > need to change __GFP_RETRY_MAYFAIL implementation for example.
> > > > 
> > > > I will add this to changelog in v3. If  __GFP_RETRY_MAYFAIL is
> > > > ineffective we will receive feedback once someone hits this problem.
> > > 
> > > I do not remember anybody hitting this with the current __GFP_NORETRY.
> > > So arguably there is nothing to be fixed ATM.
> > > 
> > 
> > I think we should still at least clear __GFP_NORETRY in this allocation: 
> > to be able to free 1GB hugepages back to the system we'd like the page 
> > allocator to at least exercise its normal order-0 allocation logic rather 
> > than exempting it from retrying reclaim by opting into __GFP_NORETRY.
> > 
> > I'd agree with the analysis in 
> > https://lore.kernel.org/linux-mm/YCafit5ruRJ+SL8I@dhcp22.suse.cz/ that 
> > either a cleared __GFP_NORETRY or a __GFP_RETRY_MAYFAIL makes logical 
> > sense.
> > 
> > We really *do* want to free these hugepages back to the system and the 
> > amount of memory freeing will always be more than the allocation for 
> > struct page.  The net result is more free memory.
> > 
> > If the allocation fails, we can't free 1GB back to the system on a 
> > saturated node if our first reclaim attempt didn't allow these struct 
> > pages to be allocated.  Stranding 1GB in the hugetlb pool that no 
> > userspace on the system can make use of at the time isn't very useful.
> 
> I do not think there is any dispute in the theoretical concern. The question is
> whether this is something that really needs a fix in practice. Have we
> ever seen workloads which rely on GB pages to fail freeing them?

Since I have never seen a failure allocating vmemmmap, I agree that this
is all a theoretical concern.

However, to me it seems that replacing __GFP_NORETRY with __GFP_RETRY_MAYFAIL
would lessen that theoretical concern just a little.  That is simply because
an allocation with __GFP_RETRY_MAYFAIL would be a little more likely to
succeed.

Again, I know this is all theoretical but if switching to __GFP_RETRY_MAYFAIL
would prevent one allocation/hugetlb page freeing failure I think it is worth
it.  Because, as soon as we see one failure we may need to look into addressing
this now theoretical concern.
diff mbox series

Patch

diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
index a559037cce00..236647e4bfec 100644
--- a/mm/hugetlb_vmemmap.c
+++ b/mm/hugetlb_vmemmap.c
@@ -384,8 +384,9 @@  static int vmemmap_remap_free(unsigned long start, unsigned long end,
 }
 
 static int alloc_vmemmap_page_list(unsigned long start, unsigned long end,
-				   gfp_t gfp_mask, struct list_head *list)
+				   struct list_head *list)
 {
+	gfp_t gfp_mask = GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_THISNODE;
 	unsigned long nr_pages = (end - start) >> PAGE_SHIFT;
 	int nid = page_to_nid((struct page *)start);
 	struct page *page, *next;
@@ -413,12 +414,11 @@  static int alloc_vmemmap_page_list(unsigned long start, unsigned long end,
  * @end:	end address of the vmemmap virtual address range that we want to
  *		remap.
  * @reuse:	reuse address.
- * @gfp_mask:	GFP flag for allocating vmemmap pages.
  *
  * Return: %0 on success, negative error code otherwise.
  */
 static int vmemmap_remap_alloc(unsigned long start, unsigned long end,
-			       unsigned long reuse, gfp_t gfp_mask)
+			       unsigned long reuse)
 {
 	LIST_HEAD(vmemmap_pages);
 	struct vmemmap_remap_walk walk = {
@@ -430,7 +430,7 @@  static int vmemmap_remap_alloc(unsigned long start, unsigned long end,
 	/* See the comment in the vmemmap_remap_free(). */
 	BUG_ON(start - reuse != PAGE_SIZE);
 
-	if (alloc_vmemmap_page_list(start, end, gfp_mask, &vmemmap_pages))
+	if (alloc_vmemmap_page_list(start, end, &vmemmap_pages))
 		return -ENOMEM;
 
 	mmap_read_lock(&init_mm);
@@ -476,8 +476,7 @@  int hugetlb_vmemmap_restore(const struct hstate *h, struct page *head)
 	 * When a HugeTLB page is freed to the buddy allocator, previously
 	 * discarded vmemmap pages must be allocated and remapping.
 	 */
-	ret = vmemmap_remap_alloc(vmemmap_start, vmemmap_end, vmemmap_reuse,
-				  GFP_KERNEL | __GFP_NORETRY | __GFP_THISNODE);
+	ret = vmemmap_remap_alloc(vmemmap_start, vmemmap_end, vmemmap_reuse);
 	if (!ret) {
 		ClearHPageVmemmapOptimized(head);
 		static_branch_dec(&hugetlb_optimize_vmemmap_key);