* [PATCH 0/3] drm/msm: Shrinker fixes and opts
@ 2020-11-14 19:30 Rob Clark
2020-11-14 19:30 ` [PATCH 1/3] drm/msm: Protect obj->active_count under obj lock Rob Clark
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Rob Clark @ 2020-11-14 19:30 UTC (permalink / raw)
To: dri-devel
Cc: Rob Clark, open list:DRM DRIVER FOR MSM ADRENO GPU, Jordan Crouse,
Kristian H. Kristensen, open list:DRM DRIVER FOR MSM ADRENO GPU,
open list
From: Rob Clark <robdclark@chromium.org>
The last patch is the main thing, motivated by some cases where we would
spend a lot of time in msm_gem_shrinker_count(). First two are fixes I
noticed along the way.
Rob Clark (3):
drm/msm: Protect obj->active_count under obj lock
drm/msm/shrinker: We can vmap shrink active_list too
drm/msm/shrinker: Only iterate dontneed objs
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 | 45 ++++++++++++++++------
drivers/gpu/drm/msm/msm_gem.h | 5 ++-
drivers/gpu/drm/msm/msm_gem_shrinker.c | 52 +++++++++++++++++++-------
drivers/gpu/drm/msm/msm_gpu.c | 10 +++--
7 files changed, 89 insertions(+), 37 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/3] drm/msm: Protect obj->active_count under obj lock 2020-11-14 19:30 [PATCH 0/3] drm/msm: Shrinker fixes and opts Rob Clark @ 2020-11-14 19:30 ` Rob Clark 2020-11-14 19:30 ` [PATCH 2/3] drm/msm/shrinker: We can vmap shrink active_list too Rob Clark 2020-11-14 19:30 ` [PATCH 3/3] drm/msm/shrinker: Only iterate dontneed objs Rob Clark 2 siblings, 0 replies; 6+ messages in thread From: Rob Clark @ 2020-11-14 19:30 UTC (permalink / raw) To: dri-devel Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter, Kristian H. Kristensen, Jordan Crouse, open list:DRM DRIVER FOR MSM ADRENO GPU, open list:DRM DRIVER FOR MSM ADRENO GPU, open list From: Rob Clark <robdclark@chromium.org> Previously we only held obj lock in the _active_get() path, and relied on atomic_dec_return() to not be racy in the _active_put() path where obj lock was not held. But this is a false sense of security. Unlike obj lifetime refcnt, where you do not expect to *increase* the refcnt after the last put (which would mean that something has gone horribly wrong with the object liveness reference counting), the active_count can increase again from zero. Racing _active_put()s and _active_get()s could leave the obj on the wrong mm list. But in the retire path, immediately after the _active_put(), the _unpin_iova() would acquire obj lock. So just move the locking earlier and rely on that to protect obj->active_count. Fixes: c5c1643cef7a ("drm/msm: Drop struct_mutex from the retire path") Signed-off-by: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/msm/msm_gem.c | 11 ++++++----- drivers/gpu/drm/msm/msm_gem.h | 5 +++-- drivers/gpu/drm/msm/msm_gpu.c | 10 ++++++---- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index a9a834bb7794..2795288b0a95 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -770,7 +770,7 @@ void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu) WARN_ON(!msm_gem_is_locked(obj)); WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED); - if (!atomic_fetch_inc(&msm_obj->active_count)) { + 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, &gpu->active_list); @@ -784,8 +784,9 @@ void msm_gem_active_put(struct drm_gem_object *obj) struct msm_drm_private *priv = obj->dev->dev_private; might_sleep(); + WARN_ON(!msm_gem_is_locked(obj)); - if (!atomic_dec_return(&msm_obj->active_count)) { + 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); @@ -936,15 +937,15 @@ void msm_gem_free_object(struct drm_gem_object *obj) struct drm_device *dev = obj->dev; struct msm_drm_private *priv = dev->dev_private; - /* object should not be on active list: */ - WARN_ON(is_active(msm_obj)); - mutex_lock(&priv->mm_lock); list_del(&msm_obj->mm_list); mutex_unlock(&priv->mm_lock); msm_gem_lock(obj); + /* object should not be on active list: */ + WARN_ON(is_active(msm_obj)); + put_iova(obj); if (obj->import_attach) { diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index d79e7019cc88..3355a48a023b 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -87,7 +87,7 @@ struct msm_gem_object { char name[32]; /* Identifier to print for the debugfs files */ - atomic_t active_count; + int active_count; }; #define to_msm_bo(x) container_of(x, struct msm_gem_object, base) @@ -185,7 +185,8 @@ msm_gem_is_locked(struct drm_gem_object *obj) static inline bool is_active(struct msm_gem_object *msm_obj) { - return atomic_read(&msm_obj->active_count); + WARN_ON(!msm_gem_is_locked(&msm_obj->base)); + return msm_obj->active_count; } static inline bool is_purgeable(struct msm_gem_object *msm_obj) diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index b597213a1890..04f7ab4d63ae 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -717,11 +717,13 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring, stats->alwayson_start, stats->alwayson_end); for (i = 0; i < submit->nr_bos; i++) { - struct msm_gem_object *msm_obj = submit->bos[i].obj; + struct drm_gem_object *obj = &submit->bos[i].obj->base; - msm_gem_active_put(&msm_obj->base); - msm_gem_unpin_iova(&msm_obj->base, submit->aspace); - drm_gem_object_put(&msm_obj->base); + msm_gem_lock(obj); + msm_gem_active_put(obj); + msm_gem_unpin_iova_locked(obj, submit->aspace); + msm_gem_unlock(obj); + drm_gem_object_put(obj); } pm_runtime_mark_last_busy(&gpu->pdev->dev); -- 2.28.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] drm/msm/shrinker: We can vmap shrink active_list too 2020-11-14 19:30 [PATCH 0/3] drm/msm: Shrinker fixes and opts Rob Clark 2020-11-14 19:30 ` [PATCH 1/3] drm/msm: Protect obj->active_count under obj lock Rob Clark @ 2020-11-14 19:30 ` Rob Clark 2020-11-14 19:30 ` [PATCH 3/3] drm/msm/shrinker: Only iterate dontneed objs Rob Clark 2 siblings, 0 replies; 6+ messages in thread From: Rob Clark @ 2020-11-14 19:30 UTC (permalink / raw) To: dri-devel Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter, open list:DRM DRIVER FOR MSM ADRENO GPU, open list:DRM DRIVER FOR MSM ADRENO GPU, open list From: Rob Clark <robdclark@chromium.org> Just because a obj is active, if the vmap_count is zero, we can still tear down the vmap. Signed-off-by: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/msm/msm_gem_shrinker.c | 47 +++++++++++++++++++------- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c index 6f4b1355725f..9d51c1eb808d 100644 --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c @@ -6,6 +6,7 @@ #include "msm_drv.h" #include "msm_gem.h" +#include "msm_gpu.h" #include "msm_gpu_trace.h" static unsigned long @@ -61,17 +62,19 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) return freed; } -static int -msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr) +/* since we don't know any better, lets bail after a few + * and if necessary the shrinker will be invoked again. + * Seems better than unmapping *everything* + */ +static const int vmap_shrink_limit = 15; + +static unsigned +vmap_shrink(struct list_head *mm_list) { - struct msm_drm_private *priv = - container_of(nb, struct msm_drm_private, vmap_notifier); struct msm_gem_object *msm_obj; unsigned unmapped = 0; - mutex_lock(&priv->mm_lock); - - list_for_each_entry(msm_obj, &priv->inactive_list, mm_list) { + list_for_each_entry(msm_obj, mm_list, mm_list) { if (!msm_gem_trylock(&msm_obj->base)) continue; if (is_vunmapable(msm_obj)) { @@ -80,11 +83,31 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr) } msm_gem_unlock(&msm_obj->base); - /* since we don't know any better, lets bail after a few - * and if necessary the shrinker will be invoked again. - * Seems better than unmapping *everything* - */ - if (++unmapped >= 15) + if (++unmapped >= vmap_shrink_limit) + break; + } + + return unmapped; +} + +static int +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->gpu ? &priv->gpu->active_list : NULL, + NULL, + }; + unsigned idx, unmapped = 0; + + mutex_lock(&priv->mm_lock); + + for (idx = 0; mm_lists[idx]; idx++) { + unmapped += vmap_shrink(mm_lists[idx]); + + if (unmapped >= vmap_shrink_limit) break; } -- 2.28.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] drm/msm/shrinker: Only iterate dontneed objs 2020-11-14 19:30 [PATCH 0/3] drm/msm: Shrinker fixes and opts Rob Clark 2020-11-14 19:30 ` [PATCH 1/3] drm/msm: Protect obj->active_count under obj lock Rob Clark 2020-11-14 19:30 ` [PATCH 2/3] drm/msm/shrinker: We can vmap shrink active_list too Rob Clark @ 2020-11-14 19:30 ` Rob Clark 2020-11-16 17:20 ` Jordan Crouse 2 siblings, 1 reply; 6+ messages in thread From: Rob Clark @ 2020-11-14 19:30 UTC (permalink / raw) To: dri-devel Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter, open list:DRM DRIVER FOR MSM ADRENO GPU, open list:DRM DRIVER FOR MSM ADRENO GPU, open list From: Rob Clark <robdclark@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 <robdclark@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); + + 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] drm/msm/shrinker: Only iterate dontneed objs 2020-11-14 19:30 ` [PATCH 3/3] drm/msm/shrinker: Only iterate dontneed objs Rob Clark @ 2020-11-16 17:20 ` Jordan Crouse 2020-11-16 17:31 ` [Freedreno] " Rob Clark 0 siblings, 1 reply; 6+ messages in thread From: Jordan Crouse @ 2020-11-16 17:20 UTC (permalink / raw) To: Rob Clark Cc: dri-devel, Rob Clark, open list:DRM DRIVER FOR MSM ADRENO GPU, David Airlie, open list:DRM DRIVER FOR MSM ADRENO GPU, open list, Sean Paul On Sat, Nov 14, 2020 at 11:30:10AM -0800, Rob Clark wrote: > From: Rob Clark <robdclark@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 <robdclark@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-devel@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Freedreno] [PATCH 3/3] drm/msm/shrinker: Only iterate dontneed objs 2020-11-16 17:20 ` Jordan Crouse @ 2020-11-16 17:31 ` Rob Clark 0 siblings, 0 replies; 6+ messages in thread From: Rob Clark @ 2020-11-16 17:31 UTC (permalink / raw) To: Rob Clark, dri-devel, Rob Clark, open list:DRM DRIVER FOR MSM ADRENO GPU, David Airlie, open list:DRM DRIVER FOR MSM ADRENO GPU, open list, Sean Paul On Mon, Nov 16, 2020 at 9:20 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: > > On Sat, Nov 14, 2020 at 11:30:10AM -0800, Rob Clark wrote: > > From: Rob Clark <robdclark@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 <robdclark@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. Oh, whoops, yeah that is inverted BR, -R > 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@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 > _______________________________________________ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-16 17:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-11-14 19:30 [PATCH 0/3] drm/msm: Shrinker fixes and opts Rob Clark 2020-11-14 19:30 ` [PATCH 1/3] drm/msm: Protect obj->active_count under obj lock Rob Clark 2020-11-14 19:30 ` [PATCH 2/3] drm/msm/shrinker: We can vmap shrink active_list too Rob Clark 2020-11-14 19:30 ` [PATCH 3/3] drm/msm/shrinker: Only iterate dontneed objs Rob Clark 2020-11-16 17:20 ` Jordan Crouse 2020-11-16 17:31 ` [Freedreno] " Rob Clark
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).