| Message ID | 20230914232721.408581-13-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers | expand |
On Fri, 15 Sep 2023 02:27:15 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > Prepare drm_gem_shmem_free() to addition of memory shrinker support > to drm-shmem by adding and using variant of put_pages() that doesn't > touch reservation lock. Reservation shouldn't be touched because lockdep > will trigger a bogus warning about locking contention with fs_reclaim > code paths that can't happen during the time when GEM is freed and > lockdep doesn't know about that. > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 55 +++++++++++++++++--------- > 1 file changed, 37 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 8a8eab4d0332..4959f51b647a 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -128,6 +128,41 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t > } > EXPORT_SYMBOL_GPL(drm_gem_shmem_create); > > +static void > +__drm_gem_shmem_release_pages(struct drm_gem_shmem_object *shmem) Could we find more descriptive names to replace those __ prefixes? drm_gem_shmem_free_pages(), drm_gem_shmem_drop_pages()? > +{ > + struct drm_gem_object *obj = &shmem->base; > + > +#ifdef CONFIG_X86 > + if (shmem->map_wc) > + set_pages_array_wb(shmem->pages, obj->size >> PAGE_SHIFT); > +#endif > + > + drm_gem_put_pages(obj, shmem->pages, > + shmem->pages_mark_dirty_on_put, > + shmem->pages_mark_accessed_on_put); > + shmem->pages = NULL; > +} > + > +static void > +__drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem) Maybe drm_gem_shmem_put_pages_no_lock_check()? But honestly, I'm not sure we want to make it a function since it's only going to be needed in drm_gem_shmem_free(). I think we can just inline if (refcount_dec_and_test(&shmem->pages_use_count)) __drm_gem_shmem_release_pages(shmem); there. > +{ > + /* > + * Destroying the object is a special case. Acquiring the obj > + * lock in drm_gem_shmem_put_pages_locked() can cause a locking > + * order inversion between reservation_ww_class_mutex and fs_reclaim > + * when called from drm_gem_shmem_free(). > + * > + * This deadlock is not actually possible, because no one should > + * be already holding the lock when drm_gem_shmem_free() is called. > + * Unfortunately lockdep is not aware of this detail. So when the > + * refcount drops to zero, make sure that the reservation lock > + * isn't touched here. > + */ > + if (refcount_dec_and_test(&shmem->pages_use_count)) > + __drm_gem_shmem_release_pages(shmem); > +} > + > /** > * drm_gem_shmem_free - Free resources associated with a shmem GEM object > * @shmem: shmem GEM object to free > @@ -142,8 +177,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > if (obj->import_attach) { > drm_prime_gem_destroy(obj, shmem->sgt); > } else { > - dma_resv_lock(shmem->base.resv, NULL); > - > drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)); > > if (shmem->sgt) { > @@ -153,11 +186,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > kfree(shmem->sgt); > } > if (shmem->pages) > - drm_gem_shmem_put_pages_locked(shmem); > + __drm_gem_shmem_put_pages(shmem); > > drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)); > - > - dma_resv_unlock(shmem->base.resv); > } > > drm_gem_object_release(obj); > @@ -207,21 +238,9 @@ static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) > */ > 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 (refcount_dec_and_test(&shmem->pages_use_count)) { > -#ifdef CONFIG_X86 > - if (shmem->map_wc) > - set_pages_array_wb(shmem->pages, obj->size >> PAGE_SHIFT); > -#endif > - > - drm_gem_put_pages(obj, shmem->pages, > - shmem->pages_mark_dirty_on_put, > - shmem->pages_mark_accessed_on_put); > - shmem->pages = NULL; > - } > + __drm_gem_shmem_put_pages(shmem); > } > EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked); >
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 8a8eab4d0332..4959f51b647a 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -128,6 +128,41 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t } EXPORT_SYMBOL_GPL(drm_gem_shmem_create); +static void +__drm_gem_shmem_release_pages(struct drm_gem_shmem_object *shmem) +{ + struct drm_gem_object *obj = &shmem->base; + +#ifdef CONFIG_X86 + if (shmem->map_wc) + set_pages_array_wb(shmem->pages, obj->size >> PAGE_SHIFT); +#endif + + drm_gem_put_pages(obj, shmem->pages, + shmem->pages_mark_dirty_on_put, + shmem->pages_mark_accessed_on_put); + shmem->pages = NULL; +} + +static void +__drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem) +{ + /* + * Destroying the object is a special case. Acquiring the obj + * lock in drm_gem_shmem_put_pages_locked() can cause a locking + * order inversion between reservation_ww_class_mutex and fs_reclaim + * when called from drm_gem_shmem_free(). + * + * This deadlock is not actually possible, because no one should + * be already holding the lock when drm_gem_shmem_free() is called. + * Unfortunately lockdep is not aware of this detail. So when the + * refcount drops to zero, make sure that the reservation lock + * isn't touched here. + */ + if (refcount_dec_and_test(&shmem->pages_use_count)) + __drm_gem_shmem_release_pages(shmem); +} + /** * drm_gem_shmem_free - Free resources associated with a shmem GEM object * @shmem: shmem GEM object to free @@ -142,8 +177,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) if (obj->import_attach) { drm_prime_gem_destroy(obj, shmem->sgt); } else { - dma_resv_lock(shmem->base.resv, NULL); - drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)); if (shmem->sgt) { @@ -153,11 +186,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) kfree(shmem->sgt); } if (shmem->pages) - drm_gem_shmem_put_pages_locked(shmem); + __drm_gem_shmem_put_pages(shmem); drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)); - - dma_resv_unlock(shmem->base.resv); } drm_gem_object_release(obj); @@ -207,21 +238,9 @@ static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) */ 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 (refcount_dec_and_test(&shmem->pages_use_count)) { -#ifdef CONFIG_X86 - if (shmem->map_wc) - set_pages_array_wb(shmem->pages, obj->size >> PAGE_SHIFT); -#endif - - drm_gem_put_pages(obj, shmem->pages, - shmem->pages_mark_dirty_on_put, - shmem->pages_mark_accessed_on_put); - shmem->pages = NULL; - } + __drm_gem_shmem_put_pages(shmem); } EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked);
Prepare drm_gem_shmem_free() to addition of memory shrinker support to drm-shmem by adding and using variant of put_pages() that doesn't touch reservation lock. Reservation shouldn't be touched because lockdep will trigger a bogus warning about locking contention with fs_reclaim code paths that can't happen during the time when GEM is freed and lockdep doesn't know about that. Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> --- drivers/gpu/drm/drm_gem_shmem_helper.c | 55 +++++++++++++++++--------- 1 file changed, 37 insertions(+), 18 deletions(-)