* [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep [not found] <20191119210844.16947-1-daniel.vetter@ffwll.ch> @ 2019-11-19 21:08 ` Daniel Vetter 2019-11-20 11:30 ` Christian König 0 siblings, 1 reply; 3+ messages in thread From: Daniel Vetter @ 2019-11-19 21:08 UTC (permalink / raw) To: Intel Graphics Development, DRI Development Cc: Daniel Vetter, Maarten Lankhorst, Chris Wilson, Christian König, Sumit Semwal, linux-media, linaro-mm-sig, Huang Rui, Eric Anholt, Ben Skeggs, Alex Deucher, Rob Herring, Lucas Stach, Russell King, Christian Gmeiner, Rob Clark, Sean Paul, Daniel Vetter Semnatically it really doesn't matter where we grab the ticket. But since the ticket is a fake lockdep lock, it matters for lockdep validation purposes. This means stuff like grabbing a ticket and then doing copy_from/to_user isn't allowed anymore. This is a changed compared to the current ttm fault handler, which doesn't bother with having a full reservation. Since I'm looking into fixing the TODO entry in ttm_mem_evict_wait_busy() I think that'll have to change sooner or later anyway, better get started. A bit more context on why I'm looking into this: For backwards compat with existing i915 gem code I think we'll have to do full slowpath locking in the i915 equivalent of the eviction code. And with dynamic dma-buf that will leak across drivers, so another thing we need to standardize and make sure it's done the same way everyway. Unfortunately this means another full audit of all drivers: - gem helpers: acquire_init is done right before taking locks, so no problem. Same for acquire_fini and unlocking, which means nothing that's not already covered by the dma_resv_lock rules will be caught with this extension here to the acquire_ctx. - etnaviv: An absolute massive amount of code is run between the acquire_init and the first lock acquisition in submit_lock_objects. But nothing that would touch user memory and could cause a fault. Furthermore nothing that uses the ticket, so even if I missed something, it would be easy to fix by pushing the acquire_init right before the first use. Similar on the unlock/acquire_fini side. - i915: Right now (and this will likely change a lot rsn) the acquire ctx and actual locks are right next to each another. No problem. - msm has a problem: submit_create calls acquire_init, but then submit_lookup_objects() has a bunch of copy_from_user to do the object lookups. That's the only thing before submit_lock_objects call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv does not have this issue since it copies all the userspace structs earlier. submit_cleanup does not have any such issues. With the prep patch to pull out the acquire_ctx and reorder it msm is going to be safe too. - nouveau: acquire_init is right next to ttm_bo_reserve, so all good. Similar on the acquire_fini/ttm_bo_unreserve side. - ttm execbuf utils: acquire context and locking are even in the same functions here (one function to reserve everything, the other to unreserve), so all good. - vc4: Another case where acquire context and locking are handled in the same functions (one function to lock everything, the other to unlock). Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Christian König <christian.koenig@amd.com> Cc: Sumit Semwal <sumit.semwal@linaro.org> Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Cc: Huang Rui <ray.huang@amd.com> Cc: Eric Anholt <eric@anholt.net> Cc: Ben Skeggs <bskeggs@redhat.com> Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Rob Herring <robh@kernel.org> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Russell King <linux+etnaviv@armlinux.org.uk> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> Cc: Rob Clark <robdclark@gmail.com> Cc: Sean Paul <sean@poorly.run> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/dma-buf/dma-resv.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index d3c760e19991..079e38fde33a 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list) static void __init dma_resv_lockdep(void) { struct mm_struct *mm = mm_alloc(); + struct ww_acquire_ctx ctx; struct dma_resv obj; + int ret; if (!mm) return; @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void) dma_resv_init(&obj); down_read(&mm->mmap_sem); - ww_mutex_lock(&obj.lock, NULL); + ww_acquire_init(&ctx, &reservation_ww_class); + ret = dma_resv_lock(&obj, &ctx); + if (ret == -EDEADLK) + dma_resv_lock_slow(&obj, &ctx); fs_reclaim_acquire(GFP_KERNEL); fs_reclaim_release(GFP_KERNEL); ww_mutex_unlock(&obj.lock); + ww_acquire_fini(&ctx); up_read(&mm->mmap_sem); mmput(mm); -- 2.24.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep 2019-11-19 21:08 ` [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep Daniel Vetter @ 2019-11-20 11:30 ` Christian König 2019-11-20 13:56 ` Maarten Lankhorst 0 siblings, 1 reply; 3+ messages in thread From: Christian König @ 2019-11-20 11:30 UTC (permalink / raw) To: Daniel Vetter, Intel Graphics Development, DRI Development Cc: Maarten Lankhorst, Chris Wilson, Sumit Semwal, linux-media, linaro-mm-sig, Huang Rui, Eric Anholt, Ben Skeggs, Alex Deucher, Rob Herring, Lucas Stach, Russell King, Christian Gmeiner, Rob Clark, Sean Paul, Daniel Vetter Am 19.11.19 um 22:08 schrieb Daniel Vetter: > Semnatically it really doesn't matter where we grab the ticket. But > since the ticket is a fake lockdep lock, it matters for lockdep > validation purposes. > > This means stuff like grabbing a ticket and then doing > copy_from/to_user isn't allowed anymore. This is a changed compared to > the current ttm fault handler, which doesn't bother with having a full > reservation. Since I'm looking into fixing the TODO entry in > ttm_mem_evict_wait_busy() I think that'll have to change sooner or > later anyway, better get started. A bit more context on why I'm > looking into this: For backwards compat with existing i915 gem code I > think we'll have to do full slowpath locking in the i915 equivalent of > the eviction code. And with dynamic dma-buf that will leak across > drivers, so another thing we need to standardize and make sure it's > done the same way everyway. > > Unfortunately this means another full audit of all drivers: > > - gem helpers: acquire_init is done right before taking locks, so no > problem. Same for acquire_fini and unlocking, which means nothing > that's not already covered by the dma_resv_lock rules will be caught > with this extension here to the acquire_ctx. > > - etnaviv: An absolute massive amount of code is run between the > acquire_init and the first lock acquisition in submit_lock_objects. > But nothing that would touch user memory and could cause a fault. > Furthermore nothing that uses the ticket, so even if I missed > something, it would be easy to fix by pushing the acquire_init right > before the first use. Similar on the unlock/acquire_fini side. > > - i915: Right now (and this will likely change a lot rsn) the acquire > ctx and actual locks are right next to each another. No problem. > > - msm has a problem: submit_create calls acquire_init, but then > submit_lookup_objects() has a bunch of copy_from_user to do the > object lookups. That's the only thing before submit_lock_objects > call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv > does not have this issue since it copies all the userspace structs > earlier. submit_cleanup does not have any such issues. > > With the prep patch to pull out the acquire_ctx and reorder it msm > is going to be safe too. > > - nouveau: acquire_init is right next to ttm_bo_reserve, so all good. > Similar on the acquire_fini/ttm_bo_unreserve side. > > - ttm execbuf utils: acquire context and locking are even in the same > functions here (one function to reserve everything, the other to > unreserve), so all good. > > - vc4: Another case where acquire context and locking are handled in > the same functions (one function to lock everything, the other to > unlock). > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Christian König <christian.koenig@amd.com> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: linux-media@vger.kernel.org > Cc: linaro-mm-sig@lists.linaro.org > Cc: Huang Rui <ray.huang@amd.com> > Cc: Eric Anholt <eric@anholt.net> > Cc: Ben Skeggs <bskeggs@redhat.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Rob Herring <robh@kernel.org> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Russell King <linux+etnaviv@armlinux.org.uk> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com> > Cc: Rob Clark <robdclark@gmail.com> > Cc: Sean Paul <sean@poorly.run> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Acked-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-resv.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index d3c760e19991..079e38fde33a 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list) > static void __init dma_resv_lockdep(void) > { > struct mm_struct *mm = mm_alloc(); > + struct ww_acquire_ctx ctx; > struct dma_resv obj; > + int ret; > > if (!mm) > return; > @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void) > dma_resv_init(&obj); > > down_read(&mm->mmap_sem); > - ww_mutex_lock(&obj.lock, NULL); > + ww_acquire_init(&ctx, &reservation_ww_class); > + ret = dma_resv_lock(&obj, &ctx); > + if (ret == -EDEADLK) > + dma_resv_lock_slow(&obj, &ctx); > fs_reclaim_acquire(GFP_KERNEL); > fs_reclaim_release(GFP_KERNEL); > ww_mutex_unlock(&obj.lock); > + ww_acquire_fini(&ctx); > up_read(&mm->mmap_sem); > > mmput(mm); ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep 2019-11-20 11:30 ` Christian König @ 2019-11-20 13:56 ` Maarten Lankhorst 0 siblings, 0 replies; 3+ messages in thread From: Maarten Lankhorst @ 2019-11-20 13:56 UTC (permalink / raw) To: Christian König, Daniel Vetter, Intel Graphics Development, DRI Development Cc: Chris Wilson, Sumit Semwal, linux-media, linaro-mm-sig, Huang Rui, Eric Anholt, Ben Skeggs, Alex Deucher, Rob Herring, Lucas Stach, Russell King, Christian Gmeiner, Rob Clark, Sean Paul, Daniel Vetter Op 20-11-2019 om 12:30 schreef Christian König: > Am 19.11.19 um 22:08 schrieb Daniel Vetter: >> Semnatically it really doesn't matter where we grab the ticket. But >> since the ticket is a fake lockdep lock, it matters for lockdep >> validation purposes. >> >> This means stuff like grabbing a ticket and then doing >> copy_from/to_user isn't allowed anymore. This is a changed compared to >> the current ttm fault handler, which doesn't bother with having a full >> reservation. Since I'm looking into fixing the TODO entry in >> ttm_mem_evict_wait_busy() I think that'll have to change sooner or >> later anyway, better get started. A bit more context on why I'm >> looking into this: For backwards compat with existing i915 gem code I >> think we'll have to do full slowpath locking in the i915 equivalent of >> the eviction code. And with dynamic dma-buf that will leak across >> drivers, so another thing we need to standardize and make sure it's >> done the same way everyway. >> >> Unfortunately this means another full audit of all drivers: >> >> - gem helpers: acquire_init is done right before taking locks, so no >> problem. Same for acquire_fini and unlocking, which means nothing >> that's not already covered by the dma_resv_lock rules will be caught >> with this extension here to the acquire_ctx. >> >> - etnaviv: An absolute massive amount of code is run between the >> acquire_init and the first lock acquisition in submit_lock_objects. >> But nothing that would touch user memory and could cause a fault. >> Furthermore nothing that uses the ticket, so even if I missed >> something, it would be easy to fix by pushing the acquire_init right >> before the first use. Similar on the unlock/acquire_fini side. >> >> - i915: Right now (and this will likely change a lot rsn) the acquire >> ctx and actual locks are right next to each another. No problem. >> >> - msm has a problem: submit_create calls acquire_init, but then >> submit_lookup_objects() has a bunch of copy_from_user to do the >> object lookups. That's the only thing before submit_lock_objects >> call dma_resv_lock(). Despite all the copypasta to etnaviv, etnaviv >> does not have this issue since it copies all the userspace structs >> earlier. submit_cleanup does not have any such issues. >> >> With the prep patch to pull out the acquire_ctx and reorder it msm >> is going to be safe too. >> >> - nouveau: acquire_init is right next to ttm_bo_reserve, so all good. >> Similar on the acquire_fini/ttm_bo_unreserve side. >> >> - ttm execbuf utils: acquire context and locking are even in the same >> functions here (one function to reserve everything, the other to >> unreserve), so all good. >> >> - vc4: Another case where acquire context and locking are handled in >> the same functions (one function to lock everything, the other to >> unlock). >> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Christian König <christian.koenig@amd.com> >> Cc: Sumit Semwal <sumit.semwal@linaro.org> >> Cc: linux-media@vger.kernel.org >> Cc: linaro-mm-sig@lists.linaro.org >> Cc: Huang Rui <ray.huang@amd.com> >> Cc: Eric Anholt <eric@anholt.net> >> Cc: Ben Skeggs <bskeggs@redhat.com> >> Cc: Alex Deucher <alexander.deucher@amd.com> >> Cc: Rob Herring <robh@kernel.org> >> Cc: Lucas Stach <l.stach@pengutronix.de> >> Cc: Russell King <linux+etnaviv@armlinux.org.uk> >> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> >> Cc: Rob Clark <robdclark@gmail.com> >> Cc: Sean Paul <sean@poorly.run> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Acked-by: Christian König <christian.koenig@amd.com> > >> --- >> drivers/dma-buf/dma-resv.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c >> index d3c760e19991..079e38fde33a 100644 >> --- a/drivers/dma-buf/dma-resv.c >> +++ b/drivers/dma-buf/dma-resv.c >> @@ -100,7 +100,9 @@ static void dma_resv_list_free(struct dma_resv_list *list) >> static void __init dma_resv_lockdep(void) >> { >> struct mm_struct *mm = mm_alloc(); >> + struct ww_acquire_ctx ctx; >> struct dma_resv obj; >> + int ret; >> if (!mm) >> return; >> @@ -108,10 +110,14 @@ static void __init dma_resv_lockdep(void) >> dma_resv_init(&obj); >> down_read(&mm->mmap_sem); >> - ww_mutex_lock(&obj.lock, NULL); >> + ww_acquire_init(&ctx, &reservation_ww_class); >> + ret = dma_resv_lock(&obj, &ctx); >> + if (ret == -EDEADLK) >> + dma_resv_lock_slow(&obj, &ctx); >> fs_reclaim_acquire(GFP_KERNEL); >> fs_reclaim_release(GFP_KERNEL); >> ww_mutex_unlock(&obj.lock); >> + ww_acquire_fini(&ctx); >> up_read(&mm->mmap_sem); >> >> mmput(mm); > For whole series: Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> typo in patch 3 btw :) ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-20 13:56 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20191119210844.16947-1-daniel.vetter@ffwll.ch> 2019-11-19 21:08 ` [PATCH 2/3] dma-resv: Also prime acquire ctx for lockdep Daniel Vetter 2019-11-20 11:30 ` Christian König 2019-11-20 13:56 ` Maarten Lankhorst
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).