On Wed, Mar 31, 2021 at 4:39 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > Hi, > > On Wed, Mar 31, 2021 at 4:23 PM Rob Clark <robdclark@xxxxxxxxx> wrote: > > > > On Wed, Mar 31, 2021 at 3:44 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > > > > > Hi, > > > > > > On Wed, Mar 31, 2021 at 3:14 PM Rob Clark <robdclark@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel