On Thu, Nov 10, 2016 at 1:00 PM, Goel, Akash <akash.goel@xxxxxxxxx> wrote: > > > On 11/10/2016 12:09 PM, Hugh Dickins wrote: >> >> On Fri, 4 Nov 2016, akash.goel@xxxxxxxxx wrote: >>> >>> From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >>> >>> 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@xxxxxxxxxx> >>> Cc: linux-mm@xxxxxxxxx >>> Signed-off-by: Sourab Gupta <sourab.gupta@xxxxxxxxx> >>> Signed-off-by: Akash Goel <akash.goel@xxxxxxxxx> >>> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >>> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> >>> Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >> >> >> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
