On 07/23/2014 06:15 PM, Chris Wilson wrote:
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.
Sounds good, just saying really to remind you to post a respin. :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
