[Intel-gfx] [PATCH] drm/i915/selftests: Let other struct_mutex users have their gpu time
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Jul 3 09:27:47 UTC 2018
On 03/07/2018 09:30, Chris Wilson wrote:
> live_gtt is a very slow test to run, simply because it tries to allocate
> and use as much as the 48b address space as possibly can and in the
> process will try to own all of the system memory. This leads to resource
> exhaustion and CPU starvation; the latter impacts us when the NMI
> watchdog declares a task hung due to a mutex contention with ourselves.
> This we can prevent by releasing the struct_mutex around cond_resched(),
> to allow e.g. the i915_gem_free_objects_work to acquire the mutex and
> cleanup.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=107094
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 58 ++++++++++++++-----
> 1 file changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> index a28ee0cc6a63..789add200720 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> @@ -32,6 +32,14 @@
> #include "mock_drm.h"
> #include "mock_gem_device.h"
>
> +static void schedule_locked(struct drm_i915_private *i915)
> +{
> + /* Let anyone else who wants the context take it */
> + mutex_unlock(&i915->drm.struct_mutex);
> + cond_resched();
> + mutex_lock(&i915->drm.struct_mutex);
> +}
> +
> static void fake_free_pages(struct drm_i915_gem_object *obj,
> struct sg_table *pages)
> {
> @@ -132,18 +140,18 @@ fake_dma_object(struct drm_i915_private *i915, u64 size)
>
> static int igt_ppgtt_alloc(void *arg)
> {
> - struct drm_i915_private *dev_priv = arg;
> + struct drm_i915_private *i915 = arg;
Adding some flavour to reviewers lives! :I
> struct i915_hw_ppgtt *ppgtt;
> u64 size, last;
> int err = 0;
>
> /* Allocate a ppggt and try to fill the entire range */
>
> - if (!USES_PPGTT(dev_priv))
> + if (!USES_PPGTT(i915))
> return 0;
>
> - mutex_lock(&dev_priv->drm.struct_mutex);
> - ppgtt = __hw_ppgtt_create(dev_priv);
> + mutex_lock(&i915->drm.struct_mutex);
> + ppgtt = __hw_ppgtt_create(i915);
> if (IS_ERR(ppgtt)) {
> err = PTR_ERR(ppgtt);
> goto err_unlock;
> @@ -169,6 +177,8 @@ static int igt_ppgtt_alloc(void *arg)
> ppgtt->vm.clear_range(&ppgtt->vm, 0, size);
> }
>
> + schedule_locked(i915);
> +
Is it needed in this test? I glanced over and couldn't spot anything
struct mutexy in page table only manipulations it does.
> /* Check we can incrementally allocate the entire range */
> for (last = 0, size = 4096;
> size <= ppgtt->vm.total;
> @@ -189,7 +199,7 @@ static int igt_ppgtt_alloc(void *arg)
> ppgtt->vm.cleanup(&ppgtt->vm);
> kfree(ppgtt);
> err_unlock:
> - mutex_unlock(&dev_priv->drm.struct_mutex);
> + mutex_unlock(&i915->drm.struct_mutex);
> return err;
> }
>
> @@ -291,6 +301,8 @@ static int lowlevel_hole(struct drm_i915_private *i915,
> i915_gem_object_put(obj);
>
> kfree(order);
> +
> + schedule_locked(i915);
> }
>
> return 0;
> @@ -519,6 +531,7 @@ static int fill_hole(struct drm_i915_private *i915,
> }
>
> close_object_list(&objects, vm);
> + schedule_locked(i915);
> }
>
> return 0;
> @@ -605,6 +618,8 @@ static int walk_hole(struct drm_i915_private *i915,
> i915_gem_object_put(obj);
> if (err)
> return err;
> +
> + schedule_locked(i915);
> }
>
> return 0;
> @@ -676,6 +691,8 @@ static int pot_hole(struct drm_i915_private *i915,
> err = -EINTR;
> goto err;
> }
> +
> + schedule_locked(i915);
> }
>
> err:
> @@ -789,6 +806,8 @@ static int drunk_hole(struct drm_i915_private *i915,
> kfree(order);
> if (err)
> return err;
> +
> + schedule_locked(i915);
> }
>
> return 0;
> @@ -854,6 +873,8 @@ static int __shrink_hole(struct drm_i915_private *i915,
> err = -EINTR;
> break;
> }
> +
> + schedule_locked(i915);
> }
>
> close_object_list(&objects, vm);
> @@ -949,6 +970,7 @@ static int shrink_boom(struct drm_i915_private *i915,
> i915_gem_object_put(explode);
>
> memset(&vm->fault_attr, 0, sizeof(vm->fault_attr));
> + schedule_locked(i915);
> }
>
> return 0;
> @@ -961,7 +983,7 @@ static int shrink_boom(struct drm_i915_private *i915,
> return err;
> }
>
> -static int exercise_ppgtt(struct drm_i915_private *dev_priv,
> +static int exercise_ppgtt(struct drm_i915_private *i915,
> int (*func)(struct drm_i915_private *i915,
> struct i915_address_space *vm,
> u64 hole_start, u64 hole_end,
> @@ -972,15 +994,15 @@ static int exercise_ppgtt(struct drm_i915_private *dev_priv,
> IGT_TIMEOUT(end_time);
> int err;
>
> - if (!USES_FULL_PPGTT(dev_priv))
> + if (!USES_FULL_PPGTT(i915))
> return 0;
>
> - file = mock_file(dev_priv);
> + file = mock_file(i915);
> if (IS_ERR(file))
> return PTR_ERR(file);
>
> - mutex_lock(&dev_priv->drm.struct_mutex);
> - ppgtt = i915_ppgtt_create(dev_priv, file->driver_priv, "mock");
> + mutex_lock(&i915->drm.struct_mutex);
> + ppgtt = i915_ppgtt_create(i915, file->driver_priv, "mock");
> if (IS_ERR(ppgtt)) {
> err = PTR_ERR(ppgtt);
> goto out_unlock;
> @@ -988,14 +1010,14 @@ static int exercise_ppgtt(struct drm_i915_private *dev_priv,
> GEM_BUG_ON(offset_in_page(ppgtt->vm.total));
> GEM_BUG_ON(ppgtt->vm.closed);
>
> - err = func(dev_priv, &ppgtt->vm, 0, ppgtt->vm.total, end_time);
> + err = func(i915, &ppgtt->vm, 0, ppgtt->vm.total, end_time);
How about a less hacky solution where func is called unlocked it is
responsible to take struct_mutex across the parts which need it?
Regards,
Tvrtko
>
> i915_ppgtt_close(&ppgtt->vm);
> i915_ppgtt_put(ppgtt);
> out_unlock:
> - mutex_unlock(&dev_priv->drm.struct_mutex);
> + mutex_unlock(&i915->drm.struct_mutex);
>
> - mock_file_free(dev_priv, file);
> + mock_file_free(i915, file);
> return err;
> }
>
> @@ -1315,6 +1337,8 @@ static int igt_gtt_reserve(void *arg)
> }
> }
>
> + schedule_locked(i915);
> +
> /* Now we start forcing evictions */
> for (total = I915_GTT_PAGE_SIZE;
> total + 2*I915_GTT_PAGE_SIZE <= i915->ggtt.vm.total;
> @@ -1364,6 +1388,8 @@ static int igt_gtt_reserve(void *arg)
> }
> }
>
> + schedule_locked(i915);
> +
> /* And then try at random */
> list_for_each_entry_safe(obj, on, &objects, st_link) {
> struct i915_vma *vma;
> @@ -1517,6 +1543,8 @@ static int igt_gtt_insert(void *arg)
> GEM_BUG_ON(!drm_mm_node_allocated(&vma->node));
> }
>
> + schedule_locked(i915);
> +
> list_for_each_entry(obj, &objects, st_link) {
> struct i915_vma *vma;
>
> @@ -1535,6 +1563,8 @@ static int igt_gtt_insert(void *arg)
> __i915_vma_unpin(vma);
> }
>
> + schedule_locked(i915);
> +
> /* If we then reinsert, we should find the same hole */
> list_for_each_entry_safe(obj, on, &objects, st_link) {
> struct i915_vma *vma;
> @@ -1575,6 +1605,8 @@ static int igt_gtt_insert(void *arg)
> }
> }
>
> + schedule_locked(i915);
> +
> /* And then force evictions */
> for (total = 0;
> total + 2*I915_GTT_PAGE_SIZE <= i915->ggtt.vm.total;
>
More information about the Intel-gfx
mailing list