lkml.org 
[lkml]   [2015]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 04/10] mm, page_alloc: Use masks and shifts when converting GFP flags to migrate types
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.


\
 
 \ /
  Last update: 2015-09-25 16:21    [W:0.091 / U:3.212 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site