From: Rob Clark <robdclark@gmail.com>
To: Doug Anderson <dianders@chromium.org>
Cc: Rob Clark <robdclark@chromium.org>,
"open list:DRM DRIVER FOR MSM ADRENO GPU"
<freedreno@lists.freedesktop.org>,
David Airlie <airlied@linux.ie>,
"open list:DRM DRIVER FOR MSM ADRENO GPU"
<linux-arm-msm@vger.kernel.org>,
open list <linux-kernel@vger.kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
Sean Paul <sean@poorly.run>
Subject: Re: [PATCH 2/4] drm/msm: Avoid mutex in shrinker_count()
Date: Wed, 31 Mar 2021 17:17:57 -0700 [thread overview]
Message-ID: <CAF6AEGtFVdE6O12kQg2+xbLmDni=1Ln_v+AhzMSqVqZ5YejGWw@mail.gmail.com> (raw)
In-Reply-To: <CAD=FV=VLzDW93COWPALyin3N-wM=a7uKG7WgttmMCncf7qRmKQ@mail.gmail.com>
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-04-01 0:14 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-31 22:16 [PATCH 0/4] drm/msm: Shrinker (and related) fixes Rob Clark
2021-03-31 22:16 ` [PATCH 1/4] drm/msm: Remove unused freed llist node Rob Clark
2021-03-31 23:46 ` Doug Anderson
2021-03-31 22:16 ` [PATCH 2/4] drm/msm: Avoid mutex in shrinker_count() Rob Clark
2021-03-31 22:44 ` Doug Anderson
2021-03-31 23:26 ` Rob Clark
2021-03-31 23:39 ` Doug Anderson
2021-04-01 0:17 ` Rob Clark [this message]
2021-03-31 22:16 ` [PATCH 3/4] drm/msm: Fix debugfs deadlock Rob Clark
2021-03-31 23:13 ` Doug Anderson
2021-03-31 23:31 ` Rob Clark
2021-03-31 22:16 ` [PATCH 4/4] drm/msm: Improved debugfs gem stats Rob Clark
2021-03-31 23:33 ` Doug Anderson
2021-03-31 23:46 ` [PATCH 0/4] drm/msm: Shrinker (and related) fixes Doug Anderson
2021-04-01 1:27 ` [PATCH v2 " Rob Clark
2021-04-01 1:27 ` [PATCH v2 1/4] drm/msm: Remove unused freed llist node Rob Clark
2021-04-01 15:34 ` Doug Anderson
2021-04-01 1:27 ` [PATCH v2 2/4] drm/msm: Avoid mutex in shrinker_count() Rob Clark
2021-04-01 15:34 ` Doug Anderson
2021-04-01 17:37 ` Rob Clark
2021-04-01 1:27 ` [PATCH v2 3/4] drm/msm: Fix debugfs deadlock Rob Clark
2021-04-01 15:34 ` Doug Anderson
2021-04-01 1:27 ` [PATCH v2 4/4] drm/msm: Improved debugfs gem stats Rob Clark
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAF6AEGtFVdE6O12kQg2+xbLmDni=1Ln_v+AhzMSqVqZ5YejGWw@mail.gmail.com' \
--to=robdclark@gmail.com \
--cc=airlied@linux.ie \
--cc=dianders@chromium.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robdclark@chromium.org \
--cc=sean@poorly.run \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).