diff mbox series

[2/4] drm/msm: Avoid mutex in shrinker_count()

Message ID 20210331221630.488498-3-robdclark@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show
Series drm/msm: Shrinker (and related) fixes | expand

Commit Message

Rob Clark March 31, 2021, 10:16 p.m. UTC
From: Rob Clark <robdclark@chromium.org>

When the system is under heavy memory pressure, we can end up with lots
of concurrent calls into the shrinker.  Keeping a running tab on what we
can shrink avoids grabbing a lock in shrinker->count(), and avoids
shrinker->scan() getting called when not profitable.

Also, we can keep purged objects in their own list to avoid re-traversing
them to help cut down time in the critical section further.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_drv.c          |  1 +
 drivers/gpu/drm/msm/msm_drv.h          |  2 ++
 drivers/gpu/drm/msm/msm_gem.c          | 16 +++++++++++--
 drivers/gpu/drm/msm/msm_gem.h          | 32 ++++++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_gem_shrinker.c | 17 +-------------
 5 files changed, 50 insertions(+), 18 deletions(-)

Comments

Doug Anderson March 31, 2021, 10:44 p.m. UTC | #1
Hi,

On Wed, Mar 31, 2021 at 3:14 PM Rob Clark <robdclark@gmail.com> wrote:
>
> @@ -818,11 +820,19 @@ static void update_inactive(struct msm_gem_object *msm_obj)
>         mutex_lock(&priv->mm_lock);
>         WARN_ON(msm_obj->active_count != 0);
>
> +       if (msm_obj->dontneed)
> +               mark_unpurgable(msm_obj);
> +
>         list_del_init(&msm_obj->mm_list);
> -       if (msm_obj->madv == MSM_MADV_WILLNEED)
> +       if (msm_obj->madv == MSM_MADV_WILLNEED) {
>                 list_add_tail(&msm_obj->mm_list, &priv->inactive_willneed);
> -       else
> +       } else if (msm_obj->madv == MSM_MADV_DONTNEED) {
>                 list_add_tail(&msm_obj->mm_list, &priv->inactive_dontneed);
> +               mark_purgable(msm_obj);
> +       } else {
> +               WARN_ON(msm_obj->madv != __MSM_MADV_PURGED);
> +               list_add_tail(&msm_obj->mm_list, &priv->inactive_purged);

I'm probably being dense, but what's the point of adding it to the
"inactive_purged" list here? You never look at that list, right? You
already did a list_del_init() on this object's list pointer
("mm_list"). I don't see how adding it to a bogus list helps with
anything.


> @@ -198,6 +203,33 @@ static inline bool is_vunmapable(struct msm_gem_object *msm_obj)
>         return (msm_obj->vmap_count == 0) && msm_obj->vaddr;
>  }
>
> +static inline void mark_purgable(struct msm_gem_object *msm_obj)
> +{
> +       struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> +
> +       WARN_ON(!mutex_is_locked(&priv->mm_lock));
> +
> +       if (WARN_ON(msm_obj->dontneed))
> +               return;

The is_purgeable() function also checks other things besides just
"MSM_MADV_DONTNEED". Do we need to check those too? Specifically:

 msm_obj->sgt && !msm_obj->base.dma_buf && !msm_obj->base.import_attach

...or is it just being paranoid?

I guess I'm just worried that if any of those might be important then
we'll consistently report back that we have a count of things that can
be purged but then scan() won't find anything to do. That wouldn't be
great.


> +       priv->shrinkable_count += msm_obj->base.size >> PAGE_SHIFT;
> +       msm_obj->dontneed = true;
> +}
> +
> +static inline void mark_unpurgable(struct msm_gem_object *msm_obj)
> +{
> +       struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> +
> +       WARN_ON(!mutex_is_locked(&priv->mm_lock));
> +
> +       if (WARN_ON(!msm_obj->dontneed))
> +               return;
> +
> +       priv->shrinkable_count -= msm_obj->base.size >> PAGE_SHIFT;
> +       WARN_ON(priv->shrinkable_count < 0);

If you changed the order maybe you could make shrinkable_count
"unsigned long" to match the shrinker API?

 new_shrinkable = msm_obj->base.size >> PAGE_SHIFT;
 WARN_ON(new_shrinkable > priv->shrinkable_count);
 priv->shrinkable_count -= new_shrinkable


-Doug
Rob Clark March 31, 2021, 11:26 p.m. UTC | #2
On Wed, Mar 31, 2021 at 3:44 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Mar 31, 2021 at 3:14 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > @@ -818,11 +820,19 @@ static void update_inactive(struct msm_gem_object *msm_obj)
> >         mutex_lock(&priv->mm_lock);
> >         WARN_ON(msm_obj->active_count != 0);
> >
> > +       if (msm_obj->dontneed)
> > +               mark_unpurgable(msm_obj);
> > +
> >         list_del_init(&msm_obj->mm_list);
> > -       if (msm_obj->madv == MSM_MADV_WILLNEED)
> > +       if (msm_obj->madv == MSM_MADV_WILLNEED) {
> >                 list_add_tail(&msm_obj->mm_list, &priv->inactive_willneed);
> > -       else
> > +       } else if (msm_obj->madv == MSM_MADV_DONTNEED) {
> >                 list_add_tail(&msm_obj->mm_list, &priv->inactive_dontneed);
> > +               mark_purgable(msm_obj);
> > +       } else {
> > +               WARN_ON(msm_obj->madv != __MSM_MADV_PURGED);
> > +               list_add_tail(&msm_obj->mm_list, &priv->inactive_purged);
>
> I'm probably being dense, but what's the point of adding it to the
> "inactive_purged" list here? You never look at that list, right? You
> already did a list_del_init() on this object's list pointer
> ("mm_list"). I don't see how adding it to a bogus list helps with
> anything.

It preserves the "every bo is in one of these lists" statement, but
other than that you are right we aren't otherwise doing anything with
that list.  (Or we could replace the list_del_init() with list_del()..
I tend to instinctively go for list_del_init())

>
> > @@ -198,6 +203,33 @@ static inline bool is_vunmapable(struct msm_gem_object *msm_obj)
> >         return (msm_obj->vmap_count == 0) && msm_obj->vaddr;
> >  }
> >
> > +static inline void mark_purgable(struct msm_gem_object *msm_obj)
> > +{
> > +       struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> > +
> > +       WARN_ON(!mutex_is_locked(&priv->mm_lock));
> > +
> > +       if (WARN_ON(msm_obj->dontneed))
> > +               return;
>
> The is_purgeable() function also checks other things besides just
> "MSM_MADV_DONTNEED". Do we need to check those too? Specifically:
>
>  msm_obj->sgt && !msm_obj->base.dma_buf && !msm_obj->base.import_attach
>
> ...or is it just being paranoid?
>
> I guess I'm just worried that if any of those might be important then
> we'll consistently report back that we have a count of things that can
> be purged but then scan() won't find anything to do. That wouldn't be
> great.

Hmm, I thought msm_gem_madvise() returned an error instead of allowing
MSM_MADV_DONTNEED to be set on imported/exported dma-bufs.. it
probably should to be complete (but userspace already knows not to
madvise an imported/exported buffer for other reasons.. ie. we can't
let a shared buffer end up in the bo cache).  I'll re-work that a bit.

The msm_obj->sgt case is a bit more tricky.. that will be the case of
a freshly allocated obj that does not have backing patches yet.  But
it seems like enough of a corner case, that I'm happy to live with
it.. ie. the tricky thing is not leaking decrements of
priv->shrinkable_count or underflowing priv->shrinkable_count, and
caring about the !msm_obj->sgt case doubles the number of states an
object can be in, and the shrinker->count() return value is just an
estimate.

>
> > +       priv->shrinkable_count += msm_obj->base.size >> PAGE_SHIFT;
> > +       msm_obj->dontneed = true;
> > +}
> > +
> > +static inline void mark_unpurgable(struct msm_gem_object *msm_obj)
> > +{
> > +       struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> > +
> > +       WARN_ON(!mutex_is_locked(&priv->mm_lock));
> > +
> > +       if (WARN_ON(!msm_obj->dontneed))
> > +               return;
> > +
> > +       priv->shrinkable_count -= msm_obj->base.size >> PAGE_SHIFT;
> > +       WARN_ON(priv->shrinkable_count < 0);
>
> If you changed the order maybe you could make shrinkable_count
> "unsigned long" to match the shrinker API?
>
>  new_shrinkable = msm_obj->base.size >> PAGE_SHIFT;
>  WARN_ON(new_shrinkable > priv->shrinkable_count);
>  priv->shrinkable_count -= new_shrinkable
>

True, although I've developed a preference for signed integers in
cases where it can underflow if you mess up

BR,
-R
Doug Anderson March 31, 2021, 11:39 p.m. UTC | #3
Hi,

On Wed, Mar 31, 2021 at 4:23 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Wed, Mar 31, 2021 at 3:44 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Mar 31, 2021 at 3:14 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > @@ -818,11 +820,19 @@ static void update_inactive(struct msm_gem_object *msm_obj)
> > >         mutex_lock(&priv->mm_lock);
> > >         WARN_ON(msm_obj->active_count != 0);
> > >
> > > +       if (msm_obj->dontneed)
> > > +               mark_unpurgable(msm_obj);
> > > +
> > >         list_del_init(&msm_obj->mm_list);
> > > -       if (msm_obj->madv == MSM_MADV_WILLNEED)
> > > +       if (msm_obj->madv == MSM_MADV_WILLNEED) {
> > >                 list_add_tail(&msm_obj->mm_list, &priv->inactive_willneed);
> > > -       else
> > > +       } else if (msm_obj->madv == MSM_MADV_DONTNEED) {
> > >                 list_add_tail(&msm_obj->mm_list, &priv->inactive_dontneed);
> > > +               mark_purgable(msm_obj);
> > > +       } else {
> > > +               WARN_ON(msm_obj->madv != __MSM_MADV_PURGED);
> > > +               list_add_tail(&msm_obj->mm_list, &priv->inactive_purged);
> >
> > I'm probably being dense, but what's the point of adding it to the
> > "inactive_purged" list here? You never look at that list, right? You
> > already did a list_del_init() on this object's list pointer
> > ("mm_list"). I don't see how adding it to a bogus list helps with
> > anything.
>
> It preserves the "every bo is in one of these lists" statement, but
> other than that you are right we aren't otherwise doing anything with
> that list.  (Or we could replace the list_del_init() with list_del()..
> I tend to instinctively go for list_del_init())

If you really want this list, it wouldn't hurt to at least have a
comment saying that it's not used for anything so people like me doing
go trying to figure out what it's used for. ;-)


> > > @@ -198,6 +203,33 @@ static inline bool is_vunmapable(struct msm_gem_object *msm_obj)
> > >         return (msm_obj->vmap_count == 0) && msm_obj->vaddr;
> > >  }
> > >
> > > +static inline void mark_purgable(struct msm_gem_object *msm_obj)
> > > +{
> > > +       struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> > > +
> > > +       WARN_ON(!mutex_is_locked(&priv->mm_lock));
> > > +
> > > +       if (WARN_ON(msm_obj->dontneed))
> > > +               return;
> >
> > The is_purgeable() function also checks other things besides just
> > "MSM_MADV_DONTNEED". Do we need to check those too? Specifically:
> >
> >  msm_obj->sgt && !msm_obj->base.dma_buf && !msm_obj->base.import_attach
> >
> > ...or is it just being paranoid?
> >
> > I guess I'm just worried that if any of those might be important then
> > we'll consistently report back that we have a count of things that can
> > be purged but then scan() won't find anything to do. That wouldn't be
> > great.
>
> Hmm, I thought msm_gem_madvise() returned an error instead of allowing
> MSM_MADV_DONTNEED to be set on imported/exported dma-bufs.. it
> probably should to be complete (but userspace already knows not to
> madvise an imported/exported buffer for other reasons.. ie. we can't
> let a shared buffer end up in the bo cache).  I'll re-work that a bit.
>
> The msm_obj->sgt case is a bit more tricky.. that will be the case of
> a freshly allocated obj that does not have backing patches yet.  But
> it seems like enough of a corner case, that I'm happy to live with
> it.. ie. the tricky thing is not leaking decrements of
> priv->shrinkable_count or underflowing priv->shrinkable_count, and
> caring about the !msm_obj->sgt case doubles the number of states an
> object can be in, and the shrinker->count() return value is just an
> estimate.

I think it's equally important to make sure that we don't constantly
have a non-zero count and then have scan() do nothing.  If there's a
transitory blip then it's fine, but it's not OK if it can be steady
state. Then you end up with:

1. How many objects do you have to free? 10
2. OK, free some. How many did you free? 0
3. Oh. You got more to do, I'll call you again.
4. Goto #1

...and it just keeps looping, right?

As long as you're confident that this case can't happen then we're
probably fine, but good to be careful. Is there any way we can make
sure that a "freshly allocated object" isn't ever in the "DONTNEED"
state?


> > > +       priv->shrinkable_count += msm_obj->base.size >> PAGE_SHIFT;
> > > +       msm_obj->dontneed = true;
> > > +}
> > > +
> > > +static inline void mark_unpurgable(struct msm_gem_object *msm_obj)
> > > +{
> > > +       struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> > > +
> > > +       WARN_ON(!mutex_is_locked(&priv->mm_lock));
> > > +
> > > +       if (WARN_ON(!msm_obj->dontneed))
> > > +               return;
> > > +
> > > +       priv->shrinkable_count -= msm_obj->base.size >> PAGE_SHIFT;
> > > +       WARN_ON(priv->shrinkable_count < 0);
> >
> > If you changed the order maybe you could make shrinkable_count
> > "unsigned long" to match the shrinker API?
> >
> >  new_shrinkable = msm_obj->base.size >> PAGE_SHIFT;
> >  WARN_ON(new_shrinkable > priv->shrinkable_count);
> >  priv->shrinkable_count -= new_shrinkable
> >
>
> True, although I've developed a preference for signed integers in
> cases where it can underflow if you mess up

Yeah, I guess it's fine since it's a count of pages and we really
can't have _that_ many pages worth of stuff to purge. It might not
hurt to at least declare it as a "long" instead of an "int" though to
match the shrinker API.

-Doug
Rob Clark April 1, 2021, 12:17 a.m. UTC | #4
On Wed, Mar 31, 2021 at 4:39 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Mar 31, 2021 at 4:23 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Wed, Mar 31, 2021 at 3:44 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Mar 31, 2021 at 3:14 PM Rob Clark <robdclark@gmail.com> wrote:
> > > >
> > > > @@ -818,11 +820,19 @@ static void update_inactive(struct msm_gem_object *msm_obj)
> > > >         mutex_lock(&priv->mm_lock);
> > > >         WARN_ON(msm_obj->active_count != 0);
> > > >
> > > > +       if (msm_obj->dontneed)
> > > > +               mark_unpurgable(msm_obj);
> > > > +
> > > >         list_del_init(&msm_obj->mm_list);
> > > > -       if (msm_obj->madv == MSM_MADV_WILLNEED)
> > > > +       if (msm_obj->madv == MSM_MADV_WILLNEED) {
> > > >                 list_add_tail(&msm_obj->mm_list, &priv->inactive_willneed);
> > > > -       else
> > > > +       } else if (msm_obj->madv == MSM_MADV_DONTNEED) {
> > > >                 list_add_tail(&msm_obj->mm_list, &priv->inactive_dontneed);
> > > > +               mark_purgable(msm_obj);
> > > > +       } else {
> > > > +               WARN_ON(msm_obj->madv != __MSM_MADV_PURGED);
> > > > +               list_add_tail(&msm_obj->mm_list, &priv->inactive_purged);
> > >
> > > I'm probably being dense, but what's the point of adding it to the
> > > "inactive_purged" list here? You never look at that list, right? You
> > > already did a list_del_init() on this object's list pointer
> > > ("mm_list"). I don't see how adding it to a bogus list helps with
> > > anything.
> >
> > It preserves the "every bo is in one of these lists" statement, but
> > other than that you are right we aren't otherwise doing anything with
> > that list.  (Or we could replace the list_del_init() with list_del()..
> > I tend to instinctively go for list_del_init())
>
> If you really want this list, it wouldn't hurt to at least have a
> comment saying that it's not used for anything so people like me doing
> go trying to figure out what it's used for. ;-)
>
>
> > > > @@ -198,6 +203,33 @@ static inline bool is_vunmapable(struct msm_gem_object *msm_obj)
> > > >         return (msm_obj->vmap_count == 0) && msm_obj->vaddr;
> > > >  }
> > > >
> > > > +static inline void mark_purgable(struct msm_gem_object *msm_obj)
> > > > +{
> > > > +       struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> > > > +
> > > > +       WARN_ON(!mutex_is_locked(&priv->mm_lock));
> > > > +
> > > > +       if (WARN_ON(msm_obj->dontneed))
> > > > +               return;
> > >
> > > The is_purgeable() function also checks other things besides just
> > > "MSM_MADV_DONTNEED". Do we need to check those too? Specifically:
> > >
> > >  msm_obj->sgt && !msm_obj->base.dma_buf && !msm_obj->base.import_attach
> > >
> > > ...or is it just being paranoid?
> > >
> > > I guess I'm just worried that if any of those might be important then
> > > we'll consistently report back that we have a count of things that can
> > > be purged but then scan() won't find anything to do. That wouldn't be
> > > great.
> >
> > Hmm, I thought msm_gem_madvise() returned an error instead of allowing
> > MSM_MADV_DONTNEED to be set on imported/exported dma-bufs.. it
> > probably should to be complete (but userspace already knows not to
> > madvise an imported/exported buffer for other reasons.. ie. we can't
> > let a shared buffer end up in the bo cache).  I'll re-work that a bit.
> >
> > The msm_obj->sgt case is a bit more tricky.. that will be the case of
> > a freshly allocated obj that does not have backing patches yet.  But
> > it seems like enough of a corner case, that I'm happy to live with
> > it.. ie. the tricky thing is not leaking decrements of
> > priv->shrinkable_count or underflowing priv->shrinkable_count, and
> > caring about the !msm_obj->sgt case doubles the number of states an
> > object can be in, and the shrinker->count() return value is just an
> > estimate.
>
> I think it's equally important to make sure that we don't constantly
> have a non-zero count and then have scan() do nothing.  If there's a
> transitory blip then it's fine, but it's not OK if it can be steady
> state. Then you end up with:
>
> 1. How many objects do you have to free? 10
> 2. OK, free some. How many did you free? 0
> 3. Oh. You got more to do, I'll call you again.
> 4. Goto #1
>
> ...and it just keeps looping, right?

Looking more closely at vmscan, it looks like we should return
SHRINK_STOP instead of zero

BR,
-R

>
> As long as you're confident that this case can't happen then we're
> probably fine, but good to be careful. Is there any way we can make
> sure that a "freshly allocated object" isn't ever in the "DONTNEED"
> state?
>
>
> > > > +       priv->shrinkable_count += msm_obj->base.size >> PAGE_SHIFT;
> > > > +       msm_obj->dontneed = true;
> > > > +}
> > > > +
> > > > +static inline void mark_unpurgable(struct msm_gem_object *msm_obj)
> > > > +{
> > > > +       struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
> > > > +
> > > > +       WARN_ON(!mutex_is_locked(&priv->mm_lock));
> > > > +
> > > > +       if (WARN_ON(!msm_obj->dontneed))
> > > > +               return;
> > > > +
> > > > +       priv->shrinkable_count -= msm_obj->base.size >> PAGE_SHIFT;
> > > > +       WARN_ON(priv->shrinkable_count < 0);
> > >
> > > If you changed the order maybe you could make shrinkable_count
> > > "unsigned long" to match the shrinker API?
> > >
> > >  new_shrinkable = msm_obj->base.size >> PAGE_SHIFT;
> > >  WARN_ON(new_shrinkable > priv->shrinkable_count);
> > >  priv->shrinkable_count -= new_shrinkable
> > >
> >
> > True, although I've developed a preference for signed integers in
> > cases where it can underflow if you mess up
>
> Yeah, I guess it's fine since it's a count of pages and we really
> can't have _that_ many pages worth of stuff to purge. It might not
> hurt to at least declare it as a "long" instead of an "int" though to
> match the shrinker API.
>
> -Doug
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 4f9fa0189a07..3462b0ea14c6 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -476,6 +476,7 @@  static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 
 	INIT_LIST_HEAD(&priv->inactive_willneed);
 	INIT_LIST_HEAD(&priv->inactive_dontneed);
+	INIT_LIST_HEAD(&priv->inactive_purged);
 	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 a1264cfcac5e..3ead5755f695 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -188,6 +188,8 @@  struct msm_drm_private {
 	 */
 	struct list_head inactive_willneed;  /* inactive + !shrinkable */
 	struct list_head inactive_dontneed;  /* inactive +  shrinkable */
+	struct list_head inactive_purged;    /* inactive +  purged */
+	int shrinkable_count;                /* write access under mm_lock */
 	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 9d10739c4eb2..74a92eedc992 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -719,6 +719,7 @@  void msm_gem_purge(struct drm_gem_object *obj)
 	put_iova_vmas(obj);
 
 	msm_obj->madv = __MSM_MADV_PURGED;
+	mark_unpurgable(msm_obj);
 
 	drm_vma_node_unmap(&obj->vma_node, dev->anon_inode->i_mapping);
 	drm_gem_free_mmap_offset(obj);
@@ -790,6 +791,7 @@  void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu)
 	might_sleep();
 	WARN_ON(!msm_gem_is_locked(obj));
 	WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED);
+	WARN_ON(msm_obj->dontneed);
 
 	if (msm_obj->active_count++ == 0) {
 		mutex_lock(&priv->mm_lock);
@@ -818,11 +820,19 @@  static void update_inactive(struct msm_gem_object *msm_obj)
 	mutex_lock(&priv->mm_lock);
 	WARN_ON(msm_obj->active_count != 0);
 
+	if (msm_obj->dontneed)
+		mark_unpurgable(msm_obj);
+
 	list_del_init(&msm_obj->mm_list);
-	if (msm_obj->madv == MSM_MADV_WILLNEED)
+	if (msm_obj->madv == MSM_MADV_WILLNEED) {
 		list_add_tail(&msm_obj->mm_list, &priv->inactive_willneed);
-	else
+	} else if (msm_obj->madv == MSM_MADV_DONTNEED) {
 		list_add_tail(&msm_obj->mm_list, &priv->inactive_dontneed);
+		mark_purgable(msm_obj);
+	} else {
+		WARN_ON(msm_obj->madv != __MSM_MADV_PURGED);
+		list_add_tail(&msm_obj->mm_list, &priv->inactive_purged);
+	}
 
 	mutex_unlock(&priv->mm_lock);
 }
@@ -971,6 +981,8 @@  void msm_gem_free_object(struct drm_gem_object *obj)
 	struct msm_drm_private *priv = dev->dev_private;
 
 	mutex_lock(&priv->mm_lock);
+	if (msm_obj->dontneed)
+		mark_unpurgable(msm_obj);
 	list_del(&msm_obj->mm_list);
 	mutex_unlock(&priv->mm_lock);
 
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 7a9107cf1818..0feabae75d3d 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -50,6 +50,11 @@  struct msm_gem_object {
 	 */
 	uint8_t madv;
 
+	/**
+	 * Is object on inactive_dontneed list (ie. counted in priv->shrinkable_count)?
+	 */
+	bool dontneed : 1;
+
 	/**
 	 * count of active vmap'ing
 	 */
@@ -198,6 +203,33 @@  static inline bool is_vunmapable(struct msm_gem_object *msm_obj)
 	return (msm_obj->vmap_count == 0) && msm_obj->vaddr;
 }
 
+static inline void mark_purgable(struct msm_gem_object *msm_obj)
+{
+	struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&priv->mm_lock));
+
+	if (WARN_ON(msm_obj->dontneed))
+		return;
+
+	priv->shrinkable_count += msm_obj->base.size >> PAGE_SHIFT;
+	msm_obj->dontneed = true;
+}
+
+static inline void mark_unpurgable(struct msm_gem_object *msm_obj)
+{
+	struct msm_drm_private *priv = msm_obj->base.dev->dev_private;
+
+	WARN_ON(!mutex_is_locked(&priv->mm_lock));
+
+	if (WARN_ON(!msm_obj->dontneed))
+		return;
+
+	priv->shrinkable_count -= msm_obj->base.size >> PAGE_SHIFT;
+	WARN_ON(priv->shrinkable_count < 0);
+	msm_obj->dontneed = false;
+}
+
 void msm_gem_purge(struct drm_gem_object *obj);
 void msm_gem_vunmap(struct drm_gem_object *obj);
 
diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c
index 9d5248be746f..7db8375f2430 100644
--- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
+++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
@@ -14,22 +14,7 @@  msm_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 {
 	struct msm_drm_private *priv =
 		container_of(shrinker, struct msm_drm_private, shrinker);
-	struct msm_gem_object *msm_obj;
-	unsigned long count = 0;
-
-	mutex_lock(&priv->mm_lock);
-
-	list_for_each_entry(msm_obj, &priv->inactive_dontneed, mm_list) {
-		if (!msm_gem_trylock(&msm_obj->base))
-			continue;
-		if (is_purgeable(msm_obj))
-			count += msm_obj->base.size >> PAGE_SHIFT;
-		msm_gem_unlock(&msm_obj->base);
-	}
-
-	mutex_unlock(&priv->mm_lock);
-
-	return count;
+	return priv->shrinkable_count;
 }
 
 static unsigned long