From: Doug Anderson <dianders@chromium.org> To: Rob Clark <robdclark@gmail.com> 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>, "Sumit Semwal" <sumit.semwal@linaro.org>, "Christian König" <christian.koenig@amd.com>, "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>, "open list:DMA BUFFER SHARING FRAMEWORK" <linux-media@vger.kernel.org>, "moderated list:DMA BUFFER SHARING FRAMEWORK" <linaro-mm-sig@lists.linaro.org> Subject: Re: [PATCH v2 2/4] drm/msm: Avoid mutex in shrinker_count() Date: Thu, 1 Apr 2021 08:34:43 -0700 [thread overview] Message-ID: <CAD=FV=XexfG9oQa8JndOgQ9JSNRmO4-xjmQdiA_9Rn9dJWxsow@mail.gmail.com> (raw) In-Reply-To: <20210401012722.527712-3-robdclark@gmail.com> Hi, On Wed, Mar 31, 2021 at 6:24 PM Rob Clark <robdclark@gmail.com> wrote: > > @@ -45,6 +30,9 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) > list_for_each_entry(msm_obj, &priv->inactive_dontneed, mm_list) { > if (freed >= sc->nr_to_scan) > break; > + /* Use trylock, because we cannot block on a obj that > + * might be trying to acquire mm_lock > + */ nit: I thought the above multi-line commenting style was only for "net" subsystem? > if (!msm_gem_trylock(&msm_obj->base)) > continue; > if (is_purgeable(msm_obj)) { > @@ -56,8 +44,11 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) > > mutex_unlock(&priv->mm_lock); > > - if (freed > 0) > + if (freed > 0) { > trace_msm_gem_purge(freed << PAGE_SHIFT); > + } else { > + return SHRINK_STOP; > + } It probably doesn't matter, but I wonder if we should still be returning SHRINK_STOP if we got any trylock failures. It could possibly be worth returning 0 in that case? > @@ -75,6 +66,9 @@ vmap_shrink(struct list_head *mm_list) > unsigned unmapped = 0; > > list_for_each_entry(msm_obj, mm_list, mm_list) { > + /* Use trylock, because we cannot block on a obj that > + * might be trying to acquire mm_lock > + */ If you end up changing the commenting style above, should also be here. At this point this seems fine to land to me. Though I'm not an expert on every interaction in this code, I've spent enough time starting at it that I'm comfortable with: Reviewed-by: Douglas Anderson <dianders@chromium.org>
WARNING: multiple messages have this Message-ID (diff)
From: Doug Anderson <dianders@chromium.org> To: Rob Clark <robdclark@gmail.com> 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>, "Christian König" <christian.koenig@amd.com>, "moderated list:DMA BUFFER SHARING FRAMEWORK" <linaro-mm-sig@lists.linaro.org>, "Sean Paul" <sean@poorly.run>, "open list:DMA BUFFER SHARING FRAMEWORK" <linux-media@vger.kernel.org> Subject: Re: [PATCH v2 2/4] drm/msm: Avoid mutex in shrinker_count() Date: Thu, 1 Apr 2021 08:34:43 -0700 [thread overview] Message-ID: <CAD=FV=XexfG9oQa8JndOgQ9JSNRmO4-xjmQdiA_9Rn9dJWxsow@mail.gmail.com> (raw) In-Reply-To: <20210401012722.527712-3-robdclark@gmail.com> Hi, On Wed, Mar 31, 2021 at 6:24 PM Rob Clark <robdclark@gmail.com> wrote: > > @@ -45,6 +30,9 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) > list_for_each_entry(msm_obj, &priv->inactive_dontneed, mm_list) { > if (freed >= sc->nr_to_scan) > break; > + /* Use trylock, because we cannot block on a obj that > + * might be trying to acquire mm_lock > + */ nit: I thought the above multi-line commenting style was only for "net" subsystem? > if (!msm_gem_trylock(&msm_obj->base)) > continue; > if (is_purgeable(msm_obj)) { > @@ -56,8 +44,11 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc) > > mutex_unlock(&priv->mm_lock); > > - if (freed > 0) > + if (freed > 0) { > trace_msm_gem_purge(freed << PAGE_SHIFT); > + } else { > + return SHRINK_STOP; > + } It probably doesn't matter, but I wonder if we should still be returning SHRINK_STOP if we got any trylock failures. It could possibly be worth returning 0 in that case? > @@ -75,6 +66,9 @@ vmap_shrink(struct list_head *mm_list) > unsigned unmapped = 0; > > list_for_each_entry(msm_obj, mm_list, mm_list) { > + /* Use trylock, because we cannot block on a obj that > + * might be trying to acquire mm_lock > + */ If you end up changing the commenting style above, should also be here. At this point this seems fine to land to me. Though I'm not an expert on every interaction in this code, I've spent enough time starting at it that I'm comfortable with: Reviewed-by: Douglas Anderson <dianders@chromium.org> _______________________________________________ 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 18:03 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 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 [this message] 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='CAD=FV=XexfG9oQa8JndOgQ9JSNRmO4-xjmQdiA_9Rn9dJWxsow@mail.gmail.com' \ --to=dianders@chromium.org \ --cc=airlied@linux.ie \ --cc=christian.koenig@amd.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=linaro-mm-sig@lists.linaro.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=robdclark@chromium.org \ --cc=robdclark@gmail.com \ --cc=sean@poorly.run \ --cc=sumit.semwal@linaro.org \ /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.