| Message ID | 1396900911-12415-4-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2014-04-07 at 20:01 +0000, Rodrigo Vivi wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > If we run out of stolen memory when trying to allocate an object, see if > we can reap enough purgeable objects to free up enough contiguous free > space for the allocation. This is in principle very much like evicting > objects to free up enough contiguous space in the vma when binding > a new object - and you will be forgiven for thinking that the code looks > very similar. > > At the moment, we do not allow userspace to allocate objects in stolen, > so there is neither the memory pressure to trigger stolen eviction nor > any purgeable objects inside the stolen arena. However, this will change > in the near future, and so better management and defragmentation of > stolen memory will become a real issue. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: "Gupta, Sourab" <sourab.gupta@intel.com> > Cc: "Goel, Akash" <akash.goel@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > --- > drivers/gpu/drm/i915/i915_gem_stolen.c | 119 ++++++++++++++++++++++++++++++--- > 1 file changed, 108 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 62ef55b..1fc1986 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -329,18 +329,25 @@ cleanup: > return NULL; > } > > -struct drm_i915_gem_object * > -i915_gem_object_create_stolen(struct drm_device *dev, u32 size) > +static bool mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind) > +{ > + if (obj->stolen == NULL) > + return false; > + > + if (obj->madv != I915_MADV_DONTNEED) > + return false; > + > + list_add(&obj->obj_exec_link, unwind); > + return drm_mm_scan_add_block(obj->stolen); > +} > + > +static struct drm_mm_node *stolen_alloc(struct drm_i915_private *dev_priv, u32 size) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > - struct drm_i915_gem_object *obj; > struct drm_mm_node *stolen; > + struct drm_i915_gem_object *obj; > + struct list_head unwind, evict; > int ret; > > - if (!drm_mm_initialized(&dev_priv->mm.stolen)) > - return NULL; > - > - DRM_DEBUG_KMS("creating stolen object: size=%x\n", size); > if (size == 0) > return NULL; > > @@ -350,11 +357,101 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size) > > ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size, > 4096, DRM_MM_SEARCH_DEFAULT); > - if (ret) { > - kfree(stolen); > - return NULL; > + if (ret == 0) > + return stolen; > + > + /* No more stolen memory available, or too fragmented. > + * Try evicting purgeable objects and search again. > + */ > + > + drm_mm_init_scan(&dev_priv->mm.stolen, size, 4096, 0); > + INIT_LIST_HEAD(&unwind); > + > + list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list) > + if (mark_free(obj, &unwind)) > + goto found; > + > + list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) > + if (mark_free(obj, &unwind)) > + goto found; > + > +found: > + INIT_LIST_HEAD(&evict); > + while (!list_empty(&unwind)) { > + obj = list_first_entry(&unwind, > + struct drm_i915_gem_object, > + obj_exec_link); > + list_del_init(&obj->obj_exec_link); > + > + if (drm_mm_scan_remove_block(obj->stolen)) { > + list_add(&obj->obj_exec_link, &evict); > + drm_gem_object_reference(&obj->base); > + } > } > > + ret = 0; > + while (!list_empty(&evict)) { > + obj = list_first_entry(&evict, > + struct drm_i915_gem_object, > + obj_exec_link); > + list_del_init(&obj->obj_exec_link); > + > + if (ret == 0) { > + struct i915_vma *vma, *vma_next; > + > + list_for_each_entry_safe(vma, vma_next, > + &obj->vma_list, > + vma_link) > + if (i915_vma_unbind(vma)) > + break; > + > + /* Stolen pins its pages to prevent the > + * normal shrinker from processing stolen > + * objects. > + */ > + i915_gem_object_unpin_pages(obj); > + > + ret = i915_gem_object_put_pages(obj); > + if (ret == 0) { > + obj->madv = __I915_MADV_PURGED; > + > + kfree(obj->stolen); > + obj->stolen = NULL; > + } else > + i915_gem_object_pin_pages(obj); > + } > + > + drm_gem_object_unreference(&obj->base); > + } > + > + if (ret == 0) > + ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size, > + 4096, DRM_MM_SEARCH_DEFAULT); > + if (ret == 0) > + return stolen; > + > + kfree(stolen); > + return NULL; > +} > + > +struct drm_i915_gem_object * > +i915_gem_object_create_stolen(struct drm_device *dev, u32 size) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_i915_gem_object *obj; > + struct drm_mm_node *stolen; > + > + lockdep_assert_held(&dev->struct_mutex); > + > + if (!drm_mm_initialized(&dev_priv->mm.stolen)) > + return NULL; > + > + DRM_DEBUG_KMS("creating stolen object: size=%x\n", size); > + > + stolen = stolen_alloc(dev_priv, size); > + if (stolen == NULL) > + return NULL; > + > obj = _i915_gem_object_create_stolen(dev, stolen); > if (obj) > return obj; Hi Rodrigo, In this patch, while freeing the purgeable stolen object, the memory node also has to be freed, so as to make space for new object. We need to call drm_mm_remove_node while freeing obj. The below modification patch was floated earlier for this purpose: http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html Regards, Sourab
On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote: > Hi Rodrigo, > In this patch, while freeing the purgeable stolen object, the memory > node also has to be freed, so as to make space for new object. We need > to call drm_mm_remove_node while freeing obj. > > The below modification patch was floated earlier for this purpose: > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html Right, I have a v2 locally with the fix you identified. -Chris
On Tue, 2014-04-08 at 06:45 +0000, Chris Wilson wrote: > On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote: > > Hi Rodrigo, > > In this patch, while freeing the purgeable stolen object, the memory > > node also has to be freed, so as to make space for new object. We need > > to call drm_mm_remove_node while freeing obj. > > > > The below modification patch was floated earlier for this purpose: > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html > > Right, I have a v2 locally with the fix you identified. > -Chris > Ok, Thanks Chris. Regards, Sourab
On Tue, Apr 08, 2014 at 06:53:03AM +0000, Gupta, Sourab wrote: > On Tue, 2014-04-08 at 06:45 +0000, Chris Wilson wrote: > > On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote: > > > Hi Rodrigo, > > > In this patch, while freeing the purgeable stolen object, the memory > > > node also has to be freed, so as to make space for new object. We need > > > to call drm_mm_remove_node while freeing obj. > > > > > > The below modification patch was floated earlier for this purpose: > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html > > > > Right, I have a v2 locally with the fix you identified. > > -Chris > > > Ok, Thanks Chris. I'd really prefer if someone would pick up all the stolen/create2_ioctl/whatever patches, pack them up into a polished series, add the testcases and submit this all for review and merging. Otherwise this will linger forever and we'll get nowhere. Chris seems swamped with other stuff, so Sourab could you please take a look at this? Please check with your manager that you have sufficient bandwidth to pull this through. Rodrigo, I think you can drop this patch from -collector, it only really makes sense in the context of all the other stolen work. Thanks, Daniel
On Wed, 2014-04-09 at 13:06 +0000, Daniel Vetter wrote: > On Tue, Apr 08, 2014 at 06:53:03AM +0000, Gupta, Sourab wrote: > > On Tue, 2014-04-08 at 06:45 +0000, Chris Wilson wrote: > > > On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote: > > > > Hi Rodrigo, > > > > In this patch, while freeing the purgeable stolen object, the memory > > > > node also has to be freed, so as to make space for new object. We need > > > > to call drm_mm_remove_node while freeing obj. > > > > > > > > The below modification patch was floated earlier for this purpose: > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html > > > > > > Right, I have a v2 locally with the fix you identified. > > > -Chris > > > > > Ok, Thanks Chris. > > I'd really prefer if someone would pick up all the > stolen/create2_ioctl/whatever patches, pack them up into a polished > series, add the testcases and submit this all for review and merging. > > Otherwise this will linger forever and we'll get nowhere. Chris seems > swamped with other stuff, so Sourab could you please take a look at this? > > Please check with your manager that you have sufficient bandwidth to pull > this through. > I'll be on vacation from next week, so I'll be able to gauge this better after coming back. Nevertheless, I have some questions regarding the expectation of userspace code changes required for these patches (i.e. libdrm changes and igt testcases) 1) For libdrm , I am assuming, a counterpart of drm_intel_gem_bo_alloc_tiled() function would call the create2 ioctl and take in the parameters needed. Should the caching of objects from libdrm need to take care of both the placement domains seperately (as in different sets of bo buckets)? Should libdrm be transparent to all the combinations of different parameters being passed by user or should the prohibited combinations be disallowed from libdrm side? 2) For the igt, since we have a lot of parameters exposed to user, the number of subtests required may be huge and still they may not test out everything. So, Is the expectation here to have exhaustive test cases for all the parameters (tiling/cache/domain/madvise/offset etc.) going in as input to the create2 ioctl? For eg. let us say we are going to check the render copy operation of src and dest bo's. Do we need to provide all possible combinations of different (create2 ioctl) input parameters to these src and dest bo's and then run the render copy test for all these combinations. Any guiding pointers from your side as to how we may go about the igt testcases? Thanks, sourab > Rodrigo, I think you can drop this patch from -collector, it only really > makes sense in the context of all the other stolen work. > > Thanks, Daniel
On Thu, Apr 10, 2014 at 10:12:39AM +0000, Gupta, Sourab wrote: > On Wed, 2014-04-09 at 13:06 +0000, Daniel Vetter wrote: > > On Tue, Apr 08, 2014 at 06:53:03AM +0000, Gupta, Sourab wrote: > > > On Tue, 2014-04-08 at 06:45 +0000, Chris Wilson wrote: > > > > On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote: > > > > > Hi Rodrigo, > > > > > In this patch, while freeing the purgeable stolen object, the memory > > > > > node also has to be freed, so as to make space for new object. We need > > > > > to call drm_mm_remove_node while freeing obj. > > > > > > > > > > The below modification patch was floated earlier for this purpose: > > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html > > > > > > > > Right, I have a v2 locally with the fix you identified. > > > > -Chris > > > > > > > Ok, Thanks Chris. > > > > I'd really prefer if someone would pick up all the > > stolen/create2_ioctl/whatever patches, pack them up into a polished > > series, add the testcases and submit this all for review and merging. > > > > Otherwise this will linger forever and we'll get nowhere. Chris seems > > swamped with other stuff, so Sourab could you please take a look at this? > > > > Please check with your manager that you have sufficient bandwidth to pull > > this through. > > > > I'll be on vacation from next week, so I'll be able to gauge this better > after coming back. > Nevertheless, I have some questions regarding the expectation of > userspace code changes required for these patches (i.e. libdrm changes > and igt testcases) > > 1) For libdrm , I am assuming, a counterpart of > drm_intel_gem_bo_alloc_tiled() function would call the create2 ioctl and > take in the parameters needed. > Should the caching of objects from libdrm need to take care of both the > placement domains seperately (as in different sets of bo buckets)? > Should libdrm be transparent to all the combinations of different > parameters being passed by user or should the prohibited combinations be > disallowed from libdrm side? I'm not sure whether we need a cache implemented in libdrm. Since stolen objects are fairly special it's probably easier to just have a simple linear cache tailor-made in the respective UMD. So just exposing create2_ioctl should be good enough. > 2) For the igt, since we have a lot of parameters exposed to user, the > number of subtests required may be huge and still they may not test out > everything. > So, Is the expectation here to have exhaustive test cases for all the > parameters (tiling/cache/domain/madvise/offset etc.) going in as input > to the create2 ioctl? > For eg. let us say we are going to check the render copy operation of > src and dest bo's. Do we need to provide all possible combinations of > different (create2 ioctl) input parameters to these src and dest bo's > and then run the render copy test for all these combinations. > Any guiding pointers from your side as to how we may go about the igt > testcases? At a high-level there's two parts for igt tests: - First is functional tests, where we try to make sure that the feature actually works. I.e. allocate some stolen memory and then do something with it, making sure that data access for the gpu and similar things work. For this we just want some reasonable base coverage so that when we hit a bug somewhere it's easy to extend the testcase to cover that bug with a specific subtest. - Then there's interface testing. kernel/userspace is a trust barrier, so we need to make sure that evil userspace can't make the kernel crash with some crazy invalid combination of flags or operations (like create a stolen object and then try to mmap it). Since this is security relevant and also since we can't ever change established userspace ABI I want full coverage of all cases. But this is just about detecting abuse correctly, no functional tests here. For details see my two blog posts on the topic: http://blog.ffwll.ch/2013/11/botching-up-ioctls.html http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html Cheers, Daniel
On Fri, 2014-04-11 at 08:55 +0000, Daniel Vetter wrote: > On Thu, Apr 10, 2014 at 10:12:39AM +0000, Gupta, Sourab wrote: > > On Wed, 2014-04-09 at 13:06 +0000, Daniel Vetter wrote: > > > On Tue, Apr 08, 2014 at 06:53:03AM +0000, Gupta, Sourab wrote: > > > > On Tue, 2014-04-08 at 06:45 +0000, Chris Wilson wrote: > > > > > On Tue, Apr 08, 2014 at 04:32:02AM +0000, Gupta, Sourab wrote: > > > > > > Hi Rodrigo, > > > > > > In this patch, while freeing the purgeable stolen object, the memory > > > > > > node also has to be freed, so as to make space for new object. We need > > > > > > to call drm_mm_remove_node while freeing obj. > > > > > > > > > > > > The below modification patch was floated earlier for this purpose: > > > > > > http://lists.freedesktop.org/archives/intel-gfx/2014-March/041282.html > > > > > > > > > > Right, I have a v2 locally with the fix you identified. > > > > > -Chris > > > > > > > > > Ok, Thanks Chris. > > > > > > I'd really prefer if someone would pick up all the > > > stolen/create2_ioctl/whatever patches, pack them up into a polished > > > series, add the testcases and submit this all for review and merging. > > > > > > Otherwise this will linger forever and we'll get nowhere. Chris seems > > > swamped with other stuff, so Sourab could you please take a look at this? > > > > > > Please check with your manager that you have sufficient bandwidth to pull > > > this through. > > > > > > > I'll be on vacation from next week, so I'll be able to gauge this better > > after coming back. > > Nevertheless, I have some questions regarding the expectation of > > userspace code changes required for these patches (i.e. libdrm changes > > and igt testcases) > > > > 1) For libdrm , I am assuming, a counterpart of > > drm_intel_gem_bo_alloc_tiled() function would call the create2 ioctl and > > take in the parameters needed. > > Should the caching of objects from libdrm need to take care of both the > > placement domains seperately (as in different sets of bo buckets)? > > Should libdrm be transparent to all the combinations of different > > parameters being passed by user or should the prohibited combinations be > > disallowed from libdrm side? > > I'm not sure whether we need a cache implemented in libdrm. Since stolen > objects are fairly special it's probably easier to just have a simple > linear cache tailor-made in the respective UMD. So just exposing > create2_ioctl should be good enough. > > > 2) For the igt, since we have a lot of parameters exposed to user, the > > number of subtests required may be huge and still they may not test out > > everything. > > So, Is the expectation here to have exhaustive test cases for all the > > parameters (tiling/cache/domain/madvise/offset etc.) going in as input > > to the create2 ioctl? > > For eg. let us say we are going to check the render copy operation of > > src and dest bo's. Do we need to provide all possible combinations of > > different (create2 ioctl) input parameters to these src and dest bo's > > and then run the render copy test for all these combinations. > > Any guiding pointers from your side as to how we may go about the igt > > testcases? > > At a high-level there's two parts for igt tests: > - First is functional tests, where we try to make sure that the feature > actually works. I.e. allocate some stolen memory and then do something > with it, making sure that data access for the gpu and similar things > work. For this we just want some reasonable base coverage so that when > we hit a bug somewhere it's easy to extend the testcase to cover that > bug with a specific subtest. > > - Then there's interface testing. kernel/userspace is a trust barrier, so > we need to make sure that evil userspace can't make the kernel crash > with some crazy invalid combination of flags or operations (like create > a stolen object and then try to mmap it). Since this is security > relevant and also since we can't ever change established userspace ABI I > want full coverage of all cases. But this is just about detecting abuse > correctly, no functional tests here. > > For details see my two blog posts on the topic: > > http://blog.ffwll.ch/2013/11/botching-up-ioctls.html > > http://blog.ffwll.ch/2013/11/testing-requirements-for-drmi915.html > > Cheers, Daniel Thanks Daniel, We'll take care of the above points for libdrm changes and igt. Regards, Sourab
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 62ef55b..1fc1986 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -329,18 +329,25 @@ cleanup: return NULL; } -struct drm_i915_gem_object * -i915_gem_object_create_stolen(struct drm_device *dev, u32 size) +static bool mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind) +{ + if (obj->stolen == NULL) + return false; + + if (obj->madv != I915_MADV_DONTNEED) + return false; + + list_add(&obj->obj_exec_link, unwind); + return drm_mm_scan_add_block(obj->stolen); +} + +static struct drm_mm_node *stolen_alloc(struct drm_i915_private *dev_priv, u32 size) { - struct drm_i915_private *dev_priv = dev->dev_private; - struct drm_i915_gem_object *obj; struct drm_mm_node *stolen; + struct drm_i915_gem_object *obj; + struct list_head unwind, evict; int ret; - if (!drm_mm_initialized(&dev_priv->mm.stolen)) - return NULL; - - DRM_DEBUG_KMS("creating stolen object: size=%x\n", size); if (size == 0) return NULL; @@ -350,11 +357,101 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size) ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size, 4096, DRM_MM_SEARCH_DEFAULT); - if (ret) { - kfree(stolen); - return NULL; + if (ret == 0) + return stolen; + + /* No more stolen memory available, or too fragmented. + * Try evicting purgeable objects and search again. + */ + + drm_mm_init_scan(&dev_priv->mm.stolen, size, 4096, 0); + INIT_LIST_HEAD(&unwind); + + list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list) + if (mark_free(obj, &unwind)) + goto found; + + list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) + if (mark_free(obj, &unwind)) + goto found; + +found: + INIT_LIST_HEAD(&evict); + while (!list_empty(&unwind)) { + obj = list_first_entry(&unwind, + struct drm_i915_gem_object, + obj_exec_link); + list_del_init(&obj->obj_exec_link); + + if (drm_mm_scan_remove_block(obj->stolen)) { + list_add(&obj->obj_exec_link, &evict); + drm_gem_object_reference(&obj->base); + } } + ret = 0; + while (!list_empty(&evict)) { + obj = list_first_entry(&evict, + struct drm_i915_gem_object, + obj_exec_link); + list_del_init(&obj->obj_exec_link); + + if (ret == 0) { + struct i915_vma *vma, *vma_next; + + list_for_each_entry_safe(vma, vma_next, + &obj->vma_list, + vma_link) + if (i915_vma_unbind(vma)) + break; + + /* Stolen pins its pages to prevent the + * normal shrinker from processing stolen + * objects. + */ + i915_gem_object_unpin_pages(obj); + + ret = i915_gem_object_put_pages(obj); + if (ret == 0) { + obj->madv = __I915_MADV_PURGED; + + kfree(obj->stolen); + obj->stolen = NULL; + } else + i915_gem_object_pin_pages(obj); + } + + drm_gem_object_unreference(&obj->base); + } + + if (ret == 0) + ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size, + 4096, DRM_MM_SEARCH_DEFAULT); + if (ret == 0) + return stolen; + + kfree(stolen); + return NULL; +} + +struct drm_i915_gem_object * +i915_gem_object_create_stolen(struct drm_device *dev, u32 size) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_gem_object *obj; + struct drm_mm_node *stolen; + + lockdep_assert_held(&dev->struct_mutex); + + if (!drm_mm_initialized(&dev_priv->mm.stolen)) + return NULL; + + DRM_DEBUG_KMS("creating stolen object: size=%x\n", size); + + stolen = stolen_alloc(dev_priv, size); + if (stolen == NULL) + return NULL; + obj = _i915_gem_object_create_stolen(dev, stolen); if (obj) return obj;