Re: [PATCH v8 4/7] drm/shmem-helper: Add memory shrinker
Hello Thomas,
On 11/9/22 13:28, Thomas Zimmermann wrote:
>> +int drm_gem_shmem_set_evictable(struct drm_gem_shmem_object *shmem)
>> +{
>> + dma_resv_lock(shmem->base.resv, NULL);
>> +
>> + if (shmem->madv < 0) {
>> + dma_resv_unlock(shmem->base.resv);
>> + return -ENOMEM;
>
> ENOMEM is not right here. It's for failed memory allocation. ENODEV
> seems more appropriate.
Had the same thought about ENOMEM and at one point was considering
ENOENT, but in the end decided it's not much better than ENOMEM.
> But why do we need an error here anyway? Why not just fail transparently?
I added the error handling everywhere for consistency. Perhaps indeed
will be better to fail transparently for now since nobody cares about
such errors and likely won't in the future.
The rest of the comments are also good to me, will start preparing the
v9. Thank you for the review!
--
Best regards,
Dmitry
Re: [PATCH v8 4/7] drm/shmem-helper: Add memory shrinker
Hi
Am 06.11.22 um 00:27 schrieb Dmitry Osipenko:
Introduce common drm-shmem shrinker for DRM drivers.
To start using drm-shmem shrinker drivers should do the following:
1. Implement evict() callback of shmem object where driver should check
whether object is purgeable or evictable and perform shrinking action
2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device)
I've long wanted to add an init call for GEM SHMEM helpers; mostly for
shrinking. So this could be called drmm_gem_shmem_init() and do all
kinds of initialization. Cleanup should be managed. It can still be
optional, so you don't have to go through all the SHMEM drivers.
It should also take the something like
struct drm_gem_shmem_funcs {
evict()
};
that contains the evict callback, as there's no point in having a
per-object evict callback. The struct can then be declared static const
within the driver.
Alternatively, and that might be preferable in the long term, the evict
hook could go into struct drm_gem_object_funcs and we could converge the
various memory managers onto the interface.
3. Use drm_gem_shmem_set_purgeable(shmem) and alike API functions to
activate shrinking of shmem GEMs
The shrinking code will allow us to aviod changes to the page tables
during vunmap. That is important for many driver's performance. So the
long-term goal should be to active all this automatically and filter out
the cases where eviction/purging is not wanted.
Signed-off-by: Daniel Almeida
Signed-off-by: Dmitry Osipenko
---
drivers/gpu/drm/drm_gem_shmem_helper.c| 493 --
.../gpu/drm/panfrost/panfrost_gem_shrinker.c | 9 +-
include/drm/drm_device.h | 4 +
include/drm/drm_gem_shmem_helper.h| 87 +++-
4 files changed, 539 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index d6e62f228989..b09d620d14fd 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -126,6 +126,54 @@ 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 msm_gem_free_object() 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(>base.refcount))
+ dma_resv_assert_held(shmem->base.resv);
+}
+
+static bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem)
+{
+ return (shmem->madv >= 0) && shmem->evict &&
+ shmem->eviction_enabled && shmem->pages_use_count &&
+ !shmem->pages_pin_count && !shmem->base.dma_buf &&
+ !shmem->base.import_attach && shmem->sgt && !shmem->evicted;
+}
+
+static void
+drm_gem_shmem_update_pages_state(struct drm_gem_shmem_object *shmem)
+{
+ struct drm_gem_object *obj = >base;
+ struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
+
+ drm_gem_shmem_resv_assert_held(shmem);
+
+ if (!gem_shrinker || obj->import_attach)
+ return;
+
+ if (drm_gem_shmem_is_evictable(shmem) ||
drm_gem_shmem_is_purgeable(shmem))
+ drm_gem_lru_move_tail(_shrinker->lru_evictable,
>base);
+ else if (shmem->madv < 0)
+ drm_gem_lru_remove(>base);
+ else if (shmem->evicted)
+ drm_gem_lru_move_tail(_shrinker->lru_evicted, >base);
+ else if (!shmem->pages)
+ drm_gem_lru_remove(>base);
+ else
+ drm_gem_lru_move_tail(_shrinker->lru_pinned, >base);
+}
+
/**
* drm_gem_shmem_free - Free resources associated with a shmem GEM object
* @shmem: shmem GEM object to free
@@ -140,7 +188,8 @@ 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);
+ /* take out shmem GEM object from the memory shrinker */
+ drm_gem_shmem_madvise(shmem, -1);
WARN_ON(shmem->vmap_use_count);
@@ -150,12 +199,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
sg_free_table(shmem->sgt);
kfree(shmem->sgt);
}
- if (shmem->pages)
+
[PATCH v8 4/7] drm/shmem-helper: Add memory shrinker
Introduce common drm-shmem shrinker for DRM drivers.
To start using drm-shmem shrinker drivers should do the following:
1. Implement evict() callback of shmem object where driver should check
whether object is purgeable or evictable and perform shrinking action
2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device)
3. Use drm_gem_shmem_set_purgeable(shmem) and alike API functions to
activate shrinking of shmem GEMs
Signed-off-by: Daniel Almeida
Signed-off-by: Dmitry Osipenko
---
drivers/gpu/drm/drm_gem_shmem_helper.c| 493 --
.../gpu/drm/panfrost/panfrost_gem_shrinker.c | 9 +-
include/drm/drm_device.h | 4 +
include/drm/drm_gem_shmem_helper.h| 87 +++-
4 files changed, 539 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index d6e62f228989..b09d620d14fd 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -126,6 +126,54 @@ 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 msm_gem_free_object() 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(>base.refcount))
+ dma_resv_assert_held(shmem->base.resv);
+}
+
+static bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem)
+{
+ return (shmem->madv >= 0) && shmem->evict &&
+ shmem->eviction_enabled && shmem->pages_use_count &&
+ !shmem->pages_pin_count && !shmem->base.dma_buf &&
+ !shmem->base.import_attach && shmem->sgt && !shmem->evicted;
+}
+
+static void
+drm_gem_shmem_update_pages_state(struct drm_gem_shmem_object *shmem)
+{
+ struct drm_gem_object *obj = >base;
+ struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
+
+ drm_gem_shmem_resv_assert_held(shmem);
+
+ if (!gem_shrinker || obj->import_attach)
+ return;
+
+ if (drm_gem_shmem_is_evictable(shmem) ||
drm_gem_shmem_is_purgeable(shmem))
+ drm_gem_lru_move_tail(_shrinker->lru_evictable,
>base);
+ else if (shmem->madv < 0)
+ drm_gem_lru_remove(>base);
+ else if (shmem->evicted)
+ drm_gem_lru_move_tail(_shrinker->lru_evicted, >base);
+ else if (!shmem->pages)
+ drm_gem_lru_remove(>base);
+ else
+ drm_gem_lru_move_tail(_shrinker->lru_pinned, >base);
+}
+
/**
* drm_gem_shmem_free - Free resources associated with a shmem GEM object
* @shmem: shmem GEM object to free
@@ -140,7 +188,8 @@ 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);
+ /* take out shmem GEM object from the memory shrinker */
+ drm_gem_shmem_madvise(shmem, -1);
WARN_ON(shmem->vmap_use_count);
@@ -150,12 +199,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
*shmem)
sg_free_table(shmem->sgt);
kfree(shmem->sgt);
}
- if (shmem->pages)
+ if (shmem->pages_use_count)
drm_gem_shmem_put_pages(shmem);
WARN_ON(shmem->pages_use_count);
-
- dma_resv_unlock(shmem->base.resv);
}
drm_gem_object_release(obj);
@@ -163,18 +210,86 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
*shmem)
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
-static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
+/**
+ * drm_gem_shmem_set_evictable() - Make GEM evictable by memory shrinker
+ * @shmem: shmem GEM object
+ *
+ * Tell memory shrinker that this GEM can be evicted. Initially eviction is
+ * disabled for all GEMs. If GEM was purged, then -ENOMEM is returned.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_gem_shmem_set_evictable(struct drm_gem_shmem_object *shmem)
+{
+ dma_resv_lock(shmem->base.resv, NULL);
+
+ if (shmem->madv < 0) {
+ dma_resv_unlock(shmem->base.resv);
+ return -ENOMEM;
+ }
+
+
