[Intel-gfx] [PATCH 2/2] drm/i915: Make GPU pages movable

[Hugh Dickins]

On Mon, 14 Nov 2016, akash goel wrote:
> On Thu, Nov 10, 2016 at 1:00 PM, Goel, Akash <akash.goel at intel.com> wrote:
> > On 11/10/2016 12:09 PM, Hugh Dickins wrote:
> >> On Fri, 4 Nov 2016, akash.goel at intel.com wrote:
> >>> @@ -4185,6 +4189,8 @@ struct drm_i915_gem_object *
> >>>
> >>>         mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
> >>> +       if (IS_ENABLED(MIGRATION))

Oh, I knew I'd seen a line like that recently, and it was bugging me
that I ought to search my mailboxes for it; but now I'm glad to find
it again.  If that condition stays, it would really need to say
              if (IS_ENABLED(CONFIG_MIGRATION))
wouldn't it?

> >>> +               mask |= __GFP_MOVABLE;
> >>
> >>
> >> I was going to suggest just make that unconditional,
> >>         mask = GFP_HIGHUSER_MOVABLE | __GFP_RECLAIMABLE;
> >>
> >> But then I wondered what that __GFP_RECLAIMABLE actually achieves?
> >> These pages are already __GFP_RECLAIM (inside GFP_HIGHUSER) and on
> >> the LRU.  It affects gfpflags_to_migratetype(), but I'm not familiar
> >> with what that different migratetype will end up doing.
> >>
> >
> > Will check for this.
> >
> 
> The anti-fragmentation technique used by kernel is based on the idea
> of grouping pages with identical mobility (UNMOVABLE, RECLAIMABLE,
> MOVABLE) together.

Yes.

> __GFP_RECLAIMABLE, like  __GFP_MOVABLE, specifies the
> mobility/migration type of the page and serves a different purpose
> than __GFP_RECLAIM.

Yes, I was wrong to mention __GFP_RECLAIM above: it describes what
to do when in difficulty allocating a page, but says nothing at all
about the nature of the page to be allocated.

> 
> Also as per the below snippet from gfpflags_to_migratetype(), looks
> like __GFP_MOVABLE &  __GFP_RECLAIMABLE can't be used together, which
> makes sense.
> /* Convert GFP flags to their corresponding migrate type */
> #define GFP_MOVABLE_MASK (__GFP_RECLAIMABLE | __GFP_MOVABLE)
> static inline int gfpflags_to_migratetype(const gfp_t gfp_flags)
> {
>         VM_WARN_ON((gfp_flags & GFP_MOVABLE_MASK) == GFP_MOVABLE_MASK);
> .....

You're right, that does exclude them from being used together.  And it
makes sense inasmuch as they're expected to be appled to quite different
uses of a page (lru pages versus slab pages).

The comment on __GFP_MOVABLE says "or can be reclaimed"; and
the comment on __GFP_RECLAIMABLE says "used for slab allocations...".
Though it does not say "used for allocations not put on a reclaimable
lru", I think that is the intention; whereas shmem allocations are put
on a reclaimable lru (though they might need your shrinker to unpin them).

> 
> So probably would need to update the mask like this,
>        mask = GFP_HIGHUSER;
>        if (IS_ENABLED(MIGRATION))
>              mask |= __GFP_MOVABLE;
>        else
>              mask |=  __GFP_RECLAIMABLE;
> 
> Please kindly let us know if this looks fine to you or not.

Thanks for looking into it more deeply.  You leave me thinking that
it should simply say

        mask = GFP_HIGHUSER_MOVABLE;

Which is the default anyway, but it then has the Crestline+Broadwater
condition to modify the mask further, so it's probably clearest to
leave the mask = GFP_HIGHUSER_MOVABLE explicit.

GFP_HIGHUSER_MOVABLE is used in many places, and includes __GFP_MOVABLE
without any condition on CONFIG_MIGRATION - because the migratetype is
irrelevant if there is no migration, I presume.

Would you lose something by not or'ing in __GFP_RECLAIMABLE when
CONFIG_MIGRATION=n?  No, because __GFP_RECLAIMABLE is not used for
anything but the migratetype, and the migratetype is then irrelevant.
(I didn't study the code closely enough to say whether the grouping
can still happen even when migration is disabled, but even if it
does still happen, I can't see that it would have any benefit.)

Hugh