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

[Johannes Weiner]

On Fri, Sep 25, 2015 at 01:50:28PM +0100, Mel Gorman wrote:
> 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.

As I said, I'm not really worried about somebody screwing up when
actually adding a new migratetype, but rather people having a harder
time reviewing and verifying the code later on and going through the
same "how can it translate a power of two to linear with a shift op?
oh, it relies on the sequences coinciding for the first two elements"
like I did. The instance works fine, but it's a questionable pattern.

> > > @@ -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?

I think the previous code is easier to comprehend and the optimization
looks miniscule. It doesn't seem worth it to me to add complications
and subtleties to a complex codebase for microoptimizations whose
effects are likely in the noise.

OTOH several others have acked this patch. If nobody else shares my
concern then I'm not insisting that the patch be dropped.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/