On Wed, Jul 01, 2015 at 12:14:34PM +0100, Tvrtko Ursulin wrote:
> > static void
> >+__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj,
> >+ bool value)
> >+{
> >+ /* During mm_invalidate_range we need to cancel any userptr that
> >+ * overlaps the range being invalidated. Doing so requires the
> >+ * struct_mutex, and that risks recursion. In order to cause
> >+ * recursion, the user must alias the userptr address space with
> >+ * a GTT mmapping (possible with a MAP_FIXED) - then when we have
> >+ * to invalidate that mmaping, mm_invalidate_range is called with
> >+ * the userptr address *and* the struct_mutex held. To prevent that
> >+ * we set a flag under the i915_mmu_notifier spinlock to indicate
> >+ * whether this object is valid.
> >+ */
> >+#if defined(CONFIG_MMU_NOTIFIER)
> >+ if (obj->userptr.mmu_object == NULL)
> >+ return;
> >+
> >+ spin_lock(&obj->userptr.mmu_object->mn->lock);
> >+ obj->userptr.mmu_object->active = value;
> >+ spin_unlock(&obj->userptr.mmu_object->mn->lock);
> >+#endif
>
> Would this be more obvious as atomic_t? Since I suspect spinlock is
> just for the memory barrier, if that.
Yes, you could probably get away with a little thought and just a memory
barrier. But since one side is guarded by the spinlock, I think it is
easier to reuse that. Especially when the expression becomes a little
more complicated.
> Hm.. What if we get invalidate
> while get_pages is running, before it set active but after gup has
> succeeded?
Yeah, that actually is a probably. Especially thinking about the
gup_worker which we need to cancel.
Back to setting active earlier and clearing it along error paths.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html