Messages in this thread | | | Date | Thu, 19 Sep 2013 10:04:12 +0200 | From | Knut Petersen <> | Subject | Re: [Intel-gfx] [PATCH] [RFC] mm/shrinker: Add a shrinker flag to always shrink a bit |
| |
On 19.09.2013 08:57, Daniel Vetter wrote: > On Wed, Sep 18, 2013 at 10:38 PM, Dave Chinner <david@fromorbit.com> wrote: >> No, that's wrong. ->count_objects should never ass SHRINK_STOP. >> Indeed, it should always return a count of objects in the cache, >> regardless of the context. >> >> SHRINK_STOP is for ->scan_objects to tell the shrinker it can make >> any progress due to the context it is called in. This allows the >> shirnker to defer the work to another call in a different context. >> However, if ->count-objects doesn't return a count, the work that >> was supposed to be done cannot be deferred, and that is what >> ->count_objects should always return the number of objects in the >> cache. > So we should rework the locking in the drm/i915 shrinker to be able to > always count objects? Thus far no one screamed yet that we're not > really able to do that in all call contexts ...
If this would have been a problem in the past, it probably would have been ended up as one of those unresolved random glitches ...
> So should I revert 81e49f or will the early return 0; completely upset > the core shrinker logic?
After Daves answer and a look at all other uses of SHRINK_STOP in the current kernel sources it is clear that 81e49f must be reverted.
Wherever else SHRINK_STOP is returned, it ends up in ->scan_objects. So i915_gem_inactive_scan() and not i915_gem_inactive_count() should return that value in case of a failed trylock:
i915_gem_inactive_scan(struct shrinker *shrinker, struct shrink_control *sc) { struct drm_i915_private *dev_priv = container_of(shrinker, struct drm_i915_private, mm.inactive_shrinker); struct drm_device *dev = dev_priv->dev; int nr_to_scan = sc->nr_to_scan; unsigned long freed; bool unlock = true;
if (!mutex_trylock(&dev->struct_mutex)) { if (!mutex_is_locked_by(&dev->struct_mutex, current)) - return 0; + return SHRINK_STOP;
if (dev_priv->mm.shrinker_no_lock_stealing) - return 0; + return SHRINK_STOP;
unlock = false; }
atm a kernel with 81e49f reverted, i915_gem_inactive_scan() changed as described above, and i915_gem_inactive_count() always counting _without_ any locking seems to work fine here. Is locking really needed at that place?
cu, Knut
| |