Re: [PATCH] drm/i915: Selectively enable self-reclaim

[Linus Torvalds]

On Wed, 27 Jan 2010, Chris Wilson wrote:
>  
> +	gfp = i915_gem_object_get_page_gfp_mask(obj);
> +	i915_gem_object_set_page_gfp_mask(obj, gfp | __GFP_NORETRY | __GFP_NOWARN);
>  	ret = i915_gem_object_get_pages(obj);
> +	i915_gem_object_set_page_gfp_mask (obj, gfp);

This is just all still totally and utterly broken.

There is no possible reason why something like drm should _ever_ play with 
the mapping gfp-mask. The fact that you guys do means that something is 
seriously wrong.

Setting the mapping gfp mask like that is totally wrong. Yes, it looks 
like you take the 'struct_mutex' lock, but I don't think the page fault 
does that, does it? So the locking in no way protects other uses of that 
mapping gfp mask.

Guys, this is just not acceptable. Playing games like this with obvious 
crap code is not the way to fix any problems what-so-ever. Even if the 
code happens to work, just a quick look at it clearly says "oh, that's 
ugly".

Just pass in the GFP mask to i915_gem_object_get_pages() instead, and stop 
making up these sh*tty idiotic interfaces. 

The whole "mapping_set_gfp_mask()" function that you use is meant to 
INITIALIZE a mapping when it is created. It's simply not safe at run-time. 
It never was, and it never will be.

So get rid of that whole i915_gem_object_set_page_gfp_mask() function - 
any user is by definition pure and utter crap.

Now, if you want to pass the gfp mask to the allocator directly (and 
judging by the code, that's _exactly_ what you want to do), then you don't 
want to use the read_mapping_page() helper function. That is very much 
designed to take the mappign GFP.

You can do your own version, something like this:

  struct page *i915_gem_get_page(struct address_space *mapping, pgoff_t index)
  {
	for (;;) {
		int err; 
		struct page *page;

		page = find_get_page(mapping, index);
		if (page)
			return page;

		page = __page_cache_alloc(gfpmask | __GFP_ZERO);
		if (!page)
			return ERR_PTR(-ENOMEM);

		/* Zero-page is already uptodate */
		__SetPageUptodate(page);

		err = add_to_page_cache_lru(page, mapping, index, GFP_KERNEL);
		if (err) {
			page_cache_release(page);
			if (err == -EEXIST)
				continue;
			return ERR_PTR(err);
		}

		return page;
	}
  }

and now you have a way to allocate cleared pages into that mapping with a 
GFP that you control.

We could even make it a generic helper function if we just did that whole 
"mapping->a_ops->readpage()" dance or whatever and add the required async 
waiting etc. But I think gem just basically wants a zero-initialized 
mapping, no?

THE ABOVE IS TOTALLY UNTESTED. I wrote it in the mail-reader. It hasn't 
seen a compiler, and I didn't actually check that gem really just wants 
pages that are initialized to zero (aka "simle_readpage"), so what the 
heck do I know. My point is that something like the above should work in 
the sense that it avoids the whole "play games with setting a global 
gfpmask" thing, and keeps the gfpmask local to the execution.

Oh, and if you actually use shmem/tmpfs and can swap your pages out, then 
you do need to do the whole readpage() thing. It gets more complex then 
(you can't just mark things up-to-date, you need to lock the page, check 
PageUptodate() and friends etc etc).

I didn't check where that 'mapping' thing gets initialized.

Another alternative would be to keep the current i915_gem logic, but pass 
the gfp down all the way to read_cache_page(), rather than take it from 
the mapping. Then we'd make the 'read_mapping_page()' function look up the 
gfp (so users of 'read_mapping_page()' would get the old behavior, but we 
could call read_cache_page() with a gfpmask that is different from the 
mapping->gfpmask).

			Linus
--
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/