From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [Intel-gfx] [PATCH] drm/i915: Revert shrinker changes from "Track unbound pages" Date: Thu, 10 Jan 2013 18:03:38 +0100 Message-ID: <20130110170338.GE5737@phenom.ffwll.local> References: <1357837380-2463-1-git-send-email-daniel.vetter@ffwll.ch> <6c3329$820h5e@orsmga002.jf.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <6c3329$820h5e@orsmga002.jf.intel.com> Sender: stable-owner@vger.kernel.org To: Chris Wilson Cc: Daniel Vetter , Intel Graphics Development , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Jan 10, 2013 at 04:58:30PM +0000, Chris Wilson wrote: > On Thu, 10 Jan 2013 18:03:00 +0100, Daniel Vetter wrote: > > This partially reverts > > > > commit 6c085a728cf000ac1865d66f8c9b52935558b328 > > Author: Chris Wilson > > Date: Mon Aug 20 11:40:46 2012 +0200 > > > > drm/i915: Track unbound pages > > > > Closer inspection of that patch revealed a bunch of unrelated changes > > in the shrinker: > > - The shrinker count is now in pages instead of objects. > > - For counting the shrinkable objects the old code only looked at the > > inactive list, the new code looks at all bounds objects (including > > pinned ones). That is obviously in addition to the new unbound list. > > - The shrinker cound is no longer scaled with > > sysctl_vfs_cache_pressure. Note though that with the default tuning > > value of vfs_cache_pressue = 100 this doesn't affect the shrinker > > behaviour. > > - When actually shrinking objects, the old code first dropped > > purgeable objects, then normal (inactive) objects. Only then did it, > > in a last-ditch effort idle the gpu and evict everything. The new > > code omits the intermediate step of evicting normal inactive > > objects. > > > > Safe for the first change, which seems benign, and the shrinker count > > scaling, which is a bit a different story, the endresult of all these > > changes is that the shrinker is _much_ more likely to fall back to the > > last-ditch resort of idling the gpu and evicting everything. The old > > code could only do that if something else evicted lots of objects > > meanwhile (since without any other changes the nr_to_scan will be > > smaller than the object count). > > > > Reverting the vfs_cache_pressure behaviour itself is a bit bogus: Only > > dentry/inode object caches should scale their shrinker counts with > > vfs_cache_pressure. Originally I've had that change reverted, too. But > > Chris Wilson insisted that it's too bogus and shouldn't again see the > > light of day. > > > > Hence revert all these other changes and restore the old shrinker > > behaviour, with the minor adjustment that we now first scan the > > unbound list, then the inactive list for each object category > > (purgeable or normal). > > > > A similar patch has been tested by a few people affected by the gen4/5 > > hangs which started to appear in 3.7, which some people bisected to > > the "drm/i915: Track unbound pages" commit. But just disabling the > > unbound logic alone didn't change things at all. > > > > Note that this patch doesn't fix the referenced bugs, it only hides > > the underlying bug(s) well enough to restore pre-3.7 behaviour. The > > key to achieve that is to massively reduce the likelyhood of going > > into a full gpu stall and evicting everything. > > > > v2: Reword commit message a bit, taking Chris Wilson's comment into > > account. > > > > v3: On Chris Wilson's insistency, do not reinstate the rather bogus > > vfs_cache_pressure change. > > > > Tested-by: Greg KH > > Tested-by: Dave Kleikamp > > References: https://bugs.freedesktop.org/show_bug.cgi?id=55984 > > References: https://bugs.freedesktop.org/show_bug.cgi?id=57122 > > References: https://bugs.freedesktop.org/show_bug.cgi?id=56916 > > References: https://bugs.freedesktop.org/show_bug.cgi?id=57136 > > Cc: Chris Wilson > > Cc: stable@vger.kernel.org > > Signed-off-by: Daniel Vetter > > Acked-by: Chris Wilson Picked up for -fixes. > I just hope the clue bat descends soonest before we find another way of > triggering the spurious hangs. Indeed. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch