| Message ID | 20230412195939.1242462-1-pasha.tatashin@soleen.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm: hugetlb_vmemmap: provide stronger vmemmap allocation guarantees | expand |
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)?
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?
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
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
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.
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
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.
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.
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?
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 --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);
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.