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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
