On Wed, Jul 23, 2014 at 05:39:49PM +0100, Tvrtko Ursulin wrote:
> On 07/21/2014 01:21 PM, Chris Wilson wrote:
> >+ mn = i915_mmu_notifier_get(obj->userptr.mm);
> >+ if (IS_ERR(mn))
> >+ return PTR_ERR(mn);
>
> Very minor, but I would perhaps consider renaming this to _find
> since _get in my mind strongly associates with reference counting
> and this does not do that. Especially if the reviewer looks at the
> bail out below and sees no matching put. But minor as I said, you
> can judge what you prefer.
The same. It was _get because it did used to a reference counter, now
that counting has been removed from the i915_mmu_notifier.
> >+static int
> >+i915_gem_userptr_init__mm_struct(struct drm_i915_gem_object *obj)
> >+{
> >+ struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> >+ struct i915_mm_struct *mm;
> >+ struct mm_struct *real;
> >+ int ret = 0;
> >+
> >+ real = get_task_mm(current);
> >+ if (real == NULL)
> >+ return -EINVAL;
>
> Do you think we need get_task_mm()/mmput() here, given it is all
> inside a single system call?
No. I kept using get_task_mm() simply because it looked neater than
current->mm, but current->mm looks like it gives simpler code.
> >+ /* During release of the GEM object we hold the struct_mutex. As the
> >+ * object may be holding onto the last reference for the task->mm,
> >+ * calling mmput() may trigger exit_mmap() which close the vma
> >+ * which will call drm_gem_vm_close() and attempt to reacquire
> >+ * the struct_mutex. In order to avoid that recursion, we have
> >+ * to defer the mmput() until after we drop the struct_mutex,
> >+ * i.e. we need to schedule a worker to do the clean up.
> >+ */
>
> This comment reads like a strange mixture and past and present eg.
> what used to be the case and what is the fix. We don't hold a
> reference to the process mm as the address space (terminology OK?).
> We do hold a reference to the mm struct itself - which is enough to
> unregister the notifiers, correct?
True. I was more or less trying to explain the bug and that comment
ended up being the changelog entry. It doesn't work well as a comment.
+ /* During release of the GEM object we hold the struct_mutex. This
+ * precludes us from calling mmput() at that time as that may be
+ * the last reference and so call exit_mmap(). exit_mmap() will
+ * attempt to reap the vma, and if we were holding a GTT mmap
+ * would then call drm_gem_vm_close() and attempt to reacquire
+ * the struct mutex. So in order to avoid that recursion, we have
+ * to defer releasing the mm reference until after we drop the
+ * struct_mutex, i.e. we need to schedule a worker to do the clean
+ * up.
*/
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
