Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Christopher Lameter]

On Sat, 21 Apr 2018, Vlastimil Babka wrote:

> > The problem is that SLUB does not honor GFP_NORETRY. The semantics of
> > GFP_NORETRY are not followed.
>
> The caller might want SLUB to try hard to get that high-order page that
> will minimize memory waste (e.g. 2MB page for 3 640k objects), and
> __GFP_NORETRY will kill the effort on allocating that high-order page.

Well yes since *_NORETRY says that fallbacks are acceptable.

> Thus, using __GPF_NORETRY for "please give me a space-optimized object,
> or nothing (because I have a fallback that's better than wasting memory,
> e.g. by using 1MB page for 640kb object)" is not ideal.
>
> Maybe __GFP_RETRY_MAYFAIL is a better fit? Or perhaps indicate this
> situation to SLUB with e.g. __GFP_COMP, although that's rather ugly?

Yuck. None of that sounds like an intuitive approach.

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Christopher Lameter]

On Sat, 21 Apr 2018, Vlastimil Babka wrote:

> > The problem is that SLUB does not honor GFP_NORETRY. The semantics of
> > GFP_NORETRY are not followed.
>
> The caller might want SLUB to try hard to get that high-order page that
> will minimize memory waste (e.g. 2MB page for 3 640k objects), and
> __GFP_NORETRY will kill the effort on allocating that high-order page.

Well yes since *_NORETRY says that fallbacks are acceptable.

> Thus, using __GPF_NORETRY for "please give me a space-optimized object,
> or nothing (because I have a fallback that's better than wasting memory,
> e.g. by using 1MB page for 640kb object)" is not ideal.
>
> Maybe __GFP_RETRY_MAYFAIL is a better fit? Or perhaps indicate this
> situation to SLUB with e.g. __GFP_COMP, although that's rather ugly?

Yuck. None of that sounds like an intuitive approach.

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Vlastimil Babka]

On 04/20/2018 04:53 PM, Christopher Lameter wrote:
> On Thu, 19 Apr 2018, Michal Hocko wrote:
> 
>> Overriding __GFP_NORETRY is just a bad idea. It will make the semantic
>> of the flag just more confusing. Note there are users who use
>> __GFP_NORETRY as a way to suppress heavy memory pressure and/or the OOM
>> killer. You do not want to change the semantic for them.
> 
> Redoing the allocation after failing a large order alloc is a retry. I
> would say its confusing right now because a retry occurs despite
> specifying GFP_NORETRY,
> 
>> Besides that the changelog is less than optimal. What is the actual
>> problem? Why somebody doesn't want a fallback? Is there a configuration
>> that could prevent the same?
> 
> The problem is that SLUB does not honor GFP_NORETRY. The semantics of
> GFP_NORETRY are not followed.

The caller might want SLUB to try hard to get that high-order page that
will minimize memory waste (e.g. 2MB page for 3 640k objects), and
__GFP_NORETRY will kill the effort on allocating that high-order page.

Thus, using __GPF_NORETRY for "please give me a space-optimized object,
or nothing (because I have a fallback that's better than wasting memory,
e.g. by using 1MB page for 640kb object)" is not ideal.

Maybe __GFP_RETRY_MAYFAIL is a better fit? Or perhaps indicate this
situation to SLUB with e.g. __GFP_COMP, although that's rather ugly?

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Vlastimil Babka]

On 04/20/2018 04:53 PM, Christopher Lameter wrote:
> On Thu, 19 Apr 2018, Michal Hocko wrote:
> 
>> Overriding __GFP_NORETRY is just a bad idea. It will make the semantic
>> of the flag just more confusing. Note there are users who use
>> __GFP_NORETRY as a way to suppress heavy memory pressure and/or the OOM
>> killer. You do not want to change the semantic for them.
> 
> Redoing the allocation after failing a large order alloc is a retry. I
> would say its confusing right now because a retry occurs despite
> specifying GFP_NORETRY,
> 
>> Besides that the changelog is less than optimal. What is the actual
>> problem? Why somebody doesn't want a fallback? Is there a configuration
>> that could prevent the same?
> 
> The problem is that SLUB does not honor GFP_NORETRY. The semantics of
> GFP_NORETRY are not followed.

The caller might want SLUB to try hard to get that high-order page that
will minimize memory waste (e.g. 2MB page for 3 640k objects), and
__GFP_NORETRY will kill the effort on allocating that high-order page.

Thus, using __GPF_NORETRY for "please give me a space-optimized object,
or nothing (because I have a fallback that's better than wasting memory,
e.g. by using 1MB page for 640kb object)" is not ideal.

Maybe __GFP_RETRY_MAYFAIL is a better fit? Or perhaps indicate this
situation to SLUB with e.g. __GFP_COMP, although that's rather ugly?

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Christopher Lameter]

On Thu, 19 Apr 2018, Michal Hocko wrote:

> Overriding __GFP_NORETRY is just a bad idea. It will make the semantic
> of the flag just more confusing. Note there are users who use
> __GFP_NORETRY as a way to suppress heavy memory pressure and/or the OOM
> killer. You do not want to change the semantic for them.

Redoing the allocation after failing a large order alloc is a retry. I
would say its confusing right now because a retry occurs despite
specifying GFP_NORETRY,

> Besides that the changelog is less than optimal. What is the actual
> problem? Why somebody doesn't want a fallback? Is there a configuration
> that could prevent the same?

The problem is that SLUB does not honor GFP_NORETRY. The semantics of
GFP_NORETRY are not followed.

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Christopher Lameter]

On Thu, 19 Apr 2018, Michal Hocko wrote:

> Overriding __GFP_NORETRY is just a bad idea. It will make the semantic
> of the flag just more confusing. Note there are users who use
> __GFP_NORETRY as a way to suppress heavy memory pressure and/or the OOM
> killer. You do not want to change the semantic for them.

Redoing the allocation after failing a large order alloc is a retry. I
would say its confusing right now because a retry occurs despite
specifying GFP_NORETRY,

> Besides that the changelog is less than optimal. What is the actual
> problem? Why somebody doesn't want a fallback? Is there a configuration
> that could prevent the same?

The problem is that SLUB does not honor GFP_NORETRY. The semantics of
GFP_NORETRY are not followed.

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Michal Hocko]

On Wed 18-04-18 09:45:39, Cristopher Lameter wrote:
> Mikulas Patoka wants to ensure that no fallback to lower order happens. I
> think __GFP_NORETRY should work correctly in that case too and not fall
> back.

Overriding __GFP_NORETRY is just a bad idea. It will make the semantic
of the flag just more confusing. Note there are users who use
__GFP_NORETRY as a way to suppress heavy memory pressure and/or the OOM
killer. You do not want to change the semantic for them.

Besides that the changelog is less than optimal. What is the actual
problem? Why somebody doesn't want a fallback? Is there a configuration
that could prevent the same?

> Allocating at a smaller order is a retry operation and should not
> be attempted.
> 
> If the caller does not want retries then respect that.
> 
> GFP_NORETRY allows callers to ensure that only maximum order
> allocations are attempted.
> 
> Signed-off-by: Christoph Lameter 
> 
> Index: linux/mm/slub.c
> ===
> --- linux.orig/mm/slub.c
> +++ linux/mm/slub.c
> @@ -1598,7 +1598,7 @@ static struct page *allocate_slab(struct
>   alloc_gfp = (alloc_gfp | __GFP_NOMEMALLOC) & 
> ~(__GFP_RECLAIM|__GFP_NOFAIL);
> 
>   page = alloc_slab_page(s, alloc_gfp, node, oo);
> - if (unlikely(!page)) {
> + if (unlikely(!page) && !(flags & __GFP_NORETRY)) {
>   oo = s->min;
>   alloc_gfp = flags;
>   /*

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Michal Hocko]

On Wed 18-04-18 09:45:39, Cristopher Lameter wrote:
> Mikulas Patoka wants to ensure that no fallback to lower order happens. I
> think __GFP_NORETRY should work correctly in that case too and not fall
> back.

Overriding __GFP_NORETRY is just a bad idea. It will make the semantic
of the flag just more confusing. Note there are users who use
__GFP_NORETRY as a way to suppress heavy memory pressure and/or the OOM
killer. You do not want to change the semantic for them.

Besides that the changelog is less than optimal. What is the actual
problem? Why somebody doesn't want a fallback? Is there a configuration
that could prevent the same?

> Allocating at a smaller order is a retry operation and should not
> be attempted.
> 
> If the caller does not want retries then respect that.
> 
> GFP_NORETRY allows callers to ensure that only maximum order
> allocations are attempted.
> 
> Signed-off-by: Christoph Lameter 
> 
> Index: linux/mm/slub.c
> ===
> --- linux.orig/mm/slub.c
> +++ linux/mm/slub.c
> @@ -1598,7 +1598,7 @@ static struct page *allocate_slab(struct
>   alloc_gfp = (alloc_gfp | __GFP_NOMEMALLOC) & 
> ~(__GFP_RECLAIM|__GFP_NOFAIL);
> 
>   page = alloc_slab_page(s, alloc_gfp, node, oo);
> - if (unlikely(!page)) {
> + if (unlikely(!page) && !(flags & __GFP_NORETRY)) {
>   oo = s->min;
>   alloc_gfp = flags;
>   /*

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[David Rientjes]

On Wed, 18 Apr 2018, Mikulas Patocka wrote:

> > Mikulas Patoka wants to ensure that no fallback to lower order happens. I
> > think __GFP_NORETRY should work correctly in that case too and not fall
> > back.
> > 
> > 
> > 
> > Allocating at a smaller order is a retry operation and should not
> > be attempted.
> > 
> > If the caller does not want retries then respect that.
> > 
> > GFP_NORETRY allows callers to ensure that only maximum order
> > allocations are attempted.
> > 
> > Signed-off-by: Christoph Lameter 
> > 
> > Index: linux/mm/slub.c
> > ===
> > --- linux.orig/mm/slub.c
> > +++ linux/mm/slub.c
> > @@ -1598,7 +1598,7 @@ static struct page *allocate_slab(struct
> > alloc_gfp = (alloc_gfp | __GFP_NOMEMALLOC) & 
> > ~(__GFP_RECLAIM|__GFP_NOFAIL);
> > 
> > page = alloc_slab_page(s, alloc_gfp, node, oo);
> > -   if (unlikely(!page)) {
> > +   if (unlikely(!page) && !(flags & __GFP_NORETRY)) {
> > oo = s->min;
> > alloc_gfp = flags;
> > /*
> 
> No, this would hit NULL pointer dereference if page is NULL and 
> __GFP_NORETRY is set. You want this:
> 
> ---
>  mm/slub.c |2 ++
>  1 file changed, 2 insertions(+)
> 
> Index: linux-2.6/mm/slub.c
> ===
> --- linux-2.6.orig/mm/slub.c  2018-04-17 20:58:23.0 +0200
> +++ linux-2.6/mm/slub.c   2018-04-18 17:04:01.0 +0200
> @@ -1599,6 +1599,8 @@ static struct page *allocate_slab(struct
>  
>   page = alloc_slab_page(s, alloc_gfp, node, oo);
>   if (unlikely(!page)) {
> + if (flags & __GFP_NORETRY)
> + goto out;
>   oo = s->min;
>   alloc_gfp = flags;
>   /*
> 

I don't see the connection between the max order, which can be influenced 
by userspace with slub_min_objects, slub_min_order, etc, and specifying 
__GFP_NORETRY which means try to reclaim and free memory but don't loop.

If I force a slab cache to try a max order of 9 for hugepages as a best 
effort, why does __GFP_NORETRY suddenly mean I won't fallback to 
oo_order(s->min)?

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[David Rientjes]

On Wed, 18 Apr 2018, Mikulas Patocka wrote:

> > Mikulas Patoka wants to ensure that no fallback to lower order happens. I
> > think __GFP_NORETRY should work correctly in that case too and not fall
> > back.
> > 
> > 
> > 
> > Allocating at a smaller order is a retry operation and should not
> > be attempted.
> > 
> > If the caller does not want retries then respect that.
> > 
> > GFP_NORETRY allows callers to ensure that only maximum order
> > allocations are attempted.
> > 
> > Signed-off-by: Christoph Lameter 
> > 
> > Index: linux/mm/slub.c
> > ===
> > --- linux.orig/mm/slub.c
> > +++ linux/mm/slub.c
> > @@ -1598,7 +1598,7 @@ static struct page *allocate_slab(struct
> > alloc_gfp = (alloc_gfp | __GFP_NOMEMALLOC) & 
> > ~(__GFP_RECLAIM|__GFP_NOFAIL);
> > 
> > page = alloc_slab_page(s, alloc_gfp, node, oo);
> > -   if (unlikely(!page)) {
> > +   if (unlikely(!page) && !(flags & __GFP_NORETRY)) {
> > oo = s->min;
> > alloc_gfp = flags;
> > /*
> 
> No, this would hit NULL pointer dereference if page is NULL and 
> __GFP_NORETRY is set. You want this:
> 
> ---
>  mm/slub.c |2 ++
>  1 file changed, 2 insertions(+)
> 
> Index: linux-2.6/mm/slub.c
> ===
> --- linux-2.6.orig/mm/slub.c  2018-04-17 20:58:23.0 +0200
> +++ linux-2.6/mm/slub.c   2018-04-18 17:04:01.0 +0200
> @@ -1599,6 +1599,8 @@ static struct page *allocate_slab(struct
>  
>   page = alloc_slab_page(s, alloc_gfp, node, oo);
>   if (unlikely(!page)) {
> + if (flags & __GFP_NORETRY)
> + goto out;
>   oo = s->min;
>   alloc_gfp = flags;
>   /*
> 

I don't see the connection between the max order, which can be influenced 
by userspace with slub_min_objects, slub_min_order, etc, and specifying 
__GFP_NORETRY which means try to reclaim and free memory but don't loop.

If I force a slab cache to try a max order of 9 for hugepages as a best 
effort, why does __GFP_NORETRY suddenly mean I won't fallback to 
oo_order(s->min)?

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Christopher Lameter]

On Wed, 18 Apr 2018, Mikulas Patocka wrote:

> No, this would hit NULL pointer dereference if page is NULL and
> __GFP_NORETRY is set. You want this:

You are right

Acked-by: Christoph Lameter 

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Christopher Lameter]

On Wed, 18 Apr 2018, Mikulas Patocka wrote:

> No, this would hit NULL pointer dereference if page is NULL and
> __GFP_NORETRY is set. You want this:

You are right

Acked-by: Christoph Lameter 

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Mikulas Patocka]

On Wed, 18 Apr 2018, Christopher Lameter wrote:

> Mikulas Patoka wants to ensure that no fallback to lower order happens. I
> think __GFP_NORETRY should work correctly in that case too and not fall
> back.
> 
> 
> 
> Allocating at a smaller order is a retry operation and should not
> be attempted.
> 
> If the caller does not want retries then respect that.
> 
> GFP_NORETRY allows callers to ensure that only maximum order
> allocations are attempted.
> 
> Signed-off-by: Christoph Lameter 
> 
> Index: linux/mm/slub.c
> ===
> --- linux.orig/mm/slub.c
> +++ linux/mm/slub.c
> @@ -1598,7 +1598,7 @@ static struct page *allocate_slab(struct
>   alloc_gfp = (alloc_gfp | __GFP_NOMEMALLOC) & 
> ~(__GFP_RECLAIM|__GFP_NOFAIL);
> 
>   page = alloc_slab_page(s, alloc_gfp, node, oo);
> - if (unlikely(!page)) {
> + if (unlikely(!page) && !(flags & __GFP_NORETRY)) {
>   oo = s->min;
>   alloc_gfp = flags;
>   /*

No, this would hit NULL pointer dereference if page is NULL and 
__GFP_NORETRY is set. You want this:

---
 mm/slub.c |2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6/mm/slub.c
===
--- linux-2.6.orig/mm/slub.c2018-04-17 20:58:23.0 +0200
+++ linux-2.6/mm/slub.c 2018-04-18 17:04:01.0 +0200
@@ -1599,6 +1599,8 @@ static struct page *allocate_slab(struct
 
page = alloc_slab_page(s, alloc_gfp, node, oo);
if (unlikely(!page)) {
+   if (flags & __GFP_NORETRY)
+   goto out;
oo = s->min;
alloc_gfp = flags;
/*

Re: [PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Mikulas Patocka]

On Wed, 18 Apr 2018, Christopher Lameter wrote:

> Mikulas Patoka wants to ensure that no fallback to lower order happens. I
> think __GFP_NORETRY should work correctly in that case too and not fall
> back.
> 
> 
> 
> Allocating at a smaller order is a retry operation and should not
> be attempted.
> 
> If the caller does not want retries then respect that.
> 
> GFP_NORETRY allows callers to ensure that only maximum order
> allocations are attempted.
> 
> Signed-off-by: Christoph Lameter 
> 
> Index: linux/mm/slub.c
> ===
> --- linux.orig/mm/slub.c
> +++ linux/mm/slub.c
> @@ -1598,7 +1598,7 @@ static struct page *allocate_slab(struct
>   alloc_gfp = (alloc_gfp | __GFP_NOMEMALLOC) & 
> ~(__GFP_RECLAIM|__GFP_NOFAIL);
> 
>   page = alloc_slab_page(s, alloc_gfp, node, oo);
> - if (unlikely(!page)) {
> + if (unlikely(!page) && !(flags & __GFP_NORETRY)) {
>   oo = s->min;
>   alloc_gfp = flags;
>   /*

No, this would hit NULL pointer dereference if page is NULL and 
__GFP_NORETRY is set. You want this:

---
 mm/slub.c |2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6/mm/slub.c
===
--- linux-2.6.orig/mm/slub.c2018-04-17 20:58:23.0 +0200
+++ linux-2.6/mm/slub.c 2018-04-18 17:04:01.0 +0200
@@ -1599,6 +1599,8 @@ static struct page *allocate_slab(struct
 
page = alloc_slab_page(s, alloc_gfp, node, oo);
if (unlikely(!page)) {
+   if (flags & __GFP_NORETRY)
+   goto out;
oo = s->min;
alloc_gfp = flags;
/*

[PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Christopher Lameter]

Mikulas Patoka wants to ensure that no fallback to lower order happens. I
think __GFP_NORETRY should work correctly in that case too and not fall
back.



Allocating at a smaller order is a retry operation and should not
be attempted.

If the caller does not want retries then respect that.

GFP_NORETRY allows callers to ensure that only maximum order
allocations are attempted.

Signed-off-by: Christoph Lameter 

Index: linux/mm/slub.c
===
--- linux.orig/mm/slub.c
+++ linux/mm/slub.c
@@ -1598,7 +1598,7 @@ static struct page *allocate_slab(struct
alloc_gfp = (alloc_gfp | __GFP_NOMEMALLOC) & 
~(__GFP_RECLAIM|__GFP_NOFAIL);

page = alloc_slab_page(s, alloc_gfp, node, oo);
-   if (unlikely(!page)) {
+   if (unlikely(!page) && !(flags & __GFP_NORETRY)) {
oo = s->min;
alloc_gfp = flags;
/*

[PATCH] SLUB: Do not fallback to mininum order if __GFP_NORETRY is set

[Christopher Lameter]

Mikulas Patoka wants to ensure that no fallback to lower order happens. I
think __GFP_NORETRY should work correctly in that case too and not fall
back.



Allocating at a smaller order is a retry operation and should not
be attempted.

If the caller does not want retries then respect that.

GFP_NORETRY allows callers to ensure that only maximum order
allocations are attempted.

Signed-off-by: Christoph Lameter 

Index: linux/mm/slub.c
===
--- linux.orig/mm/slub.c
+++ linux/mm/slub.c
@@ -1598,7 +1598,7 @@ static struct page *allocate_slab(struct
alloc_gfp = (alloc_gfp | __GFP_NOMEMALLOC) & 
~(__GFP_RECLAIM|__GFP_NOFAIL);

page = alloc_slab_page(s, alloc_gfp, node, oo);
-   if (unlikely(!page)) {
+   if (unlikely(!page) && !(flags & __GFP_NORETRY)) {
oo = s->min;
alloc_gfp = flags;
/*