[Intel-gfx] [PATCH 1/2] intel: Defer setting madv on the bo cache
Convert the bo-cache into 2 phases: 1. A two second cache of recently used buffers, keep untouched. 2. A two second cache of older buffers, marked for eviction. This helps reduce ioctl traffic on a rapid turnover in working sets. The issue is that even a 2 second window is enough for an application to fill all of memory with inactive buffers (and we would rely on the oom-killer identifying the right culprit). Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Daniel Vetter daniel.vet...@ffwll.ch Cc: Ben Widawsky benjamin.widaw...@intel.com --- intel/intel_bufmgr_gem.c | 42 +++--- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index f5d6130..ecbf8ee 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -211,6 +211,11 @@ struct _drm_intel_bo_gem { bool used_as_reloc_target; /** +* Are we keeping this buffer around in semi-permenant cache? +*/ + bool dontneed; + + /** * Boolean of whether we have encountered an error whilst building the relocation tree. */ bool has_error; @@ -714,7 +719,8 @@ retry: } if (alloc_from_cache) { - if (!drm_intel_gem_bo_madvise_internal + if (bo_gem-dontneed + !drm_intel_gem_bo_madvise_internal (bufmgr_gem, bo_gem, I915_MADV_WILLNEED)) { drm_intel_gem_bo_free(bo_gem-bo); drm_intel_gem_bo_cache_purge_bucket(bufmgr_gem, @@ -1150,6 +1156,20 @@ drm_intel_gem_bo_mark_mmaps_incoherent(drm_intel_bo *bo) #endif } +static inline void __splice(const drmMMListHead *list, + drmMMListHead *prev, + drmMMListHead *next) +{ + drmMMListHead *first = list-next; + drmMMListHead *last = list-prev; + + first-prev = prev; + prev-next = first; + + last-next = next; + next-prev = last; +} + /** Frees all cached buffers significantly older than @time. */ static void drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time) @@ -1162,19 +1182,29 @@ drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time) for (i = 0; i bufmgr_gem-num_buckets; i++) { struct drm_intel_gem_bo_bucket *bucket = bufmgr_gem-cache_bucket[i]; + drmMMListHead madv; + DRMINITLISTHEAD(madv); while (!DRMLISTEMPTY(bucket-head)) { drm_intel_bo_gem *bo_gem; bo_gem = DRMLISTENTRY(drm_intel_bo_gem, bucket-head.next, head); - if (time - bo_gem-free_time = 1) + if (time - bo_gem-free_time 2*(1+bo_gem-dontneed)) break; DRMLISTDEL(bo_gem-head); - - drm_intel_gem_bo_free(bo_gem-bo); + if (bo_gem-dontneed) { + drm_intel_gem_bo_free(bo_gem-bo); + } else { + drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem, + I915_MADV_DONTNEED); + bo_gem-dontneed = 1; + DRMLISTADDTAIL(bo_gem-head, madv); + } } + if (!DRMLISTEMPTY(madv)) + __splice(madv, bucket-head, bucket-head.next); } bufmgr_gem-time = time; @@ -1285,9 +1315,7 @@ drm_intel_gem_bo_unreference_final(drm_intel_bo *bo, time_t time) bucket = drm_intel_gem_bo_bucket_for_size(bufmgr_gem, bo-size); /* Put the buffer into our internal cache for reuse if we can. */ - if (bufmgr_gem-bo_reuse bo_gem-reusable bucket != NULL - drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem, - I915_MADV_DONTNEED)) { + if (bufmgr_gem-bo_reuse bo_gem-reusable bucket != NULL) { bo_gem-free_time = time; bo_gem-name = NULL; -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] intel: Defer setting madv on the bo cache
On Tue, Apr 14, 2015 at 12:38:56PM -0700, Ben Widawsky wrote: /** Frees all cached buffers significantly older than @time. */ static void drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time) @@ -1162,19 +1182,29 @@ drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time) for (i = 0; i bufmgr_gem-num_buckets; i++) { struct drm_intel_gem_bo_bucket *bucket = bufmgr_gem-cache_bucket[i]; + drmMMListHead madv; + DRMINITLISTHEAD(madv); while (!DRMLISTEMPTY(bucket-head)) { drm_intel_bo_gem *bo_gem; bo_gem = DRMLISTENTRY(drm_intel_bo_gem, bucket-head.next, head); - if (time - bo_gem-free_time = 1) + if (time - bo_gem-free_time 2*(1+bo_gem-dontneed)) Something somewhere will complain about mixing bools and ints, I'd guess. Also perhaps for perf bisection maybe do a patch before this changing the expiration time to 2 (or 4), then add the extra dontneed state? It is already 2, since it uses = and the patch just converts it to to be consistent after the multiplication. break; DRMLISTDEL(bo_gem-head); - - drm_intel_gem_bo_free(bo_gem-bo); + if (bo_gem-dontneed) { + drm_intel_gem_bo_free(bo_gem-bo); + } else { + drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem, + I915_MADV_DONTNEED); + bo_gem-dontneed = 1; + DRMLISTADDTAIL(bo_gem-head, madv); + } } Maybe add a comment here? /* Objects which have elapsed 2s of disuse should go to the top of the bucket. */ + if (!DRMLISTEMPTY(madv)) + __splice(madv, bucket-head, bucket-head.next); } bufmgr_gem-time = time; @@ -1285,9 +1315,7 @@ drm_intel_gem_bo_unreference_final(drm_intel_bo *bo, time_t time) bucket = drm_intel_gem_bo_bucket_for_size(bufmgr_gem, bo-size); /* Put the buffer into our internal cache for reuse if we can. */ - if (bufmgr_gem-bo_reuse bo_gem-reusable bucket != NULL - drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem, - I915_MADV_DONTNEED)) { + if (bufmgr_gem-bo_reuse bo_gem-reusable bucket != NULL) { bo_gem-free_time = time; bo_gem-name = NULL; In general, it looks fine to me. Like I said above wrt adding the patch before this, I am curious how much difference you see just messing with the expiration times versus the two state model. The issue I was looking at was simply struct_mutex contention in madvise (then busy). For busy we can tweak the ordering to reduce the contention slightly, but madvise is trickier (though madvise is a candidate for one of the first per-object locks). This just moves the contention elsewhere, but madvise/busy calls tend to dominate overall and trimming them reduces the intersection. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] intel: Defer setting madv on the bo cache
On Tue, Apr 14, 2015 at 04:08:44PM +0100, Chris Wilson wrote: Convert the bo-cache into 2 phases: 1. A two second cache of recently used buffers, keep untouched. 2. A two second cache of older buffers, marked for eviction. This helps reduce ioctl traffic on a rapid turnover in working sets. The issue is that even a 2 second window is enough for an application to fill all of memory with inactive buffers (and we would rely on the oom-killer identifying the right culprit). Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Daniel Vetter daniel.vet...@ffwll.ch Cc: Ben Widawsky benjamin.widaw...@intel.com --- intel/intel_bufmgr_gem.c | 42 +++--- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index f5d6130..ecbf8ee 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -211,6 +211,11 @@ struct _drm_intel_bo_gem { bool used_as_reloc_target; /** + * Are we keeping this buffer around in semi-permenant cache? + */ + bool dontneed; + + /** * Boolean of whether we have encountered an error whilst building the relocation tree. */ bool has_error; @@ -714,7 +719,8 @@ retry: } if (alloc_from_cache) { - if (!drm_intel_gem_bo_madvise_internal + if (bo_gem-dontneed + !drm_intel_gem_bo_madvise_internal (bufmgr_gem, bo_gem, I915_MADV_WILLNEED)) { drm_intel_gem_bo_free(bo_gem-bo); drm_intel_gem_bo_cache_purge_bucket(bufmgr_gem, @@ -1150,6 +1156,20 @@ drm_intel_gem_bo_mark_mmaps_incoherent(drm_intel_bo *bo) #endif } +static inline void __splice(const drmMMListHead *list, + drmMMListHead *prev, + drmMMListHead *next) +{ + drmMMListHead *first = list-next; + drmMMListHead *last = list-prev; + + first-prev = prev; + prev-next = first; + + last-next = next; + next-prev = last; +} + Seems worth adding to libdrm_lists.h /** Frees all cached buffers significantly older than @time. */ static void drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time) @@ -1162,19 +1182,29 @@ drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time) for (i = 0; i bufmgr_gem-num_buckets; i++) { struct drm_intel_gem_bo_bucket *bucket = bufmgr_gem-cache_bucket[i]; + drmMMListHead madv; + DRMINITLISTHEAD(madv); while (!DRMLISTEMPTY(bucket-head)) { drm_intel_bo_gem *bo_gem; bo_gem = DRMLISTENTRY(drm_intel_bo_gem, bucket-head.next, head); - if (time - bo_gem-free_time = 1) + if (time - bo_gem-free_time 2*(1+bo_gem-dontneed)) Something somewhere will complain about mixing bools and ints, I'd guess. Also perhaps for perf bisection maybe do a patch before this changing the expiration time to 2 (or 4), then add the extra dontneed state? break; DRMLISTDEL(bo_gem-head); - - drm_intel_gem_bo_free(bo_gem-bo); + if (bo_gem-dontneed) { + drm_intel_gem_bo_free(bo_gem-bo); + } else { + drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem, + I915_MADV_DONTNEED); + bo_gem-dontneed = 1; + DRMLISTADDTAIL(bo_gem-head, madv); + } } Maybe add a comment here? /* Objects which have elapsed 2s of disuse should go to the top of the bucket. */ + if (!DRMLISTEMPTY(madv)) + __splice(madv, bucket-head, bucket-head.next); } bufmgr_gem-time = time; @@ -1285,9 +1315,7 @@ drm_intel_gem_bo_unreference_final(drm_intel_bo *bo, time_t time) bucket = drm_intel_gem_bo_bucket_for_size(bufmgr_gem, bo-size); /* Put the buffer into our internal cache for reuse if we can. */ - if (bufmgr_gem-bo_reuse bo_gem-reusable bucket != NULL - drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem, - I915_MADV_DONTNEED)) { + if (bufmgr_gem-bo_reuse bo_gem-reusable bucket != NULL) { bo_gem-free_time = time; bo_gem-name = NULL; In general, it looks fine to me. Like I said above wrt adding the patch before this, I am curious how much difference you see just messing with the expiration times versus the two state model. -- Ben Widawsky,