On 4/19/22 10:22, Thomas Zimmermann wrote:
Hi
Am 18.04.22 um 00:37 schrieb Dmitry Osipenko:
Introduce a common DRM SHMEM shrinker. It allows to reduce code duplication among DRM drivers that implement theirs own shrinkers. This is initial version of the shrinker that covers basic needs of GPU drivers, both purging and eviction of shmem objects are supported.
This patch is based on a couple ideas borrowed from Rob's Clark MSM shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
In order to start using DRM SHMEM shrinker drivers should:
- Implement new purge(), evict() + swap_in() GEM callbacks.
- Register shrinker using drm_gem_shmem_shrinker_register(drm_device).
- Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API
functions to activate shrinking of GEMs.
Signed-off-by: Daniel Almeida daniel.almeida@collabora.com Signed-off-by: Dmitry Osipenko dmitry.osipenko@collabora.com
drivers/gpu/drm/drm_gem_shmem_helper.c | 765 ++++++++++++++++++++++++- include/drm/drm_device.h | 4 + include/drm/drm_gem.h | 35 ++ include/drm/drm_gem_shmem_helper.h | 105 +++- 4 files changed, 877 insertions(+), 32 deletions(-)
...
@@ -172,6 +172,41 @@ struct drm_gem_object_funcs { * This is optional but necessary for mmap support. */ const struct vm_operations_struct *vm_ops;
+ /** + * @purge: + * + * Releases the GEM object's allocated backing storage to the system. + * + * Returns the number of pages that have been freed by purging the GEM object. + * + * This callback is used by the GEM shrinker. + */ + unsigned long (*purge)(struct drm_gem_object *obj);
+ /** + * @evict: + * + * Unpins the GEM object's allocated backing storage, allowing shmem pages + * to be swapped out.
What's the difference to the existing unpin() callback?
Drivers need to do more than just unpinning pages when GEMs are evicted. Unpinning is only a part of the eviction process. I'll improve the doc-comment in v5.
For example, for VirtIO-GPU driver we need to to detach host from the guest's memory before pages are evicted [1].
[1] https://gitlab.collabora.com/dmitry.osipenko/linux-kernel-rd/-/blob/932eb031...
In case of Panfrost driver, we will need to remove mappings before pages are evicted.
+ * + * Returns the number of pages that have been unpinned. + * + * This callback is used by the GEM shrinker. + */ + unsigned long (*evict)(struct drm_gem_object *obj);
+ /** + * @swap_in: + * + * Pins GEM object's allocated backing storage if it was previously evicted, + * moving swapped out pages back to memory. + * + * Returns 0 on success, or -errno on error. + * + * This callback is used by the GEM shrinker. + */ + int (*swap_in)(struct drm_gem_object *obj);
Why do you need swap_in()? This can be done on-demand as part of a pin or vmap operation.
Similarly to the unpinning, the pining of pages is only a part of what needs to be done for GPU drivers. Besides of returning pages back to memory, we also need to make them accessible to GPU and this is a driver-specific process. This why we need the additional callbacks.
}; /** diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h index 70889533962a..a65557b446e6 100644 --- a/include/drm/drm_gem_shmem_helper.h +++ b/include/drm/drm_gem_shmem_helper.h @@ -6,6 +6,7 @@ #include <linux/fs.h> #include <linux/mm.h> #include <linux/mutex.h> +#include <linux/shrinker.h> #include <drm/drm_file.h> #include <drm/drm_gem.h> @@ -15,8 +16,18 @@ struct dma_buf_attachment; struct drm_mode_create_dumb; struct drm_printer; +struct drm_device; struct sg_table; +enum drm_gem_shmem_pages_state { + DRM_GEM_SHMEM_PAGES_STATE_PURGED = -2, + DRM_GEM_SHMEM_PAGES_STATE_EVICTED = -1, + DRM_GEM_SHMEM_PAGES_STATE_UNPINNED = 0, + DRM_GEM_SHMEM_PAGES_STATE_PINNED = 1, + DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE = 2, + DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE = 3, +};
These states can be detected by looking at the vmap and pin refcounts. No need to store them explicitly.
I'll try to revisit this, but I was finding that it's much more difficult to follow and debug code without the explicit states.
In your patch, they also come with a big zoo of trivial helpers. None of that seems necessary AFAICT.
There are couple functions which could be squashed, although this may hurt readability of the code a tad. I'll try to take another look at this for v5.
What's the difference between purge and evict BTW?
The evicted pages are moved out from memory to a SWAP partition or file.
The purged pages are destroyed permanently.
/** * struct drm_gem_shmem_object - GEM object backed by shmem */ @@ -43,8 +54,8 @@ struct drm_gem_shmem_object { * @madv: State for madvise * * 0 is active/inuse. + * 1 is not-needed/can-be-purged * A negative value is the object is purged. - * Positive values are driver specific and not used by the helpers. */ int madv; @@ -91,6 +102,40 @@ struct drm_gem_shmem_object { * @map_wc: map object write-combined (instead of using shmem defaults). */ bool map_wc;
+ /** + * @eviction_disable_count: + * + * The shmem pages are disallowed to be evicted by the memory shrinker + * while count is non-zero. Used internally by memory shrinker. + */ + unsigned int eviction_disable_count;
+ /** + * @purging_disable_count: + * + * The shmem pages are disallowed to be purged by the memory shrinker + * while count is non-zero. Used internally by memory shrinker. + */ + unsigned int purging_disable_count;
+ /** + * @pages_state: Current state of shmem pages. Used internally by + * memory shrinker. + */ + enum drm_gem_shmem_pages_state pages_state;
+ /** + * @evicted: True if shmem pages were evicted by the memory shrinker. + * Used internally by memory shrinker. + */ + bool evicted;
+ /** + * @pages_shrinkable: True if shmem pages can be evicted or purged + * by the memory shrinker. Used internally by memory shrinker. + */ + bool pages_shrinkable;
As commented before, this state can be foundby looking at existing fields. No need to store it separately.
When we're transitioning from "evictable" to a "purgeable" state, we must not add pages twice to the "shrinkable_count" variable. Hence this is not a state, but a variable which prevents the double accounting of the pages. Please see drm_gem_shmem_add_pages_to_shrinker() in this patch.
Perhaps something like "pages_accounted_by_shrinker" could be a better name for the variable. I'll revisit this for v5.