From: Michal Hocko <[email protected]>
Dave Chinner has mentioned that some of the xfs code would benefit from
kvmalloc support for __GFP_NOFAIL because they have allocations that
cannot fail and they do not fit into a single page.
The larg part of the vmalloc implementation already complies with the
given gfp flags so there is no work for those to be done. The area
and page table allocations are an exception to that. Implement a retry
loop for those.
Add a short sleep before retrying. 1 jiffy is a completely random
timeout. Ideally the retry would wait for an explicit event - e.g.
a change to the vmalloc space change if the failure was caused by
the space fragmentation or depletion. But there are multiple different
reasons to retry and this could become much more complex. Keep the retry
simple for now and just sleep to prevent from hogging CPUs.
Signed-off-by: Michal Hocko <[email protected]>
---
mm/vmalloc.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index c6cc77d2f366..602649919a9d 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2941,8 +2941,12 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
else if ((gfp_mask & (__GFP_FS | __GFP_IO)) == 0)
flags = memalloc_noio_save();
- ret = vmap_pages_range(addr, addr + size, prot, area->pages,
+ do {
+ ret = vmap_pages_range(addr, addr + size, prot, area->pages,
page_shift);
+ if (ret < 0)
+ schedule_timeout_uninterruptible(1);
+ } while ((gfp_mask & __GFP_NOFAIL) && (ret < 0));
if ((gfp_mask & (__GFP_FS | __GFP_IO)) == __GFP_IO)
memalloc_nofs_restore(flags);
@@ -3032,6 +3036,10 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
warn_alloc(gfp_mask, NULL,
"vmalloc error: size %lu, vm_struct allocation failed",
real_size);
+ if (gfp_mask & __GFP_NOFAIL) {
+ schedule_timeout_uninterruptible(1);
+ goto again;
+ }
goto fail;
}
--
2.30.2
On Tue, 26 Oct 2021, Michal Hocko wrote:
> From: Michal Hocko <[email protected]>
>
> Dave Chinner has mentioned that some of the xfs code would benefit from
> kvmalloc support for __GFP_NOFAIL because they have allocations that
> cannot fail and they do not fit into a single page.
>
> The larg part of the vmalloc implementation already complies with the
*large*
> given gfp flags so there is no work for those to be done. The area
> and page table allocations are an exception to that. Implement a retry
> loop for those.
>
> Add a short sleep before retrying. 1 jiffy is a completely random
> timeout. Ideally the retry would wait for an explicit event - e.g.
> a change to the vmalloc space change if the failure was caused by
> the space fragmentation or depletion. But there are multiple different
> reasons to retry and this could become much more complex. Keep the retry
> simple for now and just sleep to prevent from hogging CPUs.
>
> Signed-off-by: Michal Hocko <[email protected]>
> ---
> mm/vmalloc.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index c6cc77d2f366..602649919a9d 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2941,8 +2941,12 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> else if ((gfp_mask & (__GFP_FS | __GFP_IO)) == 0)
> flags = memalloc_noio_save();
>
> - ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> + do {
> + ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> page_shift);
> + if (ret < 0)
> + schedule_timeout_uninterruptible(1);
> + } while ((gfp_mask & __GFP_NOFAIL) && (ret < 0));
>
> if ((gfp_mask & (__GFP_FS | __GFP_IO)) == __GFP_IO)
> memalloc_nofs_restore(flags);
> @@ -3032,6 +3036,10 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> warn_alloc(gfp_mask, NULL,
> "vmalloc error: size %lu, vm_struct allocation failed",
> real_size);
> + if (gfp_mask & __GFP_NOFAIL) {
> + schedule_timeout_uninterruptible(1);
> + goto again;
> + }
Shouldn't the retry happen *before* the warning?
NeilBrown
> goto fail;
> }
>
> --
> 2.30.2
>
>
On Tue 26-10-21 09:59:36, Neil Brown wrote:
> On Tue, 26 Oct 2021, Michal Hocko wrote:
[...]
> > @@ -3032,6 +3036,10 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> > warn_alloc(gfp_mask, NULL,
> > "vmalloc error: size %lu, vm_struct allocation failed",
> > real_size);
> > + if (gfp_mask & __GFP_NOFAIL) {
> > + schedule_timeout_uninterruptible(1);
> > + goto again;
> > + }
>
> Shouldn't the retry happen *before* the warning?
I've done it after to catch the "depleted or fragmented" vmalloc space.
This is not related to the memory available and therefore it won't be
handled by the oom killer. The error message shouldn't imply the vmalloc
allocation failure IMHO but I am open to suggestions.
--
Michal Hocko
SUSE Labs
On Tue, 26 Oct 2021, Michal Hocko wrote:
> On Tue 26-10-21 09:59:36, Neil Brown wrote:
> > On Tue, 26 Oct 2021, Michal Hocko wrote:
> [...]
> > > @@ -3032,6 +3036,10 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> > > warn_alloc(gfp_mask, NULL,
> > > "vmalloc error: size %lu, vm_struct allocation failed",
> > > real_size);
> > > + if (gfp_mask & __GFP_NOFAIL) {
> > > + schedule_timeout_uninterruptible(1);
> > > + goto again;
> > > + }
> >
> > Shouldn't the retry happen *before* the warning?
>
> I've done it after to catch the "depleted or fragmented" vmalloc space.
> This is not related to the memory available and therefore it won't be
> handled by the oom killer. The error message shouldn't imply the vmalloc
> allocation failure IMHO but I am open to suggestions.
The word "failed" does seem to imply what you don't want it to imply...
I guess it is reasonable to have this warning, but maybe add " -- retrying"
if __GFP_NOFAIL.
Thanks,
NeilBrown
On Tue 26-10-21 21:30:52, Neil Brown wrote:
> On Tue, 26 Oct 2021, Michal Hocko wrote:
> > On Tue 26-10-21 09:59:36, Neil Brown wrote:
> > > On Tue, 26 Oct 2021, Michal Hocko wrote:
> > [...]
> > > > @@ -3032,6 +3036,10 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> > > > warn_alloc(gfp_mask, NULL,
> > > > "vmalloc error: size %lu, vm_struct allocation failed",
> > > > real_size);
> > > > + if (gfp_mask & __GFP_NOFAIL) {
> > > > + schedule_timeout_uninterruptible(1);
> > > > + goto again;
> > > > + }
> > >
> > > Shouldn't the retry happen *before* the warning?
> >
> > I've done it after to catch the "depleted or fragmented" vmalloc space.
> > This is not related to the memory available and therefore it won't be
> > handled by the oom killer. The error message shouldn't imply the vmalloc
> > allocation failure IMHO but I am open to suggestions.
>
> The word "failed" does seem to imply what you don't want it to imply...
>
> I guess it is reasonable to have this warning, but maybe add " -- retrying"
> if __GFP_NOFAIL.
I do not have a strong opinion on that. I can surely do
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 602649919a9d..3489928fafa2 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -3033,10 +3033,11 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
VM_UNINITIALIZED | vm_flags, start, end, node,
gfp_mask, caller);
if (!area) {
+ bool nofail = gfp_mask & __GFP_NOFAIL;
warn_alloc(gfp_mask, NULL,
- "vmalloc error: size %lu, vm_struct allocation failed",
- real_size);
- if (gfp_mask & __GFP_NOFAIL) {
+ "vmalloc error: size %lu, vm_struct allocation failed%s",
+ real_size, (nofail) ? ". Retrying." : "");
+ if (nofail) {
schedule_timeout_uninterruptible(1);
goto again;
}
--
Michal Hocko
SUSE Labs
> From: Michal Hocko <[email protected]>
>
> Dave Chinner has mentioned that some of the xfs code would benefit from
> kvmalloc support for __GFP_NOFAIL because they have allocations that
> cannot fail and they do not fit into a single page.
>
> The larg part of the vmalloc implementation already complies with the
> given gfp flags so there is no work for those to be done. The area
> and page table allocations are an exception to that. Implement a retry
> loop for those.
>
> Add a short sleep before retrying. 1 jiffy is a completely random
> timeout. Ideally the retry would wait for an explicit event - e.g.
> a change to the vmalloc space change if the failure was caused by
> the space fragmentation or depletion. But there are multiple different
> reasons to retry and this could become much more complex. Keep the retry
> simple for now and just sleep to prevent from hogging CPUs.
>
> Signed-off-by: Michal Hocko <[email protected]>
> ---
> mm/vmalloc.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index c6cc77d2f366..602649919a9d 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2941,8 +2941,12 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> else if ((gfp_mask & (__GFP_FS | __GFP_IO)) == 0)
> flags = memalloc_noio_save();
>
> - ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> + do {
> + ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> page_shift);
> + if (ret < 0)
> + schedule_timeout_uninterruptible(1);
> + } while ((gfp_mask & __GFP_NOFAIL) && (ret < 0));
>
1.
After that change a below code:
<snip>
if (ret < 0) {
warn_alloc(orig_gfp_mask, NULL,
"vmalloc error: size %lu, failed to map pages",
area->nr_pages * PAGE_SIZE);
goto fail;
}
<snip>
does not make any sense anymore.
2.
Can we combine two places where we handle __GFP_NOFAIL into one place?
That would look like as more sorted out.
--
Uladzislau Rezki
On Tue 26-10-21 17:48:32, Uladzislau Rezki wrote:
> > From: Michal Hocko <[email protected]>
> >
> > Dave Chinner has mentioned that some of the xfs code would benefit from
> > kvmalloc support for __GFP_NOFAIL because they have allocations that
> > cannot fail and they do not fit into a single page.
> >
> > The larg part of the vmalloc implementation already complies with the
> > given gfp flags so there is no work for those to be done. The area
> > and page table allocations are an exception to that. Implement a retry
> > loop for those.
> >
> > Add a short sleep before retrying. 1 jiffy is a completely random
> > timeout. Ideally the retry would wait for an explicit event - e.g.
> > a change to the vmalloc space change if the failure was caused by
> > the space fragmentation or depletion. But there are multiple different
> > reasons to retry and this could become much more complex. Keep the retry
> > simple for now and just sleep to prevent from hogging CPUs.
> >
> > Signed-off-by: Michal Hocko <[email protected]>
> > ---
> > mm/vmalloc.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index c6cc77d2f366..602649919a9d 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -2941,8 +2941,12 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > else if ((gfp_mask & (__GFP_FS | __GFP_IO)) == 0)
> > flags = memalloc_noio_save();
> >
> > - ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> > + do {
> > + ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> > page_shift);
> > + if (ret < 0)
> > + schedule_timeout_uninterruptible(1);
> > + } while ((gfp_mask & __GFP_NOFAIL) && (ret < 0));
> >
>
> 1.
> After that change a below code:
>
> <snip>
> if (ret < 0) {
> warn_alloc(orig_gfp_mask, NULL,
> "vmalloc error: size %lu, failed to map pages",
> area->nr_pages * PAGE_SIZE);
> goto fail;
> }
> <snip>
>
> does not make any sense anymore.
Why? Allocations without __GFP_NOFAIL can still fail, no?
> 2.
> Can we combine two places where we handle __GFP_NOFAIL into one place?
> That would look like as more sorted out.
I have to admit I am not really fluent at vmalloc code so I wanted to
make the code as simple as possible. How would I unwind all the allocated
memory (already allocated as GFP_NOFAIL) before retrying at
__vmalloc_node_range (if that is what you suggest). And isn't that a
bit wasteful?
Or did you have anything else in mind?
--
Michal Hocko
SUSE Labs
On Tue, Oct 26, 2021 at 06:28:52PM +0200, Michal Hocko wrote:
> On Tue 26-10-21 17:48:32, Uladzislau Rezki wrote:
> > > From: Michal Hocko <[email protected]>
> > >
> > > Dave Chinner has mentioned that some of the xfs code would benefit from
> > > kvmalloc support for __GFP_NOFAIL because they have allocations that
> > > cannot fail and they do not fit into a single page.
> > >
> > > The larg part of the vmalloc implementation already complies with the
> > > given gfp flags so there is no work for those to be done. The area
> > > and page table allocations are an exception to that. Implement a retry
> > > loop for those.
> > >
> > > Add a short sleep before retrying. 1 jiffy is a completely random
> > > timeout. Ideally the retry would wait for an explicit event - e.g.
> > > a change to the vmalloc space change if the failure was caused by
> > > the space fragmentation or depletion. But there are multiple different
> > > reasons to retry and this could become much more complex. Keep the retry
> > > simple for now and just sleep to prevent from hogging CPUs.
> > >
> > > Signed-off-by: Michal Hocko <[email protected]>
> > > ---
> > > mm/vmalloc.c | 10 +++++++++-
> > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > index c6cc77d2f366..602649919a9d 100644
> > > --- a/mm/vmalloc.c
> > > +++ b/mm/vmalloc.c
> > > @@ -2941,8 +2941,12 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > > else if ((gfp_mask & (__GFP_FS | __GFP_IO)) == 0)
> > > flags = memalloc_noio_save();
> > >
> > > - ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> > > + do {
> > > + ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> > > page_shift);
> > > + if (ret < 0)
> > > + schedule_timeout_uninterruptible(1);
> > > + } while ((gfp_mask & __GFP_NOFAIL) && (ret < 0));
> > >
> >
> > 1.
> > After that change a below code:
> >
> > <snip>
> > if (ret < 0) {
> > warn_alloc(orig_gfp_mask, NULL,
> > "vmalloc error: size %lu, failed to map pages",
> > area->nr_pages * PAGE_SIZE);
> > goto fail;
> > }
> > <snip>
> >
> > does not make any sense anymore.
>
> Why? Allocations without __GFP_NOFAIL can still fail, no?
>
Right. I meant one thing but wrote slightly differently. In case of
vmap_pages_range() fails(if __GFP_NOFAIL is set) should we emit any
warning message? Because either we can recover on a future iteration
or it stuck there infinitely so a user does not understand what happened.
From the other hand this is how __GFP_NOFAIL works, hm..
Another thing, i see that schedule_timeout_uninterruptible(1) is invoked
for all cases even when __GFP_NOFAIL is not set, in that scenario we do
not want to wait, instead we should return back to a caller asap. Or am
i missing something here?
> > 2.
> > Can we combine two places where we handle __GFP_NOFAIL into one place?
> > That would look like as more sorted out.
>
> I have to admit I am not really fluent at vmalloc code so I wanted to
> make the code as simple as possible. How would I unwind all the allocated
> memory (already allocated as GFP_NOFAIL) before retrying at
> __vmalloc_node_range (if that is what you suggest). And isn't that a
> bit wasteful?
>
> Or did you have anything else in mind?
>
It depends on how often all this can fail. But let me double check if
such combining is easy.
--
Vlad Rezki
On Tue 26-10-21 21:33:15, Uladzislau Rezki wrote:
> On Tue, Oct 26, 2021 at 06:28:52PM +0200, Michal Hocko wrote:
> > On Tue 26-10-21 17:48:32, Uladzislau Rezki wrote:
> > > > From: Michal Hocko <[email protected]>
> > > >
> > > > Dave Chinner has mentioned that some of the xfs code would benefit from
> > > > kvmalloc support for __GFP_NOFAIL because they have allocations that
> > > > cannot fail and they do not fit into a single page.
> > > >
> > > > The larg part of the vmalloc implementation already complies with the
> > > > given gfp flags so there is no work for those to be done. The area
> > > > and page table allocations are an exception to that. Implement a retry
> > > > loop for those.
> > > >
> > > > Add a short sleep before retrying. 1 jiffy is a completely random
> > > > timeout. Ideally the retry would wait for an explicit event - e.g.
> > > > a change to the vmalloc space change if the failure was caused by
> > > > the space fragmentation or depletion. But there are multiple different
> > > > reasons to retry and this could become much more complex. Keep the retry
> > > > simple for now and just sleep to prevent from hogging CPUs.
> > > >
> > > > Signed-off-by: Michal Hocko <[email protected]>
> > > > ---
> > > > mm/vmalloc.c | 10 +++++++++-
> > > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > > index c6cc77d2f366..602649919a9d 100644
> > > > --- a/mm/vmalloc.c
> > > > +++ b/mm/vmalloc.c
> > > > @@ -2941,8 +2941,12 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > > > else if ((gfp_mask & (__GFP_FS | __GFP_IO)) == 0)
> > > > flags = memalloc_noio_save();
> > > >
> > > > - ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> > > > + do {
> > > > + ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> > > > page_shift);
> > > > + if (ret < 0)
> > > > + schedule_timeout_uninterruptible(1);
> > > > + } while ((gfp_mask & __GFP_NOFAIL) && (ret < 0));
> > > >
> > >
> > > 1.
> > > After that change a below code:
> > >
> > > <snip>
> > > if (ret < 0) {
> > > warn_alloc(orig_gfp_mask, NULL,
> > > "vmalloc error: size %lu, failed to map pages",
> > > area->nr_pages * PAGE_SIZE);
> > > goto fail;
> > > }
> > > <snip>
> > >
> > > does not make any sense anymore.
> >
> > Why? Allocations without __GFP_NOFAIL can still fail, no?
> >
> Right. I meant one thing but wrote slightly differently. In case of
> vmap_pages_range() fails(if __GFP_NOFAIL is set) should we emit any
> warning message? Because either we can recover on a future iteration
> or it stuck there infinitely so a user does not understand what happened.
> From the other hand this is how __GFP_NOFAIL works, hm..
Yes, the page allocator doesn't warn either and I would like to keep
this in sync.
> Another thing, i see that schedule_timeout_uninterruptible(1) is invoked
> for all cases even when __GFP_NOFAIL is not set, in that scenario we do
> not want to wait, instead we should return back to a caller asap. Or am
> i missing something here?
OK, I will change that.
--
Michal Hocko
SUSE Labs
On Tue, Oct 26, 2021 at 09:33:15PM +0200, Uladzislau Rezki wrote:
> On Tue, Oct 26, 2021 at 06:28:52PM +0200, Michal Hocko wrote:
> > On Tue 26-10-21 17:48:32, Uladzislau Rezki wrote:
> > > > From: Michal Hocko <[email protected]>
> > > >
> > > > Dave Chinner has mentioned that some of the xfs code would benefit from
> > > > kvmalloc support for __GFP_NOFAIL because they have allocations that
> > > > cannot fail and they do not fit into a single page.
> > > >
> > > > The larg part of the vmalloc implementation already complies with the
> > > > given gfp flags so there is no work for those to be done. The area
> > > > and page table allocations are an exception to that. Implement a retry
> > > > loop for those.
> > > >
> > > > Add a short sleep before retrying. 1 jiffy is a completely random
> > > > timeout. Ideally the retry would wait for an explicit event - e.g.
> > > > a change to the vmalloc space change if the failure was caused by
> > > > the space fragmentation or depletion. But there are multiple different
> > > > reasons to retry and this could become much more complex. Keep the retry
> > > > simple for now and just sleep to prevent from hogging CPUs.
> > > >
> > > > Signed-off-by: Michal Hocko <[email protected]>
> > > > ---
> > > > mm/vmalloc.c | 10 +++++++++-
> > > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > > index c6cc77d2f366..602649919a9d 100644
> > > > --- a/mm/vmalloc.c
> > > > +++ b/mm/vmalloc.c
> > > > @@ -2941,8 +2941,12 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > > > else if ((gfp_mask & (__GFP_FS | __GFP_IO)) == 0)
> > > > flags = memalloc_noio_save();
> > > >
> > > > - ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> > > > + do {
> > > > + ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> > > > page_shift);
> > > > + if (ret < 0)
> > > > + schedule_timeout_uninterruptible(1);
> > > > + } while ((gfp_mask & __GFP_NOFAIL) && (ret < 0));
> > > >
> > >
> > > 1.
> > > After that change a below code:
> > >
> > > <snip>
> > > if (ret < 0) {
> > > warn_alloc(orig_gfp_mask, NULL,
> > > "vmalloc error: size %lu, failed to map pages",
> > > area->nr_pages * PAGE_SIZE);
> > > goto fail;
> > > }
> > > <snip>
> > >
> > > does not make any sense anymore.
> >
> > Why? Allocations without __GFP_NOFAIL can still fail, no?
> >
> Right. I meant one thing but wrote slightly differently. In case of
> vmap_pages_range() fails(if __GFP_NOFAIL is set) should we emit any
> warning message? Because either we can recover on a future iteration
> or it stuck there infinitely so a user does not understand what happened.
> From the other hand this is how __GFP_NOFAIL works, hm..
>
> Another thing, i see that schedule_timeout_uninterruptible(1) is invoked
> for all cases even when __GFP_NOFAIL is not set, in that scenario we do
> not want to wait, instead we should return back to a caller asap. Or am
> i missing something here?
>
> > > 2.
> > > Can we combine two places where we handle __GFP_NOFAIL into one place?
> > > That would look like as more sorted out.
> >
> > I have to admit I am not really fluent at vmalloc code so I wanted to
> > make the code as simple as possible. How would I unwind all the allocated
> > memory (already allocated as GFP_NOFAIL) before retrying at
> > __vmalloc_node_range (if that is what you suggest). And isn't that a
> > bit wasteful?
> >
> > Or did you have anything else in mind?
> >
> It depends on how often all this can fail. But let me double check if
> such combining is easy.
>
I mean something like below. The idea is to not spread the __GFP_NOFAIL
across the vmalloc file keeping it in one solid place:
<snip>
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index d77830ff604c..f4b7927e217e 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2889,8 +2889,14 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
unsigned long array_size;
unsigned int nr_small_pages = size >> PAGE_SHIFT;
unsigned int page_order;
+ unsigned long flags;
+ int ret;
array_size = (unsigned long)nr_small_pages * sizeof(struct page *);
+
+ /*
+ * This is i do not understand why we do not want to see warning messages.
+ */
gfp_mask |= __GFP_NOWARN;
if (!(gfp_mask & (GFP_DMA | GFP_DMA32)))
gfp_mask |= __GFP_HIGHMEM;
@@ -2930,8 +2936,23 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
goto fail;
}
- if (vmap_pages_range(addr, addr + size, prot, area->pages,
- page_shift) < 0) {
+ /*
+ * page tables allocations ignore external gfp mask, enforce it
+ * by the scope API
+ */
+ if ((gfp_mask & (__GFP_FS | __GFP_IO)) == __GFP_IO)
+ flags = memalloc_nofs_save();
+ else if ((gfp_mask & (__GFP_FS | __GFP_IO)) == 0)
+ flags = memalloc_noio_save();
+
+ ret = vmap_pages_range(addr, addr + size, prot, area->pages, page_shift);
+
+ if ((gfp_mask & (__GFP_FS | __GFP_IO)) == __GFP_IO)
+ memalloc_nofs_restore(flags);
+ else if ((gfp_mask & (__GFP_FS | __GFP_IO)) == 0)
+ memalloc_noio_restore(flags);
+
+ if (ret < 0) {
warn_alloc(gfp_mask, NULL,
"vmalloc error: size %lu, failed to map pages",
area->nr_pages * PAGE_SIZE);
@@ -2984,6 +3005,12 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
return NULL;
}
+ /*
+ * Suppress all warnings for __GFP_NOFAIL allocation.
+ */
+ if (gfp_mask & __GFP_NOFAIL)
+ gfp_mask |= __GFP_NOWARN;
+
if (vmap_allow_huge && !(vm_flags & VM_NO_HUGE_VMAP)) {
unsigned long size_per_node;
@@ -3010,16 +3037,22 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
area = __get_vm_area_node(real_size, align, shift, VM_ALLOC |
VM_UNINITIALIZED | vm_flags, start, end, node,
gfp_mask, caller);
- if (!area) {
- warn_alloc(gfp_mask, NULL,
- "vmalloc error: size %lu, vm_struct allocation failed",
- real_size);
- goto fail;
- }
+ if (area)
+ addr = __vmalloc_area_node(area, gfp_mask, prot, shift, node);
+
+ if (!area || !addr) {
+ if (gfp_mask & __GFP_NOFAIL) {
+ schedule_timeout_uninterruptible(1);
+ goto again;
+ }
+
+ if (!area)
+ warn_alloc(gfp_mask, NULL,
+ "vmalloc error: size %lu, vm_struct allocation failed",
+ real_size);
- addr = __vmalloc_area_node(area, gfp_mask, prot, shift, node);
- if (!addr)
goto fail;
+ }
/*
* In this function, newly allocated vm_struct has VM_UNINITIALIZED
<snip>
--
Vlad Rezki
On Wed 27-10-21 19:55:50, Uladzislau Rezki wrote:
> On Tue, Oct 26, 2021 at 09:33:15PM +0200, Uladzislau Rezki wrote:
> > On Tue, Oct 26, 2021 at 06:28:52PM +0200, Michal Hocko wrote:
> > > On Tue 26-10-21 17:48:32, Uladzislau Rezki wrote:
> > > > > From: Michal Hocko <[email protected]>
> > > > >
> > > > > Dave Chinner has mentioned that some of the xfs code would benefit from
> > > > > kvmalloc support for __GFP_NOFAIL because they have allocations that
> > > > > cannot fail and they do not fit into a single page.
> > > > >
> > > > > The larg part of the vmalloc implementation already complies with the
> > > > > given gfp flags so there is no work for those to be done. The area
> > > > > and page table allocations are an exception to that. Implement a retry
> > > > > loop for those.
> > > > >
> > > > > Add a short sleep before retrying. 1 jiffy is a completely random
> > > > > timeout. Ideally the retry would wait for an explicit event - e.g.
> > > > > a change to the vmalloc space change if the failure was caused by
> > > > > the space fragmentation or depletion. But there are multiple different
> > > > > reasons to retry and this could become much more complex. Keep the retry
> > > > > simple for now and just sleep to prevent from hogging CPUs.
> > > > >
> > > > > Signed-off-by: Michal Hocko <[email protected]>
> > > > > ---
> > > > > mm/vmalloc.c | 10 +++++++++-
> > > > > 1 file changed, 9 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > > > index c6cc77d2f366..602649919a9d 100644
> > > > > --- a/mm/vmalloc.c
> > > > > +++ b/mm/vmalloc.c
> > > > > @@ -2941,8 +2941,12 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > > > > else if ((gfp_mask & (__GFP_FS | __GFP_IO)) == 0)
> > > > > flags = memalloc_noio_save();
> > > > >
> > > > > - ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> > > > > + do {
> > > > > + ret = vmap_pages_range(addr, addr + size, prot, area->pages,
> > > > > page_shift);
> > > > > + if (ret < 0)
> > > > > + schedule_timeout_uninterruptible(1);
> > > > > + } while ((gfp_mask & __GFP_NOFAIL) && (ret < 0));
> > > > >
> > > >
> > > > 1.
> > > > After that change a below code:
> > > >
> > > > <snip>
> > > > if (ret < 0) {
> > > > warn_alloc(orig_gfp_mask, NULL,
> > > > "vmalloc error: size %lu, failed to map pages",
> > > > area->nr_pages * PAGE_SIZE);
> > > > goto fail;
> > > > }
> > > > <snip>
> > > >
> > > > does not make any sense anymore.
> > >
> > > Why? Allocations without __GFP_NOFAIL can still fail, no?
> > >
> > Right. I meant one thing but wrote slightly differently. In case of
> > vmap_pages_range() fails(if __GFP_NOFAIL is set) should we emit any
> > warning message? Because either we can recover on a future iteration
> > or it stuck there infinitely so a user does not understand what happened.
> > From the other hand this is how __GFP_NOFAIL works, hm..
> >
> > Another thing, i see that schedule_timeout_uninterruptible(1) is invoked
> > for all cases even when __GFP_NOFAIL is not set, in that scenario we do
> > not want to wait, instead we should return back to a caller asap. Or am
> > i missing something here?
> >
> > > > 2.
> > > > Can we combine two places where we handle __GFP_NOFAIL into one place?
> > > > That would look like as more sorted out.
> > >
> > > I have to admit I am not really fluent at vmalloc code so I wanted to
> > > make the code as simple as possible. How would I unwind all the allocated
> > > memory (already allocated as GFP_NOFAIL) before retrying at
> > > __vmalloc_node_range (if that is what you suggest). And isn't that a
> > > bit wasteful?
> > >
> > > Or did you have anything else in mind?
> > >
> > It depends on how often all this can fail. But let me double check if
> > such combining is easy.
> >
> I mean something like below. The idea is to not spread the __GFP_NOFAIL
> across the vmalloc file keeping it in one solid place:
>
> <snip>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index d77830ff604c..f4b7927e217e 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2889,8 +2889,14 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> unsigned long array_size;
> unsigned int nr_small_pages = size >> PAGE_SHIFT;
> unsigned int page_order;
> + unsigned long flags;
> + int ret;
>
> array_size = (unsigned long)nr_small_pages * sizeof(struct page *);
> +
> + /*
> + * This is i do not understand why we do not want to see warning messages.
> + */
> gfp_mask |= __GFP_NOWARN;
I suspect this is becauser vmalloc wants to have its own failure
reporting.
[...]
> @@ -3010,16 +3037,22 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> area = __get_vm_area_node(real_size, align, shift, VM_ALLOC |
> VM_UNINITIALIZED | vm_flags, start, end, node,
> gfp_mask, caller);
> - if (!area) {
> - warn_alloc(gfp_mask, NULL,
> - "vmalloc error: size %lu, vm_struct allocation failed",
> - real_size);
> - goto fail;
> - }
> + if (area)
> + addr = __vmalloc_area_node(area, gfp_mask, prot, shift, node);
> +
> + if (!area || !addr) {
> + if (gfp_mask & __GFP_NOFAIL) {
> + schedule_timeout_uninterruptible(1);
> + goto again;
> + }
> +
> + if (!area)
> + warn_alloc(gfp_mask, NULL,
> + "vmalloc error: size %lu, vm_struct allocation failed",
> + real_size);
>
> - addr = __vmalloc_area_node(area, gfp_mask, prot, shift, node);
> - if (!addr)
> goto fail;
> + }
>
> /*
> * In this function, newly allocated vm_struct has VM_UNINITIALIZED
> <snip>
OK, this looks easier from the code reading but isn't it quite wasteful
to throw all the pages backing the area (all of them allocated as
__GFP_NOFAIL) just to then fail to allocate few page tables pages and
drop all of that on the floor (this will happen in __vunmap AFAICS).
I mean I do not care all that strongly but it seems to me that more
changes would need to be done here and optimizations can be done on top.
Is this something you feel strongly about?
--
Michal Hocko
SUSE Labs
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index d77830ff604c..f4b7927e217e 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -2889,8 +2889,14 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > unsigned long array_size;
> > unsigned int nr_small_pages = size >> PAGE_SHIFT;
> > unsigned int page_order;
> > + unsigned long flags;
> > + int ret;
> >
> > array_size = (unsigned long)nr_small_pages * sizeof(struct page *);
> > +
> > + /*
> > + * This is i do not understand why we do not want to see warning messages.
> > + */
> > gfp_mask |= __GFP_NOWARN;
>
> I suspect this is becauser vmalloc wants to have its own failure
> reporting.
>
But as i see it is broken. All three warn_alloc() reports in the
__vmalloc_area_node()
are useless because the __GFP_NOWARN is added on top of gfp_mask:
void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
{
struct va_format vaf;
va_list args;
static DEFINE_RATELIMIT_STATE(nopage_rs, 10*HZ, 1);
if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs))
return;
...
everything with the __GFP_NOWARN is just reverted.
> [...]
> > @@ -3010,16 +3037,22 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> > area = __get_vm_area_node(real_size, align, shift, VM_ALLOC |
> > VM_UNINITIALIZED | vm_flags, start, end, node,
> > gfp_mask, caller);
> > - if (!area) {
> > - warn_alloc(gfp_mask, NULL,
> > - "vmalloc error: size %lu, vm_struct allocation failed",
> > - real_size);
> > - goto fail;
> > - }
> > + if (area)
> > + addr = __vmalloc_area_node(area, gfp_mask, prot, shift, node);
> > +
> > + if (!area || !addr) {
> > + if (gfp_mask & __GFP_NOFAIL) {
> > + schedule_timeout_uninterruptible(1);
> > + goto again;
> > + }
> > +
> > + if (!area)
> > + warn_alloc(gfp_mask, NULL,
> > + "vmalloc error: size %lu, vm_struct allocation failed",
> > + real_size);
> >
> > - addr = __vmalloc_area_node(area, gfp_mask, prot, shift, node);
> > - if (!addr)
> > goto fail;
> > + }
> >
> > /*
> > * In this function, newly allocated vm_struct has VM_UNINITIALIZED
> > <snip>
>
> OK, this looks easier from the code reading but isn't it quite wasteful
> to throw all the pages backing the area (all of them allocated as
> __GFP_NOFAIL) just to then fail to allocate few page tables pages and
> drop all of that on the floor (this will happen in __vunmap AFAICS).
>
> I mean I do not care all that strongly but it seems to me that more
> changes would need to be done here and optimizations can be done on top.
>
> Is this something you feel strongly about?
>
Will try to provide some motivations :)
It depends on how to look at it. My view is as follows a more simple code
is preferred. It is not considered as a hot path and it is rather a corner
case to me. I think "unwinding" has some advantage. At least one motivation
is to release a memory(on failure) before a delay that will prevent holding
of extra memory in case of __GFP_NOFAIL infinitelly does not succeed, i.e.
if a process stuck due to __GFP_NOFAIL it does not "hold" an extra memory
forever.
--
Uladzislau Rezki
On Fri 29-10-21 16:05:32, Uladzislau Rezki wrote:
[...]
> > OK, this looks easier from the code reading but isn't it quite wasteful
> > to throw all the pages backing the area (all of them allocated as
> > __GFP_NOFAIL) just to then fail to allocate few page tables pages and
> > drop all of that on the floor (this will happen in __vunmap AFAICS).
> >
> > I mean I do not care all that strongly but it seems to me that more
> > changes would need to be done here and optimizations can be done on top.
> >
> > Is this something you feel strongly about?
> >
> Will try to provide some motivations :)
>
> It depends on how to look at it. My view is as follows a more simple code
> is preferred. It is not considered as a hot path and it is rather a corner
> case to me.
Yes, we are definitely talking about corner cases here. Even GFP_KERNEL
allocations usually do not fail.
> I think "unwinding" has some advantage. At least one motivation
> is to release a memory(on failure) before a delay that will prevent holding
> of extra memory in case of __GFP_NOFAIL infinitelly does not succeed, i.e.
> if a process stuck due to __GFP_NOFAIL it does not "hold" an extra memory
> forever.
Well, I suspect this is something that we can disagree on and both of us
would be kinda right. I would see it as throwing baby out with the
bathwater. The vast majority of the memory will be in the area pages and
sacrificing that just to allocate few page tables or whatever that might
fail in that code path is just a lot of cycles wasted.
So unless you really feel strongly about this then I would stick with
this approach.
--
Michal Hocko
SUSE Labs
> On Fri 29-10-21 16:05:32, Uladzislau Rezki wrote:
> [...]
> > > OK, this looks easier from the code reading but isn't it quite wasteful
> > > to throw all the pages backing the area (all of them allocated as
> > > __GFP_NOFAIL) just to then fail to allocate few page tables pages and
> > > drop all of that on the floor (this will happen in __vunmap AFAICS).
> > >
> > > I mean I do not care all that strongly but it seems to me that more
> > > changes would need to be done here and optimizations can be done on top.
> > >
> > > Is this something you feel strongly about?
> > >
> > Will try to provide some motivations :)
> >
> > It depends on how to look at it. My view is as follows a more simple code
> > is preferred. It is not considered as a hot path and it is rather a corner
> > case to me.
>
> Yes, we are definitely talking about corner cases here. Even GFP_KERNEL
> allocations usually do not fail.
>
> > I think "unwinding" has some advantage. At least one motivation
> > is to release a memory(on failure) before a delay that will prevent holding
> > of extra memory in case of __GFP_NOFAIL infinitelly does not succeed, i.e.
> > if a process stuck due to __GFP_NOFAIL it does not "hold" an extra memory
> > forever.
>
> Well, I suspect this is something that we can disagree on and both of us
> would be kinda right. I would see it as throwing baby out with the
> bathwater. The vast majority of the memory will be in the area pages and
> sacrificing that just to allocate few page tables or whatever that might
> fail in that code path is just a lot of cycles wasted.
>
We are not talking about performance, no sense to measure cycles here :)
>
> So unless you really feel strongly about this then I would stick with
> this approach.
>
I have raised one concern. The memory resource is shared between all
process in case of __GFP_NOFAIL it might be that we never return back
to user in that scenario i prefer to release hold memory for other
needs instead of keeping it for nothing.
If you think it is not a problem, then i do not have much to say.
--
Vlad Rezki