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

[Knut Petersen]

On 19.09.2013 08:57, Daniel Vetter wrote:
> On Wed, Sep 18, 2013 at 10:38 PM, Dave Chinner <david@xxxxxxxxxxxxx> 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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/