On 07/08/2018 14:21, Daniel Vetter wrote:
On Wed, Jul 18, 2018 at 03:24:26PM +0200, Christian König wrote:
Hi Daniel,
Am 18.07.2018 um 14:07 schrieb Patchwork:
== Series Details ==
Series: series starting with [1/4] dma-buf: add caching of sg_table
URL : https://patchwork.freedesktop.org/series/46778/
State : failure
[SNIP]
it looks like I'm a step further understanding the problems which come with
this change.
I've more or less audited all use cases and think that only i915 is left
with the following lock inversion:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9705/fi-cfl-8700k/igt@gem_mmap_...@basic-small-bo-tiledx.html
Now my question is what is &obj->mm.lock used for and why do you guys call
dma_buf_map_attachment() while holding it?
obj->mm.lock is the lock to protect getting at the backing storage.
i915_gem_object_get_pages and _put_pages are the relevant functions.
Existing paths want to pin the backing storage while holding the
reservation lock. And your new path needs to do the inverse since
dma_buf_map_attachment now also requires the reservation lock. And that is
obviously called from within the dma-buf importer version of get_pages.
I think there's 2 solutions:
- Merge obj->mm.lock and the reservation lock. Probably the cleaner
solution, but likely more work.
- Make sure the obj->mm.lock always nests within the reservation lock, and
grab the reservation lock anywhere it's not yet grabbed. Then you can
use the dma_buf_map_attachment_locked variant in
i915_gem_object_get_pages_dmabuf to avoid the locking inversion. This
would essentially make the obj->mm.lock fully redundant.
Either way is going to be quite a bit of work. I expect that you need to
replace all the cases of dma_buf_map_attachment in i915 with
dma_buf_map_attachment_locked, and adjust the entire callchain to the new
locking scheme.
The real trouble here imo is that i915 CI is just the canary, I expect a
bunch of other drivers will also look at an inverted locking hierarchy if
dma_buf_map_attachment needs the reservation lock. And there's no
convenient CI for them, and code audit won't cut it really (at least I'm
too stupid to keep the locking hierarchy of an entire driver in my head).
We chatted about this on #intel-gfx and concluded that either solution
derives to replacing the obj->mm.lock with the reservation lock. And
that is problematic for i915, both from the reason of a general
direction towards more fine-grained locking, and also issue that
reservation lock needs to be avoided under the shrinker path (we lock
obj->mm.lock when dropping the backing store there).
I proposed that maybe we could re-jig how we use obj->mm.lock a bit, to
ensure backing store vfunc (get_pages) is not called under it (although
I haven't thought it fully through it may be possible without too
significant drawbacks), but Chris also has some patches which may work
around this in a different way. So I'll wait to see those first.
On whether or not reservation lock is the right lock to use from dma-buf
for this purpose I'll leave other guys to comment - I am not fully into
the details of dma-buf design.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
