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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <523AAFFC.2070300@t-online.de>
Date:	Thu, 19 Sep 2013 10:04:12 +0200
From:	Knut Petersen <Knut_Petersen@...nline.de>
To:	Daniel Vetter <daniel.vetter@...ll.ch>
CC:	Dave Chinner <david@...morbit.com>, Linux MM <linux-mm@...ck.org>,
	Rik van Riel <riel@...hat.com>,
	Intel Graphics Development <intel-gfx@...ts.freedesktop.org>,
	Johannes Weiner <hannes@...xchg.org>,
	LKML <linux-kernel@...r.kernel.org>,
	DRI Development <dri-devel@...ts.freedesktop.org>,
	Michal Hocko <mhocko@...e.cz>, Mel Gorman <mgorman@...e.de>,
	Glauber Costa <glommer@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>
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@...morbit.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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/