| Message ID | 20230827175449.1766701-18-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 Sun, 27 Aug 2023 20:54:43 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > In a preparation of adding drm-shmem memory shrinker, move all reservation > locking lockdep checks to use new drm_gem_shmem_resv_assert_held() that > will resolve spurious lockdep warning about wrong locking order vs > fs_reclam code paths during freeing of shmem GEM, where lockdep isn't > aware that it's impossible to have locking contention with the fs_reclam > at this special time. > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 37 +++++++++++++++++--------- > 1 file changed, 25 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index d96fee3d6166..ca5da976aafa 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -128,6 +128,23 @@ 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_resv_assert_held(struct drm_gem_shmem_object *shmem) > +{ > + /* > + * Destroying the object is a special case.. drm_gem_shmem_free() > + * calls many things that WARN_ON if the obj lock is not held. But > + * acquiring the obj lock in drm_gem_shmem_free() can cause a locking > + * order inversion between reservation_ww_class_mutex and fs_reclaim. > + * > + * 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, we pretend it is already locked. > + */ > + if (kref_read(&shmem->base.refcount)) > + drm_gem_shmem_resv_assert_held(shmem); > +} > + > /** > * drm_gem_shmem_free - Free resources associated with a shmem GEM object > * @shmem: shmem GEM object to free > @@ -142,8 +159,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > if (obj->import_attach) { > drm_prime_gem_destroy(obj, shmem->sgt); > } else if (!shmem->imported_sgt) { > - dma_resv_lock(shmem->base.resv, NULL); > - > drm_WARN_ON(obj->dev, kref_read(&shmem->vmap_use_count)); > > if (shmem->sgt) { > @@ -156,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > drm_gem_shmem_put_pages_locked(shmem); AFAICT, drm_gem_shmem_put_pages_locked() is the only function that's called in the free path and would complain about resv-lock not being held. I think I'd feel more comfortable if we were adding a drm_gem_shmem_free_pages() function that did everything drm_gem_shmem_put_pages_locked() does except for the lock_held() check and the refcount dec, and have it called here (and in drm_gem_shmem_put_pages_locked()). This way we can keep using dma_resv_assert_held() instead of having our own variant. > > drm_WARN_ON(obj->dev, kref_read(&shmem->pages_use_count)); > - > - dma_resv_unlock(shmem->base.resv); > } > > drm_gem_object_release(obj); > @@ -170,7 +183,7 @@ static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) > struct drm_gem_object *obj = &shmem->base; > struct page **pages; > > - dma_resv_assert_held(shmem->base.resv); > + drm_gem_shmem_resv_assert_held(shmem); > > if (kref_get_unless_zero(&shmem->pages_use_count)) > return 0; > @@ -228,7 +241,7 @@ static void drm_gem_shmem_kref_release_pages(struct kref *kref) > */ > void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) > { > - dma_resv_assert_held(shmem->base.resv); > + drm_gem_shmem_resv_assert_held(shmem); > > kref_put(&shmem->pages_use_count, drm_gem_shmem_kref_release_pages); > } > @@ -252,7 +265,7 @@ static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem) > { > int ret; > > - dma_resv_assert_held(shmem->base.resv); > + drm_gem_shmem_resv_assert_held(shmem); > > if (kref_get_unless_zero(&shmem->pages_pin_count)) > return 0; > @@ -276,7 +289,7 @@ static void drm_gem_shmem_kref_unpin_pages(struct kref *kref) > > static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem) > { > - dma_resv_assert_held(shmem->base.resv); > + drm_gem_shmem_resv_assert_held(shmem); > > kref_put(&shmem->pages_pin_count, drm_gem_shmem_kref_unpin_pages); > } > @@ -357,7 +370,7 @@ int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, > } else { > pgprot_t prot = PAGE_KERNEL; > > - dma_resv_assert_held(shmem->base.resv); > + drm_gem_shmem_resv_assert_held(shmem); > > if (kref_get_unless_zero(&shmem->vmap_use_count)) { > iosys_map_set_vaddr(map, shmem->vaddr); > @@ -426,7 +439,7 @@ void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, > if (obj->import_attach) { > dma_buf_vunmap(obj->import_attach->dmabuf, map); > } else { > - dma_resv_assert_held(shmem->base.resv); > + drm_gem_shmem_resv_assert_held(shmem); > kref_put(&shmem->vmap_use_count, drm_gem_shmem_kref_vunmap); > } > > @@ -462,7 +475,7 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv, > */ > int drm_gem_shmem_madvise_locked(struct drm_gem_shmem_object *shmem, int madv) > { > - dma_resv_assert_held(shmem->base.resv); > + drm_gem_shmem_resv_assert_held(shmem); > > if (shmem->madv >= 0) > shmem->madv = madv; > @@ -478,7 +491,7 @@ void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) > struct drm_gem_object *obj = &shmem->base; > struct drm_device *dev = obj->dev; > > - dma_resv_assert_held(shmem->base.resv); > + drm_gem_shmem_resv_assert_held(shmem); > > drm_WARN_ON(obj->dev, !drm_gem_shmem_is_purgeable(shmem)); >
On 8/28/23 13:12, Boris Brezillon wrote: > On Sun, 27 Aug 2023 20:54:43 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >> In a preparation of adding drm-shmem memory shrinker, move all reservation >> locking lockdep checks to use new drm_gem_shmem_resv_assert_held() that >> will resolve spurious lockdep warning about wrong locking order vs >> fs_reclam code paths during freeing of shmem GEM, where lockdep isn't >> aware that it's impossible to have locking contention with the fs_reclam >> at this special time. >> >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> --- >> drivers/gpu/drm/drm_gem_shmem_helper.c | 37 +++++++++++++++++--------- >> 1 file changed, 25 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >> index d96fee3d6166..ca5da976aafa 100644 >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >> @@ -128,6 +128,23 @@ 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_resv_assert_held(struct drm_gem_shmem_object *shmem) >> +{ >> + /* >> + * Destroying the object is a special case.. drm_gem_shmem_free() >> + * calls many things that WARN_ON if the obj lock is not held. But >> + * acquiring the obj lock in drm_gem_shmem_free() can cause a locking >> + * order inversion between reservation_ww_class_mutex and fs_reclaim. >> + * >> + * 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, we pretend it is already locked. >> + */ >> + if (kref_read(&shmem->base.refcount)) >> + drm_gem_shmem_resv_assert_held(shmem); >> +} >> + >> /** >> * drm_gem_shmem_free - Free resources associated with a shmem GEM object >> * @shmem: shmem GEM object to free >> @@ -142,8 +159,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >> if (obj->import_attach) { >> drm_prime_gem_destroy(obj, shmem->sgt); >> } else if (!shmem->imported_sgt) { >> - dma_resv_lock(shmem->base.resv, NULL); >> - >> drm_WARN_ON(obj->dev, kref_read(&shmem->vmap_use_count)); >> >> if (shmem->sgt) { >> @@ -156,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >> drm_gem_shmem_put_pages_locked(shmem); > > AFAICT, drm_gem_shmem_put_pages_locked() is the only function that's > called in the free path and would complain about resv-lock not being > held. I think I'd feel more comfortable if we were adding a > drm_gem_shmem_free_pages() function that did everything > drm_gem_shmem_put_pages_locked() does except for the lock_held() check > and the refcount dec, and have it called here (and in > drm_gem_shmem_put_pages_locked()). This way we can keep using > dma_resv_assert_held() instead of having our own variant. It's not only drm_gem_shmem_free_pages(), but any drm-shmem function that drivers may use in the GEM's freeing callback. For example, panfrost_gem_free_object() may unpin shmem BO and then do drm_gem_shmem_free().
On Tue, 29 Aug 2023 05:34:23 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 8/28/23 13:12, Boris Brezillon wrote: > > On Sun, 27 Aug 2023 20:54:43 +0300 > > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > > >> In a preparation of adding drm-shmem memory shrinker, move all reservation > >> locking lockdep checks to use new drm_gem_shmem_resv_assert_held() that > >> will resolve spurious lockdep warning about wrong locking order vs > >> fs_reclam code paths during freeing of shmem GEM, where lockdep isn't > >> aware that it's impossible to have locking contention with the fs_reclam > >> at this special time. > >> > >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >> --- > >> drivers/gpu/drm/drm_gem_shmem_helper.c | 37 +++++++++++++++++--------- > >> 1 file changed, 25 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> index d96fee3d6166..ca5da976aafa 100644 > >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> @@ -128,6 +128,23 @@ 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_resv_assert_held(struct drm_gem_shmem_object *shmem) > >> +{ > >> + /* > >> + * Destroying the object is a special case.. drm_gem_shmem_free() > >> + * calls many things that WARN_ON if the obj lock is not held. But > >> + * acquiring the obj lock in drm_gem_shmem_free() can cause a locking > >> + * order inversion between reservation_ww_class_mutex and fs_reclaim. > >> + * > >> + * 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, we pretend it is already locked. > >> + */ > >> + if (kref_read(&shmem->base.refcount)) > >> + drm_gem_shmem_resv_assert_held(shmem); > >> +} > >> + > >> /** > >> * drm_gem_shmem_free - Free resources associated with a shmem GEM object > >> * @shmem: shmem GEM object to free > >> @@ -142,8 +159,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > >> if (obj->import_attach) { > >> drm_prime_gem_destroy(obj, shmem->sgt); > >> } else if (!shmem->imported_sgt) { > >> - dma_resv_lock(shmem->base.resv, NULL); > >> - > >> drm_WARN_ON(obj->dev, kref_read(&shmem->vmap_use_count)); > >> > >> if (shmem->sgt) { > >> @@ -156,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > >> drm_gem_shmem_put_pages_locked(shmem); > > > > AFAICT, drm_gem_shmem_put_pages_locked() is the only function that's > > called in the free path and would complain about resv-lock not being > > held. I think I'd feel more comfortable if we were adding a > > drm_gem_shmem_free_pages() function that did everything > > drm_gem_shmem_put_pages_locked() does except for the lock_held() check > > and the refcount dec, and have it called here (and in > > drm_gem_shmem_put_pages_locked()). This way we can keep using > > dma_resv_assert_held() instead of having our own variant. > > It's not only drm_gem_shmem_free_pages(), but any drm-shmem function > that drivers may use in the GEM's freeing callback. > > For example, panfrost_gem_free_object() may unpin shmem BO and then do > drm_gem_shmem_free(). Is this really a valid use case? If the GEM refcount dropped to zero, we should certainly not have pages_pin_count > 0 (thinking of vmap-ed buffers that might disappear while kernel still has a pointer to the CPU-mapped area). The only reason we have this drm_gem_shmem_put_pages_locked() in drm_gem_shmem_free() is because of this implicit ref hold by the sgt, and IMHO, we should be stricter and check that pages_use_count == 1 when sgt != NULL and pages_use_count == 0 otherwise. I actually think it's a good thing to try and catch any attempt to call functions trying lock the resv in a path they're not supposed to. At least we can decide whether these actions are valid or not in this context, and provide dedicated helpers for the free path if they are.
Am 29.08.23 um 09:29 schrieb Boris Brezillon: > On Tue, 29 Aug 2023 05:34:23 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >> On 8/28/23 13:12, Boris Brezillon wrote: >>> On Sun, 27 Aug 2023 20:54:43 +0300 >>> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: >>> >>>> In a preparation of adding drm-shmem memory shrinker, move all reservation >>>> locking lockdep checks to use new drm_gem_shmem_resv_assert_held() that >>>> will resolve spurious lockdep warning about wrong locking order vs >>>> fs_reclam code paths during freeing of shmem GEM, where lockdep isn't >>>> aware that it's impossible to have locking contention with the fs_reclam >>>> at this special time. >>>> >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> --- >>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 37 +++++++++++++++++--------- >>>> 1 file changed, 25 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >>>> index d96fee3d6166..ca5da976aafa 100644 >>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >>>> @@ -128,6 +128,23 @@ 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_resv_assert_held(struct drm_gem_shmem_object *shmem) >>>> +{ >>>> + /* >>>> + * Destroying the object is a special case.. drm_gem_shmem_free() >>>> + * calls many things that WARN_ON if the obj lock is not held. But >>>> + * acquiring the obj lock in drm_gem_shmem_free() can cause a locking >>>> + * order inversion between reservation_ww_class_mutex and fs_reclaim. >>>> + * >>>> + * 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, we pretend it is already locked. >>>> + */ >>>> + if (kref_read(&shmem->base.refcount)) >>>> + drm_gem_shmem_resv_assert_held(shmem); >>>> +} >>>> + >>>> /** >>>> * drm_gem_shmem_free - Free resources associated with a shmem GEM object >>>> * @shmem: shmem GEM object to free >>>> @@ -142,8 +159,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >>>> if (obj->import_attach) { >>>> drm_prime_gem_destroy(obj, shmem->sgt); >>>> } else if (!shmem->imported_sgt) { >>>> - dma_resv_lock(shmem->base.resv, NULL); >>>> - >>>> drm_WARN_ON(obj->dev, kref_read(&shmem->vmap_use_count)); >>>> >>>> if (shmem->sgt) { >>>> @@ -156,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >>>> drm_gem_shmem_put_pages_locked(shmem); >>> AFAICT, drm_gem_shmem_put_pages_locked() is the only function that's >>> called in the free path and would complain about resv-lock not being >>> held. I think I'd feel more comfortable if we were adding a >>> drm_gem_shmem_free_pages() function that did everything >>> drm_gem_shmem_put_pages_locked() does except for the lock_held() check >>> and the refcount dec, and have it called here (and in >>> drm_gem_shmem_put_pages_locked()). This way we can keep using >>> dma_resv_assert_held() instead of having our own variant. >> It's not only drm_gem_shmem_free_pages(), but any drm-shmem function >> that drivers may use in the GEM's freeing callback. >> >> For example, panfrost_gem_free_object() may unpin shmem BO and then do >> drm_gem_shmem_free(). > Is this really a valid use case? I haven't followed the whole discussion, but I think it isn't a valid use case. That page_use_count is none zero while the GEM object is about to be destroyed can only happen is someone managed to grab a reference to the page without referencing the GEM object. This is turn usually happens when somebody incorrectly walks the CPU page tables and grabs page references where it shouldn't. KMS used to do this and we had already had a discussion that they shouldn't do this. Regards, Christian. > If the GEM refcount dropped to zero, > we should certainly not have pages_pin_count > 0 (thinking of vmap-ed > buffers that might disappear while kernel still has a pointer to the > CPU-mapped area). The only reason we have this > drm_gem_shmem_put_pages_locked() in drm_gem_shmem_free() is because of > this implicit ref hold by the sgt, and IMHO, we should be stricter and > check that pages_use_count == 1 when sgt != NULL and pages_use_count == > 0 otherwise. > > I actually think it's a good thing to try and catch any attempt to call > functions trying lock the resv in a path they're not supposed to. At > least we can decide whether these actions are valid or not in this > context, and provide dedicated helpers for the free path if they are.
On Tue, 29 Aug 2023 10:52:03 +0200 Christian König <christian.koenig@amd.com> wrote: > Am 29.08.23 um 09:29 schrieb Boris Brezillon: > > On Tue, 29 Aug 2023 05:34:23 +0300 > > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > > >> On 8/28/23 13:12, Boris Brezillon wrote: > >>> On Sun, 27 Aug 2023 20:54:43 +0300 > >>> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >>> > >>>> In a preparation of adding drm-shmem memory shrinker, move all reservation > >>>> locking lockdep checks to use new drm_gem_shmem_resv_assert_held() that > >>>> will resolve spurious lockdep warning about wrong locking order vs > >>>> fs_reclam code paths during freeing of shmem GEM, where lockdep isn't > >>>> aware that it's impossible to have locking contention with the fs_reclam > >>>> at this special time. > >>>> > >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >>>> --- > >>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 37 +++++++++++++++++--------- > >>>> 1 file changed, 25 insertions(+), 12 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > >>>> index d96fee3d6166..ca5da976aafa 100644 > >>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > >>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > >>>> @@ -128,6 +128,23 @@ 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_resv_assert_held(struct drm_gem_shmem_object *shmem) > >>>> +{ > >>>> + /* > >>>> + * Destroying the object is a special case.. drm_gem_shmem_free() > >>>> + * calls many things that WARN_ON if the obj lock is not held. But > >>>> + * acquiring the obj lock in drm_gem_shmem_free() can cause a locking > >>>> + * order inversion between reservation_ww_class_mutex and fs_reclaim. > >>>> + * > >>>> + * 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, we pretend it is already locked. > >>>> + */ > >>>> + if (kref_read(&shmem->base.refcount)) > >>>> + drm_gem_shmem_resv_assert_held(shmem); > >>>> +} > >>>> + > >>>> /** > >>>> * drm_gem_shmem_free - Free resources associated with a shmem GEM object > >>>> * @shmem: shmem GEM object to free > >>>> @@ -142,8 +159,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > >>>> if (obj->import_attach) { > >>>> drm_prime_gem_destroy(obj, shmem->sgt); > >>>> } else if (!shmem->imported_sgt) { > >>>> - dma_resv_lock(shmem->base.resv, NULL); > >>>> - > >>>> drm_WARN_ON(obj->dev, kref_read(&shmem->vmap_use_count)); > >>>> > >>>> if (shmem->sgt) { > >>>> @@ -156,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > >>>> drm_gem_shmem_put_pages_locked(shmem); > >>> AFAICT, drm_gem_shmem_put_pages_locked() is the only function that's > >>> called in the free path and would complain about resv-lock not being > >>> held. I think I'd feel more comfortable if we were adding a > >>> drm_gem_shmem_free_pages() function that did everything > >>> drm_gem_shmem_put_pages_locked() does except for the lock_held() check > >>> and the refcount dec, and have it called here (and in > >>> drm_gem_shmem_put_pages_locked()). This way we can keep using > >>> dma_resv_assert_held() instead of having our own variant. > >> It's not only drm_gem_shmem_free_pages(), but any drm-shmem function > >> that drivers may use in the GEM's freeing callback. > >> > >> For example, panfrost_gem_free_object() may unpin shmem BO and then do > >> drm_gem_shmem_free(). > > Is this really a valid use case? > > I haven't followed the whole discussion, but I think it isn't a valid > use case. > > That page_use_count is none zero while the GEM object is about to be > destroyed can only happen is someone managed to grab a reference to the > page without referencing the GEM object. Actually, drm_gem_shmem_object is a bit special (weird?) in this regard. drm_gem_shmem_get_pages_sgt_locked() creates the sgt and takes a pages ref (pages_use_count++). The sgt itself is cached (next call to drm_gem_shmem_get_pages_sgt_locked() will return the existing sgt) but not refcounted, which means it will stay around until the GEM object is destroyed or its pages are purged (GEM eviction). Because of that, shmem->pages_use_count == 1 in drm_gem_shmem_free_pages() is valid iff shmem->sgt != NULL. pages_use_count > 1 is invalid though, as should be pages_pin_count after Dmitry's patches. If we want to 'fix' that (not convinced this is a bug, more a design choice), we need to refcount the sgt users and add drm_gem_shmem_put_pages_sgt[_locked](), so drivers can reflect when they're done using the sgt.
On Tue, 29 Aug 2023 11:44:13 +0200 Boris Brezillon <boris.brezillon@collabora.com> wrote: > On Tue, 29 Aug 2023 10:52:03 +0200 > Christian König <christian.koenig@amd.com> wrote: > > > Am 29.08.23 um 09:29 schrieb Boris Brezillon: > > > On Tue, 29 Aug 2023 05:34:23 +0300 > > > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > > > > >> On 8/28/23 13:12, Boris Brezillon wrote: > > >>> On Sun, 27 Aug 2023 20:54:43 +0300 > > >>> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > >>> > > >>>> In a preparation of adding drm-shmem memory shrinker, move all reservation > > >>>> locking lockdep checks to use new drm_gem_shmem_resv_assert_held() that > > >>>> will resolve spurious lockdep warning about wrong locking order vs > > >>>> fs_reclam code paths during freeing of shmem GEM, where lockdep isn't > > >>>> aware that it's impossible to have locking contention with the fs_reclam > > >>>> at this special time. > > >>>> > > >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > > >>>> --- > > >>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 37 +++++++++++++++++--------- > > >>>> 1 file changed, 25 insertions(+), 12 deletions(-) > > >>>> > > >>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > > >>>> index d96fee3d6166..ca5da976aafa 100644 > > >>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > > >>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > > >>>> @@ -128,6 +128,23 @@ 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_resv_assert_held(struct drm_gem_shmem_object *shmem) > > >>>> +{ > > >>>> + /* > > >>>> + * Destroying the object is a special case.. drm_gem_shmem_free() > > >>>> + * calls many things that WARN_ON if the obj lock is not held. But > > >>>> + * acquiring the obj lock in drm_gem_shmem_free() can cause a locking > > >>>> + * order inversion between reservation_ww_class_mutex and fs_reclaim. > > >>>> + * > > >>>> + * 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, we pretend it is already locked. > > >>>> + */ > > >>>> + if (kref_read(&shmem->base.refcount)) > > >>>> + drm_gem_shmem_resv_assert_held(shmem); > > >>>> +} > > >>>> + > > >>>> /** > > >>>> * drm_gem_shmem_free - Free resources associated with a shmem GEM object > > >>>> * @shmem: shmem GEM object to free > > >>>> @@ -142,8 +159,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > > >>>> if (obj->import_attach) { > > >>>> drm_prime_gem_destroy(obj, shmem->sgt); > > >>>> } else if (!shmem->imported_sgt) { > > >>>> - dma_resv_lock(shmem->base.resv, NULL); > > >>>> - > > >>>> drm_WARN_ON(obj->dev, kref_read(&shmem->vmap_use_count)); > > >>>> > > >>>> if (shmem->sgt) { > > >>>> @@ -156,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > > >>>> drm_gem_shmem_put_pages_locked(shmem); > > >>> AFAICT, drm_gem_shmem_put_pages_locked() is the only function that's > > >>> called in the free path and would complain about resv-lock not being > > >>> held. I think I'd feel more comfortable if we were adding a > > >>> drm_gem_shmem_free_pages() function that did everything > > >>> drm_gem_shmem_put_pages_locked() does except for the lock_held() check > > >>> and the refcount dec, and have it called here (and in > > >>> drm_gem_shmem_put_pages_locked()). This way we can keep using > > >>> dma_resv_assert_held() instead of having our own variant. > > >> It's not only drm_gem_shmem_free_pages(), but any drm-shmem function > > >> that drivers may use in the GEM's freeing callback. > > >> > > >> For example, panfrost_gem_free_object() may unpin shmem BO and then do > > >> drm_gem_shmem_free(). > > > Is this really a valid use case? > > > > I haven't followed the whole discussion, but I think it isn't a valid > > use case. > > > > That page_use_count is none zero while the GEM object is about to be > > destroyed can only happen is someone managed to grab a reference to the > > page without referencing the GEM object. > > Actually, drm_gem_shmem_object is a bit special (weird?) in this regard. > drm_gem_shmem_get_pages_sgt_locked() creates the sgt and takes a > pages ref (pages_use_count++). The sgt itself is cached (next call to > drm_gem_shmem_get_pages_sgt_locked() will return the existing sgt) but > not refcounted, which means it will stay around until the GEM object is > destroyed or its pages are purged (GEM eviction). Because of that, > shmem->pages_use_count == 1 in drm_gem_shmem_free_pages() is valid iff > shmem->sgt != NULL. pages_use_count > 1 is invalid though, as should be > pages_pin_count after Dmitry's patches. > > If we want to 'fix' that (not convinced this is a bug, more a design > choice), we need to refcount the sgt users and add > drm_gem_shmem_put_pages_sgt[_locked](), so drivers can reflect when > they're done using the sgt. Or we simply create the sgt in drm_gem_shmem_get_pages_locked(), and make drm_gem_shmem_get_pages_sgt() a dummy wrapper returning shmem->sgt, which will force callers to explicitly call drm_gem_shmem_{get,pin}_pages[_locked]() if they want a non-NULL sgt. By doing that, we avoid adding yet another level of refcounting and we keep drivers responsible for pages_{use,pin}_count balancing. The only downside would be the unconditional creation of the sg_table, but I suspect all current users of drm_gem_shmem_object want this sg_table anyway.
On 8/29/23 10:29, Boris Brezillon wrote: > On Tue, 29 Aug 2023 05:34:23 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >> On 8/28/23 13:12, Boris Brezillon wrote: >>> On Sun, 27 Aug 2023 20:54:43 +0300 >>> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: >>> >>>> In a preparation of adding drm-shmem memory shrinker, move all reservation >>>> locking lockdep checks to use new drm_gem_shmem_resv_assert_held() that >>>> will resolve spurious lockdep warning about wrong locking order vs >>>> fs_reclam code paths during freeing of shmem GEM, where lockdep isn't >>>> aware that it's impossible to have locking contention with the fs_reclam >>>> at this special time. >>>> >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> --- >>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 37 +++++++++++++++++--------- >>>> 1 file changed, 25 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >>>> index d96fee3d6166..ca5da976aafa 100644 >>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >>>> @@ -128,6 +128,23 @@ 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_resv_assert_held(struct drm_gem_shmem_object *shmem) >>>> +{ >>>> + /* >>>> + * Destroying the object is a special case.. drm_gem_shmem_free() >>>> + * calls many things that WARN_ON if the obj lock is not held. But >>>> + * acquiring the obj lock in drm_gem_shmem_free() can cause a locking >>>> + * order inversion between reservation_ww_class_mutex and fs_reclaim. >>>> + * >>>> + * 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, we pretend it is already locked. >>>> + */ >>>> + if (kref_read(&shmem->base.refcount)) >>>> + drm_gem_shmem_resv_assert_held(shmem); >>>> +} >>>> + >>>> /** >>>> * drm_gem_shmem_free - Free resources associated with a shmem GEM object >>>> * @shmem: shmem GEM object to free >>>> @@ -142,8 +159,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >>>> if (obj->import_attach) { >>>> drm_prime_gem_destroy(obj, shmem->sgt); >>>> } else if (!shmem->imported_sgt) { >>>> - dma_resv_lock(shmem->base.resv, NULL); >>>> - >>>> drm_WARN_ON(obj->dev, kref_read(&shmem->vmap_use_count)); >>>> >>>> if (shmem->sgt) { >>>> @@ -156,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >>>> drm_gem_shmem_put_pages_locked(shmem); >>> >>> AFAICT, drm_gem_shmem_put_pages_locked() is the only function that's >>> called in the free path and would complain about resv-lock not being >>> held. I think I'd feel more comfortable if we were adding a >>> drm_gem_shmem_free_pages() function that did everything >>> drm_gem_shmem_put_pages_locked() does except for the lock_held() check >>> and the refcount dec, and have it called here (and in >>> drm_gem_shmem_put_pages_locked()). This way we can keep using >>> dma_resv_assert_held() instead of having our own variant. >> >> It's not only drm_gem_shmem_free_pages(), but any drm-shmem function >> that drivers may use in the GEM's freeing callback. >> >> For example, panfrost_gem_free_object() may unpin shmem BO and then do >> drm_gem_shmem_free(). > > Is this really a valid use case? If the GEM refcount dropped to zero, > we should certainly not have pages_pin_count > 0 (thinking of vmap-ed > buffers that might disappear while kernel still has a pointer to the > CPU-mapped area). The only reason we have this > drm_gem_shmem_put_pages_locked() in drm_gem_shmem_free() is because of > this implicit ref hold by the sgt, and IMHO, we should be stricter and > check that pages_use_count == 1 when sgt != NULL and pages_use_count == > 0 otherwise. > > I actually think it's a good thing to try and catch any attempt to call > functions trying lock the resv in a path they're not supposed to. At > least we can decide whether these actions are valid or not in this > context, and provide dedicated helpers for the free path if they are. To me it's a valid use-case. I was going to do it for the virtio-gpu driver for a specific BO type that should be permanently pinned in memory. So I made the BO pinned in the virto_gpu's bo_create() and unpinned it from the virtio-gpu's gem->free(), this is a perfectly valid case to me. Though, in the end I switched to another approach that doesn't require to do the pinning in the virtio-gpu driver. For now we can do it as you suggested, to use custom put_pages() in the shmem_free() since neither of drivers need that. Let's try that.
On Sat, 2 Sep 2023 22:43:02 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 8/29/23 10:29, Boris Brezillon wrote: > > On Tue, 29 Aug 2023 05:34:23 +0300 > > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > > >> On 8/28/23 13:12, Boris Brezillon wrote: > >>> On Sun, 27 Aug 2023 20:54:43 +0300 > >>> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >>> > >>>> In a preparation of adding drm-shmem memory shrinker, move all reservation > >>>> locking lockdep checks to use new drm_gem_shmem_resv_assert_held() that > >>>> will resolve spurious lockdep warning about wrong locking order vs > >>>> fs_reclam code paths during freeing of shmem GEM, where lockdep isn't > >>>> aware that it's impossible to have locking contention with the fs_reclam > >>>> at this special time. > >>>> > >>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >>>> --- > >>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 37 +++++++++++++++++--------- > >>>> 1 file changed, 25 insertions(+), 12 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > >>>> index d96fee3d6166..ca5da976aafa 100644 > >>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > >>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > >>>> @@ -128,6 +128,23 @@ 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_resv_assert_held(struct drm_gem_shmem_object *shmem) > >>>> +{ > >>>> + /* > >>>> + * Destroying the object is a special case.. drm_gem_shmem_free() > >>>> + * calls many things that WARN_ON if the obj lock is not held. But > >>>> + * acquiring the obj lock in drm_gem_shmem_free() can cause a locking > >>>> + * order inversion between reservation_ww_class_mutex and fs_reclaim. > >>>> + * > >>>> + * 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, we pretend it is already locked. > >>>> + */ > >>>> + if (kref_read(&shmem->base.refcount)) > >>>> + drm_gem_shmem_resv_assert_held(shmem); > >>>> +} > >>>> + > >>>> /** > >>>> * drm_gem_shmem_free - Free resources associated with a shmem GEM object > >>>> * @shmem: shmem GEM object to free > >>>> @@ -142,8 +159,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > >>>> if (obj->import_attach) { > >>>> drm_prime_gem_destroy(obj, shmem->sgt); > >>>> } else if (!shmem->imported_sgt) { > >>>> - dma_resv_lock(shmem->base.resv, NULL); > >>>> - > >>>> drm_WARN_ON(obj->dev, kref_read(&shmem->vmap_use_count)); > >>>> > >>>> if (shmem->sgt) { > >>>> @@ -156,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > >>>> drm_gem_shmem_put_pages_locked(shmem); > >>> > >>> AFAICT, drm_gem_shmem_put_pages_locked() is the only function that's > >>> called in the free path and would complain about resv-lock not being > >>> held. I think I'd feel more comfortable if we were adding a > >>> drm_gem_shmem_free_pages() function that did everything > >>> drm_gem_shmem_put_pages_locked() does except for the lock_held() check > >>> and the refcount dec, and have it called here (and in > >>> drm_gem_shmem_put_pages_locked()). This way we can keep using > >>> dma_resv_assert_held() instead of having our own variant. > >> > >> It's not only drm_gem_shmem_free_pages(), but any drm-shmem function > >> that drivers may use in the GEM's freeing callback. > >> > >> For example, panfrost_gem_free_object() may unpin shmem BO and then do > >> drm_gem_shmem_free(). > > > > Is this really a valid use case? If the GEM refcount dropped to zero, > > we should certainly not have pages_pin_count > 0 (thinking of vmap-ed > > buffers that might disappear while kernel still has a pointer to the > > CPU-mapped area). The only reason we have this > > drm_gem_shmem_put_pages_locked() in drm_gem_shmem_free() is because of > > this implicit ref hold by the sgt, and IMHO, we should be stricter and > > check that pages_use_count == 1 when sgt != NULL and pages_use_count == > > 0 otherwise. > > > > I actually think it's a good thing to try and catch any attempt to call > > functions trying lock the resv in a path they're not supposed to. At > > least we can decide whether these actions are valid or not in this > > context, and provide dedicated helpers for the free path if they are. > > To me it's a valid use-case. I was going to do it for the virtio-gpu > driver for a specific BO type that should be permanently pinned in > memory. So I made the BO pinned in the virto_gpu's bo_create() and > unpinned it from the virtio-gpu's gem->free(), this is a perfectly valid > case to me. Though, in the end I switched to another approach that > doesn't require to do the pinning in the virtio-gpu driver. Not saying driver-specific gem_create() methods can't own an implicit ref on pages, but not checking that pages_{use,ref}_count <= max_implicit_refs means you leave an opportunity for pages ref leaks to go unnoticed. If your driver has a pin_on_creation flag, it should get a ref in the creation path, and release this ref in the gem_free() path, before calling drm_gem_shmem_free(), so the shmem layer can still make sure there's at most one implicit ref left (the one taken by the sgt creation logic) in drm_gem_shmem_free().
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index d96fee3d6166..ca5da976aafa 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -128,6 +128,23 @@ 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_resv_assert_held(struct drm_gem_shmem_object *shmem) +{ + /* + * Destroying the object is a special case.. drm_gem_shmem_free() + * calls many things that WARN_ON if the obj lock is not held. But + * acquiring the obj lock in drm_gem_shmem_free() can cause a locking + * order inversion between reservation_ww_class_mutex and fs_reclaim. + * + * 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, we pretend it is already locked. + */ + if (kref_read(&shmem->base.refcount)) + drm_gem_shmem_resv_assert_held(shmem); +} + /** * drm_gem_shmem_free - Free resources associated with a shmem GEM object * @shmem: shmem GEM object to free @@ -142,8 +159,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) if (obj->import_attach) { drm_prime_gem_destroy(obj, shmem->sgt); } else if (!shmem->imported_sgt) { - dma_resv_lock(shmem->base.resv, NULL); - drm_WARN_ON(obj->dev, kref_read(&shmem->vmap_use_count)); if (shmem->sgt) { @@ -156,8 +171,6 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) drm_gem_shmem_put_pages_locked(shmem); drm_WARN_ON(obj->dev, kref_read(&shmem->pages_use_count)); - - dma_resv_unlock(shmem->base.resv); } drm_gem_object_release(obj); @@ -170,7 +183,7 @@ static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem) struct drm_gem_object *obj = &shmem->base; struct page **pages; - dma_resv_assert_held(shmem->base.resv); + drm_gem_shmem_resv_assert_held(shmem); if (kref_get_unless_zero(&shmem->pages_use_count)) return 0; @@ -228,7 +241,7 @@ static void drm_gem_shmem_kref_release_pages(struct kref *kref) */ void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem) { - dma_resv_assert_held(shmem->base.resv); + drm_gem_shmem_resv_assert_held(shmem); kref_put(&shmem->pages_use_count, drm_gem_shmem_kref_release_pages); } @@ -252,7 +265,7 @@ static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem) { int ret; - dma_resv_assert_held(shmem->base.resv); + drm_gem_shmem_resv_assert_held(shmem); if (kref_get_unless_zero(&shmem->pages_pin_count)) return 0; @@ -276,7 +289,7 @@ static void drm_gem_shmem_kref_unpin_pages(struct kref *kref) static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem) { - dma_resv_assert_held(shmem->base.resv); + drm_gem_shmem_resv_assert_held(shmem); kref_put(&shmem->pages_pin_count, drm_gem_shmem_kref_unpin_pages); } @@ -357,7 +370,7 @@ int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem, } else { pgprot_t prot = PAGE_KERNEL; - dma_resv_assert_held(shmem->base.resv); + drm_gem_shmem_resv_assert_held(shmem); if (kref_get_unless_zero(&shmem->vmap_use_count)) { iosys_map_set_vaddr(map, shmem->vaddr); @@ -426,7 +439,7 @@ void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem, if (obj->import_attach) { dma_buf_vunmap(obj->import_attach->dmabuf, map); } else { - dma_resv_assert_held(shmem->base.resv); + drm_gem_shmem_resv_assert_held(shmem); kref_put(&shmem->vmap_use_count, drm_gem_shmem_kref_vunmap); } @@ -462,7 +475,7 @@ drm_gem_shmem_create_with_handle(struct drm_file *file_priv, */ int drm_gem_shmem_madvise_locked(struct drm_gem_shmem_object *shmem, int madv) { - dma_resv_assert_held(shmem->base.resv); + drm_gem_shmem_resv_assert_held(shmem); if (shmem->madv >= 0) shmem->madv = madv; @@ -478,7 +491,7 @@ void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem) struct drm_gem_object *obj = &shmem->base; struct drm_device *dev = obj->dev; - dma_resv_assert_held(shmem->base.resv); + drm_gem_shmem_resv_assert_held(shmem); drm_WARN_ON(obj->dev, !drm_gem_shmem_is_purgeable(shmem));
In a preparation of adding drm-shmem memory shrinker, move all reservation locking lockdep checks to use new drm_gem_shmem_resv_assert_held() that will resolve spurious lockdep warning about wrong locking order vs fs_reclam code paths during freeing of shmem GEM, where lockdep isn't aware that it's impossible to have locking contention with the fs_reclam at this special time. Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> --- drivers/gpu/drm/drm_gem_shmem_helper.c | 37 +++++++++++++++++--------- 1 file changed, 25 insertions(+), 12 deletions(-)