diff mbox

[External] Re: [PATCH 2/3] include/linux/gfp.h: use unsigned int in gfp_zone

Message ID 20180508002547.GA16338@bombadil.infradead.org
State New, archived
Headers show

Commit Message

Matthew Wilcox May 8, 2018, 12:25 a.m. UTC
On Mon, May 07, 2018 at 11:25:01PM +0200, David Sterba wrote:
> On Mon, May 07, 2018 at 11:44:10AM -0700, Matthew Wilcox wrote:
> > But something like btrfs should almost certainly be using ~GFP_ZONEMASK.
> 
> Agreed, the direct use of __GFP_DMA32 was added in 3ba7ab220e8918176c6f
> to substitute GFP_NOFS, so the allocation flags are less restrictive but
> still acceptable for allocation from slab.
> 
> The requirement from btrfs is to avoid highmem, the 'must be acceptable
> for slab' requirement is more MM internal and should have been hidden
> under some opaque flag mask. There was no strong need for that at the
> time.

The GFP flags encode a multiple of different requirements.  There's
"What can the allocator do to free memory" and "what area of memory
can the allocation come from".  btrfs doesn't actually want to
allocate memory from ZONE_MOVABLE or ZONE_DMA either.  It's probably never
been called with those particular flags set, but in the spirit of
future-proofing btrfs, perhaps a patch like this is in order?

---- >8 ----

Subject: btrfs: Allocate extents from ZONE_NORMAL
From: Matthew Wilcox <mawilcox@microsoft.com>

If anyone ever passes a GFP_DMA or GFP_MOVABLE allocation flag to
allocate_extent_state, it will try to allocate memory from the wrong zone.
We just want to allocate memory from ZONE_NORMAL, so use GFP_RECLAIM_MASK
to get what we want.

Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>

Comments

David Sterba May 9, 2018, 9:36 a.m. UTC | #1
On Mon, May 07, 2018 at 05:25:47PM -0700, Matthew Wilcox wrote:
> On Mon, May 07, 2018 at 11:25:01PM +0200, David Sterba wrote:
> > On Mon, May 07, 2018 at 11:44:10AM -0700, Matthew Wilcox wrote:
> > > But something like btrfs should almost certainly be using ~GFP_ZONEMASK.
> > 
> > Agreed, the direct use of __GFP_DMA32 was added in 3ba7ab220e8918176c6f
> > to substitute GFP_NOFS, so the allocation flags are less restrictive but
> > still acceptable for allocation from slab.
> > 
> > The requirement from btrfs is to avoid highmem, the 'must be acceptable
> > for slab' requirement is more MM internal and should have been hidden
> > under some opaque flag mask. There was no strong need for that at the
> > time.
> 
> The GFP flags encode a multiple of different requirements.  There's
> "What can the allocator do to free memory" and "what area of memory
> can the allocation come from".  btrfs doesn't actually want to
> allocate memory from ZONE_MOVABLE or ZONE_DMA either.  It's probably never
> been called with those particular flags set, but in the spirit of
> future-proofing btrfs, perhaps a patch like this is in order?
> 
> ---- >8 ----
> 
> Subject: btrfs: Allocate extents from ZONE_NORMAL
> From: Matthew Wilcox <mawilcox@microsoft.com>
> 
> If anyone ever passes a GFP_DMA or GFP_MOVABLE allocation flag to
> allocate_extent_state, it will try to allocate memory from the wrong zone.
> We just want to allocate memory from ZONE_NORMAL, so use GFP_RECLAIM_MASK
> to get what we want.

Looks good to me.

> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index e99b329002cf..4e4a67b7b29d 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -216,12 +216,7 @@ static struct extent_state *alloc_extent_state(gfp_t mask)
>  {
>  	struct extent_state *state;
>  
> -	/*
> -	 * The given mask might be not appropriate for the slab allocator,
> -	 * drop the unsupported bits
> -	 */
> -	mask &= ~(__GFP_DMA32|__GFP_HIGHMEM);

I've noticed there's GFP_SLAB_BUG_MASK that's basically open coded here,
but this would not filter out the placement flags.

> -	state = kmem_cache_alloc(extent_state_cache, mask);

I'd prefer some comment here, it's not obvious why the mask is used.

> +	state = kmem_cache_alloc(extent_state_cache, mask & GFP_RECLAIM_MASK);
>  	if (!state)
>  		return state;
>  	state->state = 0;
Matthew Wilcox May 15, 2018, 11:54 a.m. UTC | #2
On Wed, May 09, 2018 at 11:36:59AM +0200, David Sterba wrote:
> On Mon, May 07, 2018 at 05:25:47PM -0700, Matthew Wilcox wrote:
> > On Mon, May 07, 2018 at 11:25:01PM +0200, David Sterba wrote:
> > > On Mon, May 07, 2018 at 11:44:10AM -0700, Matthew Wilcox wrote:
> > > > But something like btrfs should almost certainly be using ~GFP_ZONEMASK.
> > > 
> > > Agreed, the direct use of __GFP_DMA32 was added in 3ba7ab220e8918176c6f
> > > to substitute GFP_NOFS, so the allocation flags are less restrictive but
> > > still acceptable for allocation from slab.
> > > 
> > > The requirement from btrfs is to avoid highmem, the 'must be acceptable
> > > for slab' requirement is more MM internal and should have been hidden
> > > under some opaque flag mask. There was no strong need for that at the
> > > time.
> > 
> > The GFP flags encode a multiple of different requirements.  There's
> > "What can the allocator do to free memory" and "what area of memory
> > can the allocation come from".  btrfs doesn't actually want to
> > allocate memory from ZONE_MOVABLE or ZONE_DMA either.  It's probably never
> > been called with those particular flags set, but in the spirit of
> > future-proofing btrfs, perhaps a patch like this is in order?
> > 
> > ---- >8 ----
> > 
> > Subject: btrfs: Allocate extents from ZONE_NORMAL
> > From: Matthew Wilcox <mawilcox@microsoft.com>
> > 
> > If anyone ever passes a GFP_DMA or GFP_MOVABLE allocation flag to
> > allocate_extent_state, it will try to allocate memory from the wrong zone.
> > We just want to allocate memory from ZONE_NORMAL, so use GFP_RECLAIM_MASK
> > to get what we want.
> 
> Looks good to me.
> 
> > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index e99b329002cf..4e4a67b7b29d 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -216,12 +216,7 @@ static struct extent_state *alloc_extent_state(gfp_t mask)
> >  {
> >  	struct extent_state *state;
> >  
> > -	/*
> > -	 * The given mask might be not appropriate for the slab allocator,
> > -	 * drop the unsupported bits
> > -	 */
> > -	mask &= ~(__GFP_DMA32|__GFP_HIGHMEM);
> 
> I've noticed there's GFP_SLAB_BUG_MASK that's basically open coded here,
> but this would not filter out the placement flags.
> 
> > -	state = kmem_cache_alloc(extent_state_cache, mask);
> 
> I'd prefer some comment here, it's not obvious why the mask is used.

Sorry, I dropped the ball on this.  Would you prefer:

        /* Allocate from ZONE_NORMAL */
        state = kmem_cache_alloc(extent_state_cache, mask & GFP_RECLAIM_MASK);

or

	/*
	 * Callers may pass in a mask which indicates they want to allocate
	 * from a special zone, so clear those bits here rather than forcing
	 * each caller to do it.  We only want to use their mask to indicate
	 * what strategies the memory allocator can use to free memory.
	 */
        state = kmem_cache_alloc(extent_state_cache, mask & GFP_RECLAIM_MASK);

I tend to lean towards being more terse, but it's not about me, it's
about whoever reads this code next.

> > +	state = kmem_cache_alloc(extent_state_cache, mask & GFP_RECLAIM_MASK);
> >  	if (!state)
> >  		return state;
> >  	state->state = 0;
>
David Sterba May 21, 2018, 5:06 p.m. UTC | #3
On Tue, May 15, 2018 at 04:54:04AM -0700, Matthew Wilcox wrote:
> > > Subject: btrfs: Allocate extents from ZONE_NORMAL
> > > From: Matthew Wilcox <mawilcox@microsoft.com>
> > > 
> > > If anyone ever passes a GFP_DMA or GFP_MOVABLE allocation flag to
> > > allocate_extent_state, it will try to allocate memory from the wrong zone.
> > > We just want to allocate memory from ZONE_NORMAL, so use GFP_RECLAIM_MASK
> > > to get what we want.
> > 
> > Looks good to me.
> > 
> > > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> > > 
> > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > > index e99b329002cf..4e4a67b7b29d 100644
> > > --- a/fs/btrfs/extent_io.c
> > > +++ b/fs/btrfs/extent_io.c
> > > @@ -216,12 +216,7 @@ static struct extent_state *alloc_extent_state(gfp_t mask)
> > >  {
> > >  	struct extent_state *state;
> > >  
> > > -	/*
> > > -	 * The given mask might be not appropriate for the slab allocator,
> > > -	 * drop the unsupported bits
> > > -	 */
> > > -	mask &= ~(__GFP_DMA32|__GFP_HIGHMEM);
> > 
> > I've noticed there's GFP_SLAB_BUG_MASK that's basically open coded here,
> > but this would not filter out the placement flags.
> > 
> > > -	state = kmem_cache_alloc(extent_state_cache, mask);
> > 
> > I'd prefer some comment here, it's not obvious why the mask is used.
> 
> Sorry, I dropped the ball on this.  Would you prefer:
> 
>         /* Allocate from ZONE_NORMAL */
>         state = kmem_cache_alloc(extent_state_cache, mask & GFP_RECLAIM_MASK);
> 
> or
> 
> 	/*
> 	 * Callers may pass in a mask which indicates they want to allocate
> 	 * from a special zone, so clear those bits here rather than forcing
> 	 * each caller to do it.  We only want to use their mask to indicate
> 	 * what strategies the memory allocator can use to free memory.
> 	 */
>         state = kmem_cache_alloc(extent_state_cache, mask & GFP_RECLAIM_MASK);
> 
> I tend to lean towards being more terse, but it's not about me, it's
> about whoever reads this code next.

I prefer the latter variant, it's clear that it's some MM stuff. Thanks.
diff mbox

Patch

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index e99b329002cf..4e4a67b7b29d 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -216,12 +216,7 @@  static struct extent_state *alloc_extent_state(gfp_t mask)
 {
 	struct extent_state *state;
 
-	/*
-	 * The given mask might be not appropriate for the slab allocator,
-	 * drop the unsupported bits
-	 */
-	mask &= ~(__GFP_DMA32|__GFP_HIGHMEM);
-	state = kmem_cache_alloc(extent_state_cache, mask);
+	state = kmem_cache_alloc(extent_state_cache, mask & GFP_RECLAIM_MASK);
 	if (!state)
 		return state;
 	state->state = 0;