From: Rob Clark <robdclark@gmail.com> To: Doug Anderson <dianders@chromium.org> Cc: dri-devel <dri-devel@lists.freedesktop.org>, Rob Clark <robdclark@chromium.org>, Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <linux-arm-msm@vger.kernel.org>, "open list:DRM DRIVER FOR MSM ADRENO GPU" <freedreno@lists.freedesktop.org>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 2/4] drm/msm: Avoid mutex in shrinker_count() Date: Wed, 31 Mar 2021 16:26:59 -0700 [thread overview] Message-ID: <CAF6AEGutvjUQ-bQMsAYDLq5kdRo7rQ5XwWjGSRV27VT_UOuMTw@mail.gmail.com> (raw) In-Reply-To: <CAD=FV=USXBm-ZLafNWbUK=Ny7_vwtyG164mQFs87SkXqim-Vpw@mail.gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
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 16:26:59 -0700 [thread overview] Message-ID: <CAF6AEGutvjUQ-bQMsAYDLq5kdRo7rQ5XwWjGSRV27VT_UOuMTw@mail.gmail.com> (raw) In-Reply-To: <CAD=FV=USXBm-ZLafNWbUK=Ny7_vwtyG164mQFs87SkXqim-Vpw@mail.gmail.com> 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-03-31 23:24 UTC|newest] Thread overview: 47+ 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 ` Rob Clark 2021-03-31 22:16 ` [PATCH 1/4] drm/msm: Remove unused freed llist node Rob Clark 2021-03-31 22:16 ` Rob Clark 2021-03-31 23:46 ` Doug Anderson 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:16 ` Rob Clark 2021-03-31 22:44 ` Doug Anderson 2021-03-31 22:44 ` Doug Anderson 2021-03-31 23:26 ` Rob Clark [this message] 2021-03-31 23:26 ` Rob Clark 2021-03-31 23:39 ` Doug Anderson 2021-03-31 23:39 ` Doug Anderson 2021-04-01 0:17 ` Rob Clark 2021-04-01 0:17 ` Rob Clark 2021-03-31 22:16 ` [PATCH 3/4] drm/msm: Fix debugfs deadlock Rob Clark 2021-03-31 22:16 ` Rob Clark 2021-03-31 23:13 ` Doug Anderson 2021-03-31 23:13 ` Doug Anderson 2021-03-31 23:31 ` Rob Clark 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 22:16 ` Rob Clark 2021-03-31 23:33 ` Doug Anderson 2021-03-31 23:33 ` Doug Anderson 2021-03-31 23:46 ` [PATCH 0/4] drm/msm: Shrinker (and related) fixes Doug Anderson 2021-03-31 23:46 ` Doug Anderson 2021-04-01 1:27 ` [PATCH v2 " Rob Clark 2021-04-01 1:27 ` Rob Clark 2021-04-01 1:27 ` [PATCH v2 1/4] drm/msm: Remove unused freed llist node Rob Clark 2021-04-01 1:27 ` Rob Clark 2021-04-01 15:34 ` Doug Anderson 2021-04-01 15:34 ` Doug Anderson 2021-05-26 19:03 ` patchwork-bot+linux-arm-msm 2021-04-01 1:27 ` [PATCH v2 2/4] drm/msm: Avoid mutex in shrinker_count() Rob Clark 2021-04-01 1:27 ` Rob Clark 2021-04-01 15:34 ` Doug Anderson 2021-04-01 15:34 ` Doug Anderson 2021-04-01 17:37 ` Rob Clark 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 1:27 ` Rob Clark 2021-04-01 15:34 ` Doug Anderson 2021-04-01 15:34 ` Doug Anderson 2021-04-01 1:27 ` [PATCH v2 4/4] drm/msm: Improved debugfs gem stats Rob Clark 2021-04-01 1:27 ` 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=CAF6AEGutvjUQ-bQMsAYDLq5kdRo7rQ5XwWjGSRV27VT_UOuMTw@mail.gmail.com \ --to=robdclark@gmail.com \ --cc=airlied@linux.ie \ --cc=daniel@ffwll.ch \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.