diff mbox

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

Message ID alpine.DEB.2.20.1804180944180.1062@nuc-kabylake (mailing list archive)
State Not Applicable, archived
Delegated to: Mike Snitzer
Headers show

Commit Message

Christoph Lameter (Ampere) April 18, 2018, 2:45 p.m. UTC
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 <cl@linux.com>


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Comments

Michal Hocko April 19, 2018, 11 a.m. UTC | #1
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 <cl@linux.com>
> 
> 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;
>  		/*
Christoph Lameter (Ampere) April 20, 2018, 2:53 p.m. UTC | #2
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.


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Vlastimil Babka April 21, 2018, 5:02 p.m. UTC | #3
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?

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Christoph Lameter (Ampere) April 23, 2018, 10:41 p.m. UTC | #4
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.


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox

Patch

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;
 		/*