i915 modeset memory corruption issues? (Fwd: Oops in ext3_block_to_path.isra.40+0x26/0x11b)

[Hugh Dickins]

On Sat, 17 Mar 2012, Linus Torvalds wrote:
> On Sat, Mar 17, 2012 at 1:23 PM, Dave Airlie <airlied at gmail.com> wrote:
> >
> > I did however get a flashback in google to this:
> >
> > http://lists.fedoraproject.org/pipermail/scm-commits/2010-July/456636.html
> >
> > Linus don't think we ever did work out why that worked, I wonder if we
> > lost something after that.
> 
> Hmm. Maybe we should stop making the default gfp_mask be
> GFP_HIGHMEM_MOVABLE (see inode_init_always() in fs/inode.c), and
> instead add the _MOVABLE flag only for mappings that use
> "generic_file_mmap()".
> 
> It does sound a bit scary to just make default mappings use MOVABLE
> pages, when we know that can be incorrect for some cases.
> 
> But that would require that filesystems like ext4 etc that don't just
> use the generic_file_mmap() function would have add do that MOVABLE
> thing on their own. And the *normal* case is certainly that pages are
> movable and putting them in themovable zone should be ok. It's just
> *not* ok if you also map them into some hardware GTT thing or just
> otherwise keep track of the pages directly, like DRI does.
> 
> The other possibility is to just make this a shm thing, since it's
> generally that layer that has the case of "pages allocated with the
> mapping gfp masks". Hugh Dickins clearly tried to make sure that the
> DRM initialization of the gfp mask was honored, but maybe there is
> some case where it is missed. Hugh added a mapping_set_gfp_mask() to
> i915_gem_alloc_object(), but maybe we have i915 shmem mappings that
> get set up through some other path.
> 
> What about the ttm swap storage thing, for example? That also does
> shmem_file_setup(), without limiting the pages. But I don't think i915
> uses ttm, right?
> 
> Al? Hugh? Opinions? Right now anybody who uses the
> shmem_read_mapping_page() function is in danger of getting a MOVABLE
> page by default. Which may or may not be right.
> 
> That said, I don't see how i915 would get a movable page. It *seems*
> to do the right setup for the gfp flags of the mapping.

Yes, i915_gem_alloc_object() does its own mapping_set_gfp_mask(mapping,
GFP_HIGHUSER | __GFP_RECLAIMABLE), which should be as good as ever.

I've got to go out for an hour: I'll digest more and think more about
this when I get back.  If someone could explain the original problem
with _MOVABLE, that would help me: I didn't see an explanation in the
patch or in the bugzilla.  I would expect using _MOVABLE for i915_gem
would render a block of otherwise movable pages immovable because of
the GEM pages pinned, but I wouldn't expect them to move in mysterious
ways - or are references held to the pages without them being pinned??.

A factor which might turn out to be relevant: swapin_readahead() (or
swapoff) brings in pages from swap before it knows who they belong to,
so the swapped-in swapcache pages don't necessarily have the limitations
mapping_set_gfp_mask() has asked for.  It's been discussed with Alan,
we know it will need fixing for gma500 when eventually that comes to
be used on machines with more than 4GB, but for now I wasn't aware of
a problem.

Hugh