On Sat, Nov 14, 2020 at 11:30:10AM -0800, Rob Clark wrote:
> From: Rob Clark <robdcl...@chromium.org>
>
> In situations where the GPU is mostly idle, all or nearly all buffer
> objects will be in the inactive list. But if the system is under memory
> pressure (from something other than GPU), we could still get a lot of
> shrinker calls. Which results in traversing a list of thousands of objs
> and in the end finding nothing to shrink. Which isn't so efficient.
>
> Instead split the inactive_list into two lists, one inactive objs which
> are shrinkable, and a second one for those that are not. This way we
> can avoid traversing objs which we know are not shrinker candidates.
>
> Signed-off-by: Rob Clark <robdcl...@chromium.org>
> ---
> drivers/gpu/drm/msm/msm_debugfs.c | 3 ++-
> drivers/gpu/drm/msm/msm_drv.c | 3 ++-
> drivers/gpu/drm/msm/msm_drv.h | 8 +++---
> drivers/gpu/drm/msm/msm_gem.c | 34 ++++++++++++++++++++------
> drivers/gpu/drm/msm/msm_gem_shrinker.c | 7 +++---
> 5 files changed, 40 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_debugfs.c
> b/drivers/gpu/drm/msm/msm_debugfs.c
> index 64afbed89821..85ad0babc326 100644
> --- a/drivers/gpu/drm/msm/msm_debugfs.c
> +++ b/drivers/gpu/drm/msm/msm_debugfs.c
> @@ -124,7 +124,8 @@ static int msm_gem_show(struct drm_device *dev, struct
> seq_file *m)
> }
>
> seq_printf(m, "Inactive Objects:\n");
> - msm_gem_describe_objects(&priv->inactive_list, m);
> + msm_gem_describe_objects(&priv->inactive_dontneed, m);
> + msm_gem_describe_objects(&priv->inactive_willneed, m);
>
> mutex_unlock(&priv->mm_lock);
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 4d808769e6ed..39a54f364aa8 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -465,7 +465,8 @@ static int msm_drm_init(struct device *dev, struct
> drm_driver *drv)
>
> priv->wq = alloc_ordered_workqueue("msm", 0);
>
> - INIT_LIST_HEAD(&priv->inactive_list);
> + INIT_LIST_HEAD(&priv->inactive_willneed);
> + INIT_LIST_HEAD(&priv->inactive_dontneed);
> mutex_init(&priv->mm_lock);
>
> /* Teach lockdep about lock ordering wrt. shrinker: */
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index f869ed67b5da..ed18c5bed10f 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -175,8 +175,9 @@ struct msm_drm_private {
> struct msm_perf_state *perf;
>
> /*
> - * List of inactive GEM objects. Every bo is either in the
> inactive_list
> - * or gpu->active_list (for the gpu it is active on[1])
> + * Lists of inactive GEM objects. Every bo is either in one of the
> + * inactive lists (depending on whether or not it is shrinkable) or
> + * gpu->active_list (for the gpu it is active on[1])
> *
> * These lists are protected by mm_lock. If struct_mutex is involved,
> it
> * should be aquired prior to mm_lock. One should *not* hold mm_lock in
> @@ -185,7 +186,8 @@ struct msm_drm_private {
> * [1] if someone ever added support for the old 2d cores, there could
> be
> * more than one gpu object
> */
> - struct list_head inactive_list;
> + struct list_head inactive_willneed; /* inactive + !shrinkable */
> + struct list_head inactive_dontneed; /* inactive + shrinkable */
> struct mutex mm_lock;
>
> struct workqueue_struct *wq;
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 2795288b0a95..de8d2cfada24 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -17,6 +17,7 @@
> #include "msm_gpu.h"
> #include "msm_mmu.h"
>
> +static void update_inactive(struct msm_gem_object *msm_obj);
>
> static dma_addr_t physaddr(struct drm_gem_object *obj)
> {
> @@ -678,6 +679,12 @@ int msm_gem_madvise(struct drm_gem_object *obj, unsigned
> madv)
>
> madv = msm_obj->madv;
>
> + /* If the obj is inactive, we might need to move it
> + * between inactive lists
> + */
> + if (msm_obj->active_count == 0)
> + update_inactive(msm_obj);
> +
> msm_gem_unlock(obj);
>
> return (madv != __MSM_MADV_PURGED);
> @@ -781,19 +788,31 @@ void msm_gem_active_get(struct drm_gem_object *obj,
> struct msm_gpu *gpu)
> void msm_gem_active_put(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> - struct msm_drm_private *priv = obj->dev->dev_private;
>
> might_sleep();
> WARN_ON(!msm_gem_is_locked(obj));
>
> if (--msm_obj->active_count == 0) {
> - mutex_lock(&priv->mm_lock);
> - list_del_init(&msm_obj->mm_list);
> - list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> - mutex_unlock(&priv->mm_lock);
> + update_inactive(msm_obj);
> }
> }
>
> +static void update_inactive(struct msm_gem_object *msm_obj)
> +{
> + struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> +
> + mutex_lock(&priv->mm_lock);
> + WARN_ON(msm_obj->active_count != 0);
> +
> + list_del_init(&msm_obj->mm_list);
> + if (msm_obj->madv == MSM_MADV_DONTNEED)
> + list_add_tail(&msm_obj->mm_list, &priv->inactive_willneed);
> + else
> + list_add_tail(&msm_obj->mm_list, &priv->inactive_dontneed);
Is the logic here inverted or is this just really confusing nomenclature? If it
is correct a comment might help remind us whats happening.
Jordan
> +
> + mutex_unlock(&priv->mm_lock);
> +}
> +
> int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t op, ktime_t
> *timeout)
> {
> bool write = !!(op & MSM_PREP_WRITE);
> @@ -1099,7 +1118,8 @@ static struct drm_gem_object *_msm_gem_new(struct
> drm_device *dev,
> }
>
> mutex_lock(&priv->mm_lock);
> - list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> + /* Initially obj is idle, obj->madv == WILLNEED: */
> + list_add_tail(&msm_obj->mm_list, &priv->inactive_willneed);
> mutex_unlock(&priv->mm_lock);
>
> return obj;
> @@ -1169,7 +1189,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device
> *dev,
> msm_gem_unlock(obj);
>
> mutex_lock(&priv->mm_lock);
> - list_add_tail(&msm_obj->mm_list, &priv->inactive_list);
> + list_add_tail(&msm_obj->mm_list, &priv->inactive_willneed);
> mutex_unlock(&priv->mm_lock);
>
> return obj;
> diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> index 9d51c1eb808d..81dfa57b6a0d 100644
> --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> @@ -19,7 +19,7 @@ msm_gem_shrinker_count(struct shrinker *shrinker, struct
> shrink_control *sc)
>
> mutex_lock(&priv->mm_lock);
>
> - list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) {
> + list_for_each_entry(msm_obj, &priv->inactive_dontneed, mm_list) {
> if (!msm_gem_trylock(&msm_obj->base))
> continue;
> if (is_purgeable(msm_obj))
> @@ -42,7 +42,7 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct
> shrink_control *sc)
>
> mutex_lock(&priv->mm_lock);
>
> - list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) {
> + list_for_each_entry(msm_obj, &priv->inactive_dontneed, mm_list) {
> if (freed >= sc->nr_to_scan)
> break;
> if (!msm_gem_trylock(&msm_obj->base))
> @@ -96,7 +96,8 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned
> long event, void *ptr)
> struct msm_drm_private *priv =
> container_of(nb, struct msm_drm_private, vmap_notifier);
> struct list_head *mm_lists[] = {
> - &priv->inactive_list,
> + &priv->inactive_dontneed,
> + &priv->inactive_willneed,
> priv->gpu ? &priv->gpu->active_list : NULL,
> NULL,
> };
> --
> 2.28.0
>
> _______________________________________________
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
