On 12/02/2014 03:13 AM, Chris Wilson wrote: > On Mon, Dec 01, 2014 at 02:39:51PM -0800, Michael H. Nguyen wrote: >> Re: madvise on creation >> >> Were you referring to this? >> >> from http://lists.freedesktop.org/archives/intel-gfx/2014-November/055060.htm >> >> obj = i915_gem_obj_alloc(); >> i915_gem_object_get_pages(obj); >> obj->madv = I915_MADV_DONTNEED; >> >> If so, I don't understand . _get is returning obj and it'll be >> needed so would expect to set 'obj->madv = I915_MADV_WILLNEED' which >> is the case now. > > madv is only evaluated at get_pages(). Once you have the pages, you keep > them until the shrinker purges them. Hence you only need to call > get_pages() once and set obj->madv = DONTNEED afterwards, and then you > only need to check whether the obj is purged before your next reuse (you > do not need to touch madv ever again). Whilst the object is active it is > a low priority target for the shrinker. That greatly simplifies the pool > code. I have a feeling this may make the driver less readable imo and could also require a re-write of the series. The current code may call get_pages() more than once and occurs outside of the batch_pool management fncs. Would have to re-write things to stop that. i915_parse_cmds() copy_batch() i915_gem_obj_prepare_shmem_read() i915_gem_object_get_pages() After removing the fancy retry loop suggested by Daniel and moving to a single cache list, the implementation looks very simple imo. And, setting 'obj->madv = I915_MADV_WILLNEED;' looks right. Readability wise, you don't have to investigate further to understand the justification for that statement. Here is an RFC snippet... i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool, size_t size) { struct drm_i915_gem_object *obj = NULL; struct drm_i915_gem_object *tmp, *next; WARN_ON(!mutex_is_locked(&pool->dev->struct_mutex)); list_for_each_entry_safe(tmp, next, &pool->cache_list, batch_pool_list) { if (tmp->active) continue; if (tmp->madv == __I915_MADV_PURGED) { list_del(&tmp->batch_pool_list); drm_gem_object_unreference(&tmp->base); continue; } if (tmp->base.size >= size && tmp->base.size <= (2 * size)) { obj = tmp; break; } } if (!obj) { obj = i915_gem_alloc_object(pool->dev, size); if (!obj) return ERR_PTR(-ENOMEM); list_add_tail(&obj->batch_pool_list, &pool->cache_list); } obj->madv = I915_MADV_WILLNEED; return obj; } Given the spirit of your feedback was to simplify pool_get(), does this RFC do it for you? If not, I kindly request we have a sync up meeting to discuss your 'obj->madv = DONTNEED' suggestion. Thank you much, -Mike > -Chris > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
