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