[PATCH v3 2/4] drm/i915: refactor i915_gem_object_pin_map()

[Dave Gordon]

On 20/04/16 21:02, Chris Wilson wrote:
> On Wed, Apr 20, 2016 at 03:35:45PM +0100, Dave Gordon wrote:
>> The recently-added i915_gem_object_pin_map() can be further optimised
>> for "small" objects. To facilitate this, and simplify the error paths
>> before adding the new code, this patch pulls out the "mapping" part of
>> the operation (involving local allocations which must be undone before
>> return) into its own subfunction.
>>
>> The next patch will then insert the new optimisation into the middle of
>> the now-separated subfunction.
>>
>> This reorganisation will probably not affect the generated code, as the
>> compiler will most likely inline it anyway, but it makes the logical
>> structure a bit clearer and easier to modify.
>>
>> v2:
>>      Restructure loop-over-pages & error check (Chris Wilson)
>>
>> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 58 ++++++++++++++++++++++++++---------------
>>   1 file changed, 37 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 261a3ef..98b6486 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2388,6 +2388,42 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
>>   	return 0;
>>   }
>>
>> +/* The 'mapping' part of i915_gem_object_pin_map() below */
>> +static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
>> +{
>> +	unsigned long n_pages = obj->base.size >> PAGE_SHIFT;
>> +	struct scatterlist *sg = obj->pages->sgl;
>> +	struct sg_page_iter sg_iter;
>> +	struct page **pages;
>> +	unsigned long i = 0;
>> +	void *addr = NULL;
>> +
>> +	/* A single page can always be kmapped */
>> +	if (n_pages == 1)
>> +		return kmap(sg_page(sg));
>> +
>> +	pages = drm_malloc_gfp(n_pages, sizeof(*pages), GFP_TEMPORARY);
>> +	if (pages == NULL) {
>> +		DRM_DEBUG_DRIVER("Failed to get space for pages\n");
>
> The oom and subsequent ENOMEM to userspace (if it survives) is not enough?

I don't want to assume anything about OOM behaviour; it may not even be 
configured. And there's no reason for this code to assume there's any 
connection between the object to be mapped and anything in userspace.

>> +		return NULL;
>> +	}
>> +
>
> Please put the loop counter here so I can see it is correct.

Presumably you mean in the DEBUG message above?
Yes, that would be more helpful; if the log says
  "Failed to get space for 256287654 pages"
you might suspect the system wasn't really low on memory.

>> +	for_each_sg_page(sg, &sg_iter, n_pages, 0)
>
> This must be nents. Ah. I see why you needed so much error detection
> now.

If the SGL contains multi-page chunks, nents < npages (but would still 
result in iterating over all pages). If the GEM allocator has allocated 
more pages than are strictly required for the object size (and I don't 
want to make any assumptions about that) then possibly nents > npages. 
If both apply it could (accidentally) be equal!

This is why I previously had the loop-termination check inside the loop 
rather than relying on the iterator!

> A simpler iterator for this case would be
> https://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=tasklet&id=9db86845416ffc8c9d41fc187b0de6fd015cb319

I've already written a (different) simple iterator which I'm hoping to 
post soon ... but yes, the current macro is really not user-friendly.

>> +		pages[i++] = sg_page_iter_page(&sg_iter);
>> +
>> +	/* Check that we have the expected number of pages */
>> +	if (!WARN_ON(i != n_pages))
>
> We've already trampled over the end of the buffer because of code we
> wrote (for_each_sg_page) is broken? Job for ubsan/kasan/kmemcheck to
> identify the culprit, because it is very unlikely to be here.

No, that wasn't the intent; I was actually trying to catch EARLY 
termination of the loop, rather than overrun. We don't want to do the 
vmap() if the array hasn't been completely filled.

>> +		addr = vmap(pages, n_pages, 0, PAGE_KERNEL);
>> +
>> +	if (addr == NULL)
>> +		DRM_DEBUG_DRIVER("Failed to vmap pages\n");
>
> More debug after lower level WARNs.
> -Chris

The debug also triggers if the loop is correct but the vmap fails.
Printing the message after the WARN is merely incidental, but we can't 
return early from an if (WARN_ON(...)) 'cos we still have to release the 
pages[]. And a goto would be ugly.

.Dave.