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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>