Re: [PATCH v13 01/10] drm/shmem-helper: Switch to reservation lock
On 3/26/23 12:19, Christian König wrote: > Am 25.03.23 um 15:58 schrieb Dmitry Osipenko: >> On 3/15/23 16:46, Dmitry Osipenko wrote: >>> On 3/14/23 05:26, Dmitry Osipenko wrote: @@ -633,7 +605,10 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct return ret; } + dma_resv_lock(shmem->base.resv, NULL); ret = drm_gem_shmem_get_pages(shmem); + dma_resv_unlock(shmem->base.resv); >>> Intel CI reported locking problem [1] here. It actually was also >>> reported for v12, but I missed that report because of the other noisy >>> reports. >>> >>> [1] >>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114671v2/shard-snb5/igt@prime_vgem@s...@rcs0.html >>> >>> The test does the following: >>> >>> 1. creates vgem >>> 2. export it do dmabuf >>> 3. mmaps dmabuf >>> >>> There is an obvious deadlock there. The DRM code assumes that mmap() is >>> unlocked, while for dma-buf it's locked. I'll see how to fix it for v14. >>> >> Christian, there is a deadlock problem in drm_gem_shmem_mmap() once we >> move drm-shmem to use resv lock. The current dma-buf locking policy >> claims that importer holds the lock for mmap(), but DRM code assumes >> that obj->mmap() handles the locking itself and then the same >> obj->mmap() code path is used by both dma-buf DRM and a usual DRM object >> paths. Hence importer -> dma_buf_mmap_internal()[takes the lock] -> >> exporter -> drm_gem_shmem_mmap()[takes the lock] deadlocks. >> >> I was looking at how to fix it and to me the best option is to change >> the dma-buf locking policy, making exporter responsible for handling the >> resv lock. Changing DRM code mmap lockings might be possible too [moving >> locking to drm_gem_mmap_obj()], but will be very messy and doesn't feel >> intuitive. >> >> Want to get yours thoughts on this before sending out the dma-buf mmap() >> policy-change patch. Does the new mmap() locking policy sound good to >> you? Thanks! > > > IIRC we tried that before and ran into problems. > > dma_buf_mmap() needs to swap the backing file of the VMA and for this > calls fput() on the old file. > > This fput() in turn could (in theory) grab the resv lock as well and > there isn't anything we could do about that. > > Just information from the back of my memory, probably best if you double > check that. Thanks, Christian! The fput() code path will be unlocked with updated locking policy, like it was before. The new locking policy looks goods on my side, don't see anything that needs locking protection from the importer side for mmap(). I'll send the patches, letting intel-ci test them. Will be also easier to discuss it there with the code. -- Best regards, Dmitry
Re: [PATCH v13 01/10] drm/shmem-helper: Switch to reservation lock
Am 25.03.23 um 15:58 schrieb Dmitry Osipenko: On 3/15/23 16:46, Dmitry Osipenko wrote: On 3/14/23 05:26, Dmitry Osipenko wrote: @@ -633,7 +605,10 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct return ret; } + dma_resv_lock(shmem->base.resv, NULL); ret = drm_gem_shmem_get_pages(shmem); + dma_resv_unlock(shmem->base.resv); Intel CI reported locking problem [1] here. It actually was also reported for v12, but I missed that report because of the other noisy reports. [1] https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114671v2/shard-snb5/igt@prime_vgem@s...@rcs0.html The test does the following: 1. creates vgem 2. export it do dmabuf 3. mmaps dmabuf There is an obvious deadlock there. The DRM code assumes that mmap() is unlocked, while for dma-buf it's locked. I'll see how to fix it for v14. Christian, there is a deadlock problem in drm_gem_shmem_mmap() once we move drm-shmem to use resv lock. The current dma-buf locking policy claims that importer holds the lock for mmap(), but DRM code assumes that obj->mmap() handles the locking itself and then the same obj->mmap() code path is used by both dma-buf DRM and a usual DRM object paths. Hence importer -> dma_buf_mmap_internal()[takes the lock] -> exporter -> drm_gem_shmem_mmap()[takes the lock] deadlocks. I was looking at how to fix it and to me the best option is to change the dma-buf locking policy, making exporter responsible for handling the resv lock. Changing DRM code mmap lockings might be possible too [moving locking to drm_gem_mmap_obj()], but will be very messy and doesn't feel intuitive. Want to get yours thoughts on this before sending out the dma-buf mmap() policy-change patch. Does the new mmap() locking policy sound good to you? Thanks! IIRC we tried that before and ran into problems. dma_buf_mmap() needs to swap the backing file of the VMA and for this calls fput() on the old file. This fput() in turn could (in theory) grab the resv lock as well and there isn't anything we could do about that. Just information from the back of my memory, probably best if you double check that. Regards, Christian.
Re: [PATCH v13 01/10] drm/shmem-helper: Switch to reservation lock
On 3/15/23 16:46, Dmitry Osipenko wrote: > On 3/14/23 05:26, Dmitry Osipenko wrote: >> @@ -633,7 +605,10 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object >> *shmem, struct vm_area_struct >> return ret; >> } >> >> +dma_resv_lock(shmem->base.resv, NULL); >> ret = drm_gem_shmem_get_pages(shmem); >> +dma_resv_unlock(shmem->base.resv); > > Intel CI reported locking problem [1] here. It actually was also > reported for v12, but I missed that report because of the other noisy > reports. > > [1] > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114671v2/shard-snb5/igt@prime_vgem@s...@rcs0.html > > The test does the following: > > 1. creates vgem > 2. export it do dmabuf > 3. mmaps dmabuf > > There is an obvious deadlock there. The DRM code assumes that mmap() is > unlocked, while for dma-buf it's locked. I'll see how to fix it for v14. > Christian, there is a deadlock problem in drm_gem_shmem_mmap() once we move drm-shmem to use resv lock. The current dma-buf locking policy claims that importer holds the lock for mmap(), but DRM code assumes that obj->mmap() handles the locking itself and then the same obj->mmap() code path is used by both dma-buf DRM and a usual DRM object paths. Hence importer -> dma_buf_mmap_internal()[takes the lock] -> exporter -> drm_gem_shmem_mmap()[takes the lock] deadlocks. I was looking at how to fix it and to me the best option is to change the dma-buf locking policy, making exporter responsible for handling the resv lock. Changing DRM code mmap lockings might be possible too [moving locking to drm_gem_mmap_obj()], but will be very messy and doesn't feel intuitive. Want to get yours thoughts on this before sending out the dma-buf mmap() policy-change patch. Does the new mmap() locking policy sound good to you? Thanks! -- Best regards, Dmitry
Re: [PATCH v13 01/10] drm/shmem-helper: Switch to reservation lock
On 3/14/23 05:26, Dmitry Osipenko wrote: > @@ -633,7 +605,10 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object > *shmem, struct vm_area_struct > return ret; > } > > + dma_resv_lock(shmem->base.resv, NULL); > ret = drm_gem_shmem_get_pages(shmem); > + dma_resv_unlock(shmem->base.resv); Intel CI reported locking problem [1] here. It actually was also reported for v12, but I missed that report because of the other noisy reports. [1] https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114671v2/shard-snb5/igt@prime_vgem@s...@rcs0.html The test does the following: 1. creates vgem 2. export it do dmabuf 3. mmaps dmabuf There is an obvious deadlock there. The DRM code assumes that mmap() is unlocked, while for dma-buf it's locked. I'll see how to fix it for v14. -- Best regards, Dmitry
[PATCH v13 01/10] drm/shmem-helper: Switch to reservation lock
Replace all drm-shmem locks with a GEM reservation lock. This makes locks
consistent with dma-buf locking convention where importers are responsible
for holding reservation lock for all operations performed over dma-bufs,
preventing deadlock between dma-buf importers and exporters.
Suggested-by: Daniel Vetter
Acked-by: Thomas Zimmermann
Signed-off-by: Dmitry Osipenko
---
drivers/gpu/drm/drm_gem_shmem_helper.c| 217 --
drivers/gpu/drm/lima/lima_gem.c | 8 +-
drivers/gpu/drm/panfrost/panfrost_drv.c | 7 +-
.../gpu/drm/panfrost/panfrost_gem_shrinker.c | 6 +-
drivers/gpu/drm/panfrost/panfrost_mmu.c | 19 +-
include/drm/drm_gem_shmem_helper.h| 14 +-
6 files changed, 120 insertions(+), 151 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 4ea6507a77e5..8fc2a3277486 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -88,8 +88,6 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size,
bool private)
if (ret)
goto err_release;
- mutex_init(&shmem->pages_lock);
- mutex_init(&shmem->vmap_lock);
INIT_LIST_HEAD(&shmem->madv_list);
if (!private) {
@@ -141,11 +139,13 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
*shmem)
{
struct drm_gem_object *obj = &shmem->base;
- drm_WARN_ON(obj->dev, shmem->vmap_use_count);
-
if (obj->import_attach) {
drm_prime_gem_destroy(obj, shmem->sgt);
} else {
+ dma_resv_lock(shmem->base.resv, NULL);
+
+ drm_WARN_ON(obj->dev, shmem->vmap_use_count);
+
if (shmem->sgt) {
dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
DMA_BIDIRECTIONAL, 0);
@@ -154,18 +154,18 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
*shmem)
}
if (shmem->pages)
drm_gem_shmem_put_pages(shmem);
- }
- drm_WARN_ON(obj->dev, shmem->pages_use_count);
+ drm_WARN_ON(obj->dev, shmem->pages_use_count);
+
+ dma_resv_unlock(shmem->base.resv);
+ }
drm_gem_object_release(obj);
- mutex_destroy(&shmem->pages_lock);
- mutex_destroy(&shmem->vmap_lock);
kfree(shmem);
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
-static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
+static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
{
struct drm_gem_object *obj = &shmem->base;
struct page **pages;
@@ -197,35 +197,16 @@ static int drm_gem_shmem_get_pages_locked(struct
drm_gem_shmem_object *shmem)
}
/*
- * drm_gem_shmem_get_pages - Allocate backing pages for a shmem GEM object
+ * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a
shmem GEM object
* @shmem: shmem GEM object
*
- * This function makes sure that backing pages exists for the shmem GEM object
- * and increases the use count.
- *
- * Returns:
- * 0 on success or a negative error code on failure.
+ * This function decreases the use count and puts the backing pages when use
drops to zero.
*/
-int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
+void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
{
struct drm_gem_object *obj = &shmem->base;
- int ret;
- drm_WARN_ON(obj->dev, obj->import_attach);
-
- ret = mutex_lock_interruptible(&shmem->pages_lock);
- if (ret)
- return ret;
- ret = drm_gem_shmem_get_pages_locked(shmem);
- mutex_unlock(&shmem->pages_lock);
-
- return ret;
-}
-EXPORT_SYMBOL(drm_gem_shmem_get_pages);
-
-static void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem)
-{
- struct drm_gem_object *obj = &shmem->base;
+ dma_resv_assert_held(shmem->base.resv);
if (drm_WARN_ON_ONCE(obj->dev, !shmem->pages_use_count))
return;
@@ -243,20 +224,32 @@ static void drm_gem_shmem_put_pages_locked(struct
drm_gem_shmem_object *shmem)
shmem->pages_mark_accessed_on_put);
shmem->pages = NULL;
}
+EXPORT_SYMBOL(drm_gem_shmem_put_pages);
-/*
- * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a
shmem GEM object
- * @shmem: shmem GEM object
- *
- * This function decreases the use count and puts the backing pages when use
drops to zero.
- */
-void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
+static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem)
{
- mutex_lock(&shmem->pages_lock);
- drm_gem_shmem_put_pages_locked(shmem);
- mutex_unlock(&shmem->pages_lock);
+ struct drm_gem_object *obj = &shmem->base;
+ int ret;
+
+ dma_resv_assert_held(shmem->base.resv);
+
+ drm_WARN_ON(obj->dev, obj->import_
