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

[akash goel]

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:
>>>
>>> From: Chris Wilson <chris at chris-wilson.co.uk>
>>>
>>> On a long run of more than 2-3 days, physical memory tends to get
>>> fragmented severely, which considerably slows down the system. In such a
>>> scenario, the shrinker is also unable to help as lack of memory is not
>>> the actual problem, since it has been observed that there are enough free
>>> pages of 0 order. This also manifests itself when an indiviual zone in
>>> the mm runs out of pages and if we cannot migrate pages between zones,
>>> the kernel hits an out-of-memory even though there are free pages (and
>>> often all of swap) available.
>>>
>>> To address the issue of external fragementation, kernel does a compaction
>>> (which involves migration of pages) but it's efficacy depends upon how
>>> many pages are marked as MOVABLE, as only those pages can be migrated.
>>>
>>> Currently the backing pages for GPU buffers are allocated from shmemfs
>>> with GFP_RECLAIMABLE flag, in units of 4KB pages.  In the case of limited
>>> swap space, it may not be possible always to reclaim or swap-out pages of
>>> all the inactive objects, to make way for free space allowing formation
>>> of higher order groups of physically-contiguous pages on compaction.
>>>
>>> Just marking the GPU pages as MOVABLE will not suffice, as i915.ko has to
>>> pin the pages if they are in use by GPU, which will prevent their
>>> migration. So the migratepage callback in shmem is also hooked up to get
>>> a notification when kernel initiates the page migration. On the
>>> notification, i915.ko appropriately unpin the pages.  With this we can
>>> effectively mark the GPU pages as MOVABLE and hence mitigate the
>>> fragmentation problem.
>>>
>>> v2:
>>>  - Rename the migration routine to gem_shrink_migratepage, move it to the
>>>    shrinker file, and use the existing constructs (Chris)
>>>  - To cleanup, add a new helper function to encapsulate all page
>>> migration
>>>    skip conditions (Chris)
>>>  - Add a new local helper function in shrinker file, for dropping the
>>>    backing pages, and call the same from gem_shrink() also (Chris)
>>>
>>> v3:
>>>  - Fix/invert the check on the return value of unsafe_drop_pages (Chris)
>>>
>>> v4:
>>>  - Minor tidy
>>>
>>> v5:
>>>  - Fix unsafe usage of unsafe_drop_pages()
>>>  - Rebase onto vmap-notifier
>>>
>>> v6:
>>> - Remove i915_gem_object_get/put across unsafe_drop_pages() as with
>>>   struct_mutex protection object can't disappear. (Chris)
>>>
>>> Testcase: igt/gem_shrink
>>> Bugzilla: (e.g.) https://bugs.freedesktop.org/show_bug.cgi?id=90254
>>> Cc: Hugh Dickins <hughd at google.com>
>>> Cc: linux-mm at kvack.org
>>> Signed-off-by: Sourab Gupta <sourab.gupta at intel.com>
>>> Signed-off-by: Akash Goel <akash.goel at intel.com>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
>>
>>
>> I'm confused!  But perhaps it's gone around and around between you all,
>> I'm not sure what the rules are then.  I think this sequence implies
>> that Sourab wrote it originally, then Akash and Chris passed it on
>> with refinements - but then Chris wouldn't add Reviewed-by.
>>
> Thank you very much for the review and sorry for all the needless confusion.
>
> Chris actually conceived the patches and prepared an initial version of them
> (hence he is the Author).
> I & Sourab did the further refinements and fixed issues (all those
> page_private stuff).
> Chris then reviewed the final patch and also recently did a rebase for it.
>
>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h          |   2 +
>>>  drivers/gpu/drm/i915/i915_gem.c          |   9 ++-
>>>  drivers/gpu/drm/i915/i915_gem_shrinker.c | 132
>>> +++++++++++++++++++++++++++++++
>>>  3 files changed, 142 insertions(+), 1 deletion(-)
>>>
snip
>>
>>> @@ -4185,6 +4189,8 @@ struct drm_i915_gem_object *
>>>                 goto fail;
>>>
>>>         mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
>>> +       if (IS_ENABLED(MIGRATION))
>>> +               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.
__GFP_RECLAIMABLE, like  __GFP_MOVABLE, specifies the
mobility/migration type of the page and serves a different purpose
than __GFP_RECLAIM.

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);
.....

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.

Best regards
Akash

>
>>>         if (IS_CRESTLINE(dev_priv) || IS_BROADWATER(dev_priv)) {
>>>                 /* 965gm cannot relocate objects above 4GiB. */
>>>                 mask &= ~__GFP_HIGHMEM;
>>> @@ -4193,6 +4199,7 @@ struct drm_i915_gem_object *
>>>
>>>         mapping = obj->base.filp->f_mapping;
>>>         mapping_set_gfp_mask(mapping, mask);
>>> +       shmem_set_dev_info(mapping, &dev_priv->mm.shmem_info);
>>>
>>>         i915_gem_object_init(obj, &i915_gem_object_ops);
>>>
>>>  }
>>>
>>>  /**
>>> --
>>> 1.9.2