Re: [PATCH 3/3] drm/msm/shrinker: Only iterate dontneed objs

[Jordan Crouse]

On Sat, Nov 14, 2020 at 11:30:10AM -0800, Rob Clark wrote:
> From: Rob Clark <robdclark@xxxxxxxxxxxx>
> 
> 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 <robdclark@xxxxxxxxxxxx>
> ---
>  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-devel@xxxxxxxxxxxxxxxxxxxxx
> 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