| Message ID | 20231029230205.93277-12-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 Mon, 30 Oct 2023 02:01:50 +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 | 35 +++++++++++++------------- > 1 file changed, 18 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 08b5a57c59d8..24ff2b99e75b 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -128,6 +128,22 @@ 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_free_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; > +} > + > /** > * drm_gem_shmem_free - Free resources associated with a shmem GEM object > * @shmem: shmem GEM object to free > @@ -142,8 +158,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) { > @@ -157,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > If you drop the dma_resv_lock/unlock(), you should also replace the drm_gem_shmem_put_pages_locked() by a drm_gem_shmem_free_pages() in this commit. > drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)); > drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count)); > - > - dma_resv_unlock(shmem->base.resv); > } > > drm_gem_object_release(obj); > @@ -208,21 +220,10 @@ 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; > - } > + if (refcount_dec_and_test(&shmem->pages_use_count)) > + drm_gem_shmem_free_pages(shmem); > } > EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked); >
On 11/10/23 13:16, Boris Brezillon wrote: > On Mon, 30 Oct 2023 02:01:50 +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 | 35 +++++++++++++------------- >> 1 file changed, 18 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >> index 08b5a57c59d8..24ff2b99e75b 100644 >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >> @@ -128,6 +128,22 @@ 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_free_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; >> +} >> + >> /** >> * drm_gem_shmem_free - Free resources associated with a shmem GEM object >> * @shmem: shmem GEM object to free >> @@ -142,8 +158,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) { >> @@ -157,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >> > If you drop the dma_resv_lock/unlock(), you should also replace the > drm_gem_shmem_put_pages_locked() by a drm_gem_shmem_free_pages() in this > commit. drm_gem_shmem_put_pages_locked() is exported by a later patch of this series, it's not worthwhile to remove this function
On Mon, 20 Nov 2023 14:02:29 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 11/10/23 13:16, Boris Brezillon wrote: > > On Mon, 30 Oct 2023 02:01:50 +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 | 35 +++++++++++++------------- > >> 1 file changed, 18 insertions(+), 17 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> index 08b5a57c59d8..24ff2b99e75b 100644 > >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> @@ -128,6 +128,22 @@ 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_free_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; > >> +} > >> + > >> /** > >> * drm_gem_shmem_free - Free resources associated with a shmem GEM object > >> * @shmem: shmem GEM object to free > >> @@ -142,8 +158,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) { > >> @@ -157,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > >> > > If you drop the dma_resv_lock/unlock(), you should also replace the > > drm_gem_shmem_put_pages_locked() by a drm_gem_shmem_free_pages() in this > > commit. > > drm_gem_shmem_put_pages_locked() is exported by a later patch of this > series, it's not worthwhile to remove this function I'm not talking about removing drm_gem_shmem_put_pages_locked(), but replacing the drm_gem_shmem_put_pages_locked() call you have in drm_gem_shmem_free() by a drm_gem_shmem_free_pages(), so you don't end up with a lockdep warning when you stop exactly here in the patch series, which is important if we want to keep things bisectable.
On 11/20/23 14:19, Boris Brezillon wrote: ... >>>> - dma_resv_lock(shmem->base.resv, NULL); >>>> - >>>> drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)); >>>> >>>> if (shmem->sgt) { >>>> @@ -157,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >>>> >>> If you drop the dma_resv_lock/unlock(), you should also replace the >>> drm_gem_shmem_put_pages_locked() by a drm_gem_shmem_free_pages() in this >>> commit. >> >> drm_gem_shmem_put_pages_locked() is exported by a later patch of this >> series, it's not worthwhile to remove this function > > I'm not talking about removing drm_gem_shmem_put_pages_locked(), but > replacing the drm_gem_shmem_put_pages_locked() call you have in > drm_gem_shmem_free() by a drm_gem_shmem_free_pages(), so you don't end > up with a lockdep warning when you stop exactly here in the patch > series, which is important if we want to keep things bisectable. Indeed, there is assert_locked() there. Thanks for the clarification :)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 08b5a57c59d8..24ff2b99e75b 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -128,6 +128,22 @@ 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_free_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; +} + /** * drm_gem_shmem_free - Free resources associated with a shmem GEM object * @shmem: shmem GEM object to free @@ -142,8 +158,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) { @@ -157,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)); drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count)); - - dma_resv_unlock(shmem->base.resv); } drm_gem_object_release(obj); @@ -208,21 +220,10 @@ 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; - } + if (refcount_dec_and_test(&shmem->pages_use_count)) + drm_gem_shmem_free_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 | 35 +++++++++++++------------- 1 file changed, 18 insertions(+), 17 deletions(-)