Re: [Intel-gfx] [PATCH] [RFC] mm/shrinker: Add a shrinker flag to always shrink a bit

[Knut Petersen <Knut_Petersen@t-online.de>]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>