Re: [PATCH 04/10] mm, page_alloc: Use masks and shifts when converting GFP flags to migrate types

[Mel Gorman <mgorman@techsingularity.net>]

On Thu, Sep 24, 2015 at 04:34:45PM -0400, Johannes Weiner wrote:
> On Mon, Sep 21, 2015 at 11:52:36AM +0100, Mel Gorman wrote:
> > @@ -14,7 +14,7 @@ struct vm_area_struct;
> >  #define ___GFP_HIGHMEM		0x02u
> >  #define ___GFP_DMA32		0x04u
> >  #define ___GFP_MOVABLE		0x08u
> > -#define ___GFP_WAIT		0x10u
> > +#define ___GFP_RECLAIMABLE	0x10u
> >  #define ___GFP_HIGH		0x20u
> >  #define ___GFP_IO		0x40u
> >  #define ___GFP_FS		0x80u
> > @@ -29,7 +29,7 @@ struct vm_area_struct;
> >  #define ___GFP_NOMEMALLOC	0x10000u
> >  #define ___GFP_HARDWALL		0x20000u
> >  #define ___GFP_THISNODE		0x40000u
> > -#define ___GFP_RECLAIMABLE	0x80000u
> > +#define ___GFP_WAIT		0x80000u
> >  #define ___GFP_NOACCOUNT	0x100000u
> >  #define ___GFP_NOTRACK		0x200000u
> >  #define ___GFP_NO_KSWAPD	0x400000u
> > @@ -126,6 +126,7 @@ struct vm_area_struct;
> >  
> >  /* This mask makes up all the page movable related flags */
> >  #define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE|__GFP_MOVABLE)
> > +#define GFP_MOVABLE_SHIFT 3
> 
> This connects the power-of-two gfp bits to the linear migrate type
> enum, so shifting back and forth between them works only with up to
> two items. A hypothetical ___GFP_FOOABLE would translate to 4, not
> 3. I'm not expecting new migratetypes to show up anytime soon, but
> this implication does not make the code exactly robust and obvious.
> 

In the event __GFP_FOOABLE is added then it would need to be reverted.
Adding new migrate types has other consequences as it increases the number
of free lists in struct zone and depending on the type then new per-cpu
lists and the the fallbacks have to be updated. It's fairly obvious if a
new migratetype is added that cares.

> > @@ -152,14 +153,15 @@ struct vm_area_struct;
> >  /* Convert GFP flags to their corresponding migrate type */
> >  static inline int gfpflags_to_migratetype(const gfp_t gfp_flags)
> >  {
> > -	WARN_ON((gfp_flags & GFP_MOVABLE_MASK) == GFP_MOVABLE_MASK);
> > +	VM_WARN_ON((gfp_flags & GFP_MOVABLE_MASK) == GFP_MOVABLE_MASK);
> > +	BUILD_BUG_ON((1UL << GFP_MOVABLE_SHIFT) != ___GFP_MOVABLE);
> > +	BUILD_BUG_ON((___GFP_MOVABLE >> GFP_MOVABLE_SHIFT) != MIGRATE_MOVABLE);
> >  
> >  	if (unlikely(page_group_by_mobility_disabled))
> >  		return MIGRATE_UNMOVABLE;
> >  
> >  	/* Group based on mobility */
> > -	return (((gfp_flags & __GFP_MOVABLE) != 0) << 1) |
> > -		((gfp_flags & __GFP_RECLAIMABLE) != 0);
> > +	return (gfp_flags & GFP_MOVABLE_MASK) >> GFP_MOVABLE_SHIFT;
> 
> I'm not sure the simplification of this line is worth the fragile
> dependency between those two tables.

The BUILD_BUG_ON is there to blow up immediately if the dependency is
broken. Sure, it's complexity but it's well contained in a single
place. Do you want to insist the patch is dropped in case someone
decides to add a new migrate type that has per-cpu lists?

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>