Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720958 > unrolled thread

[PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for optimistic try

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for optimistic try js1304@gmail.com - 2017-08-28 03:20 +0200
    Re: [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for  optimistic try Vlastimil Babka <vbabka@suse.cz> - 2017-08-28 13:30 +0200
      Re: [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for  optimistic try Michal Hocko <mhocko@kernel.org> - 2017-08-28 15:10 +0200
        Re: [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for  optimistic try Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2017-08-29 02:40 +0200
          Re: [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for  optimistic try Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2017-08-31 03:50 +0200
            Re: [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for  optimistic try Michal Hocko <mhocko@kernel.org> - 2017-08-31 07:30 +0200

#1720958 — [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for optimistic try

From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

High-order atomic allocation is difficult to succeed since we cannot
reclaim anything in this context. So, we reserves the pageblock for
this kind of request.

In slub, we try to allocate higher-order page more than it actually
needs in order to get the best performance. If this optimistic try is
used with GFP_ATOMIC, alloc_flags will be set as ALLOC_HARDER and
the pageblock reserved for high-order atomic allocation would be used.
Moreover, this request would reserve the MIGRATE_HIGHATOMIC pageblock
,if succeed, to prepare further request. It would not be good to use
MIGRATE_HIGHATOMIC pageblock in terms of fragmentation management
since it unconditionally set a migratetype to request's migratetype
when unreserving the pageblock without considering the migratetype of
used pages in the pageblock.

This is not what we don't intend so fix it by unconditionally setting
__GFP_NOMEMALLOC in order to not set ALLOC_HARDER.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 mm/slub.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index e1e442c..fd8dd89 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1579,10 +1579,8 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 	 */
 	alloc_gfp = (flags | __GFP_NOWARN | __GFP_NORETRY) & ~__GFP_NOFAIL;
 	if (oo_order(oo) > oo_order(s->min)) {
-		if (alloc_gfp & __GFP_DIRECT_RECLAIM) {
-			alloc_gfp |= __GFP_NOMEMALLOC;
-			alloc_gfp &= ~__GFP_DIRECT_RECLAIM;
-		}
+		alloc_gfp |= __GFP_NOMEMALLOC;
+		alloc_gfp &= ~__GFP_DIRECT_RECLAIM;
 	}
 
 	page = alloc_slab_page(s, alloc_gfp, node, oo);
-- 
2.7.4

[toc] | [next] | [standalone]

#1721533 — Re: [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for optimistic try

On 08/28/2017 03:11 AM, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> High-order atomic allocation is difficult to succeed since we cannot
> reclaim anything in this context. So, we reserves the pageblock for
> this kind of request.
> 
> In slub, we try to allocate higher-order page more than it actually
> needs in order to get the best performance. If this optimistic try is
> used with GFP_ATOMIC, alloc_flags will be set as ALLOC_HARDER and
> the pageblock reserved for high-order atomic allocation would be used.
> Moreover, this request would reserve the MIGRATE_HIGHATOMIC pageblock
> ,if succeed, to prepare further request. It would not be good to use
> MIGRATE_HIGHATOMIC pageblock in terms of fragmentation management
> since it unconditionally set a migratetype to request's migratetype
> when unreserving the pageblock without considering the migratetype of
> used pages in the pageblock.
> 
> This is not what we don't intend so fix it by unconditionally setting
> __GFP_NOMEMALLOC in order to not set ALLOC_HARDER.

I wonder if it would be more robust to strip GFP_ATOMIC from alloc_gfp.
E.g. __GFP_NOMEMALLOC does seem to prevent ALLOC_HARDER, but not
ALLOC_HIGH. Or maybe we should adjust __GFP_NOMEMALLOC implementation
and document it more thoroughly? CC Michal Hocko

Also, were these 2 patches done via code inspection or you noticed
suboptimal behavior which got fixed? Thanks.

> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
>  mm/slub.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index e1e442c..fd8dd89 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1579,10 +1579,8 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
>  	 */
>  	alloc_gfp = (flags | __GFP_NOWARN | __GFP_NORETRY) & ~__GFP_NOFAIL;
>  	if (oo_order(oo) > oo_order(s->min)) {
> -		if (alloc_gfp & __GFP_DIRECT_RECLAIM) {
> -			alloc_gfp |= __GFP_NOMEMALLOC;
> -			alloc_gfp &= ~__GFP_DIRECT_RECLAIM;
> -		}
> +		alloc_gfp |= __GFP_NOMEMALLOC;
> +		alloc_gfp &= ~__GFP_DIRECT_RECLAIM;
>  	}
>  
>  	page = alloc_slab_page(s, alloc_gfp, node, oo);
> 

[toc] | [prev] | [next] | [standalone]

#1721608 — Re: [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for optimistic try

On Mon 28-08-17 13:29:29, Vlastimil Babka wrote:
> On 08/28/2017 03:11 AM, js1304@gmail.com wrote:
> > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > 
> > High-order atomic allocation is difficult to succeed since we cannot
> > reclaim anything in this context. So, we reserves the pageblock for
> > this kind of request.
> > 
> > In slub, we try to allocate higher-order page more than it actually
> > needs in order to get the best performance. If this optimistic try is
> > used with GFP_ATOMIC, alloc_flags will be set as ALLOC_HARDER and
> > the pageblock reserved for high-order atomic allocation would be used.
> > Moreover, this request would reserve the MIGRATE_HIGHATOMIC pageblock
> > ,if succeed, to prepare further request. It would not be good to use
> > MIGRATE_HIGHATOMIC pageblock in terms of fragmentation management
> > since it unconditionally set a migratetype to request's migratetype
> > when unreserving the pageblock without considering the migratetype of
> > used pages in the pageblock.
> > 
> > This is not what we don't intend so fix it by unconditionally setting
> > __GFP_NOMEMALLOC in order to not set ALLOC_HARDER.
> 
> I wonder if it would be more robust to strip GFP_ATOMIC from alloc_gfp.
> E.g. __GFP_NOMEMALLOC does seem to prevent ALLOC_HARDER, but not
> ALLOC_HIGH. Or maybe we should adjust __GFP_NOMEMALLOC implementation
> and document it more thoroughly? CC Michal Hocko

Yeah, __GFP_NOMEMALLOC is rather inconsistent. It has been added to
override __GFP_MEMALLOC resp. PF_MEMALLOC AFAIK. In this particular
case I would agree that dropping __GFP_HIGH and __GFP_ATOMIC would
be more precise. I am not sure we want to touch the existing semantic of
__GFP_NOMEMALLOC though. This would require auditing all the existing
users (something tells me that quite some of those will be incorrect...)

> Also, were these 2 patches done via code inspection or you noticed
> suboptimal behavior which got fixed? Thanks.

The patch description is not very clear to me either but I guess that
Joonsoo sees to many larger order pages to back slab objects when the
system is not under heavy memory pressure and that increases internal
fragmentation?

> > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > ---
> >  mm/slub.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mm/slub.c b/mm/slub.c
> > index e1e442c..fd8dd89 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -1579,10 +1579,8 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
> >  	 */
> >  	alloc_gfp = (flags | __GFP_NOWARN | __GFP_NORETRY) & ~__GFP_NOFAIL;
> >  	if (oo_order(oo) > oo_order(s->min)) {
> > -		if (alloc_gfp & __GFP_DIRECT_RECLAIM) {
> > -			alloc_gfp |= __GFP_NOMEMALLOC;
> > -			alloc_gfp &= ~__GFP_DIRECT_RECLAIM;
> > -		}
> > +		alloc_gfp |= __GFP_NOMEMALLOC;
> > +		alloc_gfp &= ~__GFP_DIRECT_RECLAIM;
> >  	}
> >  
> >  	page = alloc_slab_page(s, alloc_gfp, node, oo);
> > 

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1722081 — Re: [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for optimistic try

On Mon, Aug 28, 2017 at 03:08:29PM +0200, Michal Hocko wrote:
> On Mon 28-08-17 13:29:29, Vlastimil Babka wrote:
> > On 08/28/2017 03:11 AM, js1304@gmail.com wrote:
> > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > > 
> > > High-order atomic allocation is difficult to succeed since we cannot
> > > reclaim anything in this context. So, we reserves the pageblock for
> > > this kind of request.
> > > 
> > > In slub, we try to allocate higher-order page more than it actually
> > > needs in order to get the best performance. If this optimistic try is
> > > used with GFP_ATOMIC, alloc_flags will be set as ALLOC_HARDER and
> > > the pageblock reserved for high-order atomic allocation would be used.
> > > Moreover, this request would reserve the MIGRATE_HIGHATOMIC pageblock
> > > ,if succeed, to prepare further request. It would not be good to use
> > > MIGRATE_HIGHATOMIC pageblock in terms of fragmentation management
> > > since it unconditionally set a migratetype to request's migratetype
> > > when unreserving the pageblock without considering the migratetype of
> > > used pages in the pageblock.
> > > 
> > > This is not what we don't intend so fix it by unconditionally setting
> > > __GFP_NOMEMALLOC in order to not set ALLOC_HARDER.
> > 
> > I wonder if it would be more robust to strip GFP_ATOMIC from alloc_gfp.
> > E.g. __GFP_NOMEMALLOC does seem to prevent ALLOC_HARDER, but not
> > ALLOC_HIGH. Or maybe we should adjust __GFP_NOMEMALLOC implementation
> > and document it more thoroughly? CC Michal Hocko
> 
> Yeah, __GFP_NOMEMALLOC is rather inconsistent. It has been added to
> override __GFP_MEMALLOC resp. PF_MEMALLOC AFAIK. In this particular
> case I would agree that dropping __GFP_HIGH and __GFP_ATOMIC would
> be more precise. I am not sure we want to touch the existing semantic of
> __GFP_NOMEMALLOC though. This would require auditing all the existing
> users (something tells me that quite some of those will be incorrect...)

Hmm... now I realize that there is another reason that we need to use
__GFP_NOMEMALLOC. Even if this allocation comes from PF_MEMALLOC user,
this optimistic try should not use the reserved memory below the
watermark. That is, it should not use ALLOC_NO_WATERMARKS. It can
only be accomplished by using __GFP_NOMEMALLOC.

> 
> > Also, were these 2 patches done via code inspection or you noticed
> > suboptimal behavior which got fixed? Thanks.
> 
> The patch description is not very clear to me either but I guess that
> Joonsoo sees to many larger order pages to back slab objects when the
> system is not under heavy memory pressure and that increases internal
> fragmentation?

Your guess is right. I found this problem when I checked the
fragmentation ratio through the benchmark some months ago. I don't
remember detailed system state in that benchmark.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1723765 — Re: [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for optimistic try

On Tue, Aug 29, 2017 at 09:33:44AM +0900, Joonsoo Kim wrote:
> On Mon, Aug 28, 2017 at 03:08:29PM +0200, Michal Hocko wrote:
> > On Mon 28-08-17 13:29:29, Vlastimil Babka wrote:
> > > On 08/28/2017 03:11 AM, js1304@gmail.com wrote:
> > > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > > > 
> > > > High-order atomic allocation is difficult to succeed since we cannot
> > > > reclaim anything in this context. So, we reserves the pageblock for
> > > > this kind of request.
> > > > 
> > > > In slub, we try to allocate higher-order page more than it actually
> > > > needs in order to get the best performance. If this optimistic try is
> > > > used with GFP_ATOMIC, alloc_flags will be set as ALLOC_HARDER and
> > > > the pageblock reserved for high-order atomic allocation would be used.
> > > > Moreover, this request would reserve the MIGRATE_HIGHATOMIC pageblock
> > > > ,if succeed, to prepare further request. It would not be good to use
> > > > MIGRATE_HIGHATOMIC pageblock in terms of fragmentation management
> > > > since it unconditionally set a migratetype to request's migratetype
> > > > when unreserving the pageblock without considering the migratetype of
> > > > used pages in the pageblock.
> > > > 
> > > > This is not what we don't intend so fix it by unconditionally setting
> > > > __GFP_NOMEMALLOC in order to not set ALLOC_HARDER.
> > > 
> > > I wonder if it would be more robust to strip GFP_ATOMIC from alloc_gfp.
> > > E.g. __GFP_NOMEMALLOC does seem to prevent ALLOC_HARDER, but not
> > > ALLOC_HIGH. Or maybe we should adjust __GFP_NOMEMALLOC implementation
> > > and document it more thoroughly? CC Michal Hocko
> > 
> > Yeah, __GFP_NOMEMALLOC is rather inconsistent. It has been added to
> > override __GFP_MEMALLOC resp. PF_MEMALLOC AFAIK. In this particular
> > case I would agree that dropping __GFP_HIGH and __GFP_ATOMIC would
> > be more precise. I am not sure we want to touch the existing semantic of
> > __GFP_NOMEMALLOC though. This would require auditing all the existing
> > users (something tells me that quite some of those will be incorrect...)
> 
> Hmm... now I realize that there is another reason that we need to use
> __GFP_NOMEMALLOC. Even if this allocation comes from PF_MEMALLOC user,
> this optimistic try should not use the reserved memory below the
> watermark. That is, it should not use ALLOC_NO_WATERMARKS. It can
> only be accomplished by using __GFP_NOMEMALLOC.

Michal, Vlastimil, Any thought?

Thanks.

[toc] | [prev] | [next] | [standalone]

#1723842 — Re: [PATCH 2/2] mm/slub: don't use reserved highatomic pageblock for optimistic try

On Thu 31-08-17 10:42:41, Joonsoo Kim wrote:
> On Tue, Aug 29, 2017 at 09:33:44AM +0900, Joonsoo Kim wrote:
> > On Mon, Aug 28, 2017 at 03:08:29PM +0200, Michal Hocko wrote:
> > > On Mon 28-08-17 13:29:29, Vlastimil Babka wrote:
> > > > On 08/28/2017 03:11 AM, js1304@gmail.com wrote:
> > > > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > > > > 
> > > > > High-order atomic allocation is difficult to succeed since we cannot
> > > > > reclaim anything in this context. So, we reserves the pageblock for
> > > > > this kind of request.
> > > > > 
> > > > > In slub, we try to allocate higher-order page more than it actually
> > > > > needs in order to get the best performance. If this optimistic try is
> > > > > used with GFP_ATOMIC, alloc_flags will be set as ALLOC_HARDER and
> > > > > the pageblock reserved for high-order atomic allocation would be used.
> > > > > Moreover, this request would reserve the MIGRATE_HIGHATOMIC pageblock
> > > > > ,if succeed, to prepare further request. It would not be good to use
> > > > > MIGRATE_HIGHATOMIC pageblock in terms of fragmentation management
> > > > > since it unconditionally set a migratetype to request's migratetype
> > > > > when unreserving the pageblock without considering the migratetype of
> > > > > used pages in the pageblock.
> > > > > 
> > > > > This is not what we don't intend so fix it by unconditionally setting
> > > > > __GFP_NOMEMALLOC in order to not set ALLOC_HARDER.
> > > > 
> > > > I wonder if it would be more robust to strip GFP_ATOMIC from alloc_gfp.
> > > > E.g. __GFP_NOMEMALLOC does seem to prevent ALLOC_HARDER, but not
> > > > ALLOC_HIGH. Or maybe we should adjust __GFP_NOMEMALLOC implementation
> > > > and document it more thoroughly? CC Michal Hocko
> > > 
> > > Yeah, __GFP_NOMEMALLOC is rather inconsistent. It has been added to
> > > override __GFP_MEMALLOC resp. PF_MEMALLOC AFAIK. In this particular
> > > case I would agree that dropping __GFP_HIGH and __GFP_ATOMIC would
> > > be more precise. I am not sure we want to touch the existing semantic of
> > > __GFP_NOMEMALLOC though. This would require auditing all the existing
> > > users (something tells me that quite some of those will be incorrect...)
> > 
> > Hmm... now I realize that there is another reason that we need to use
> > __GFP_NOMEMALLOC. Even if this allocation comes from PF_MEMALLOC user,
> > this optimistic try should not use the reserved memory below the
> > watermark. That is, it should not use ALLOC_NO_WATERMARKS. It can
> > only be accomplished by using __GFP_NOMEMALLOC.
> 
> Michal, Vlastimil, Any thought?

Hmm, I would go with a helper like below and use it in slub

gfp_t gfp_drop_reserves(gfp_t mask)
{
	mask &= ~(__GFP_HIGH|__GFP_ATOMIC)
	mask |= __GFP_NOMEMALLOC;

	return mask;
}
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web