Re: [PATCH 2/4] drm/i915: Mark the execbuffer validation failures as unlikely

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Aug 10, 2014 at 06:29:09AM +0100, Chris Wilson wrote:
> This just allows the compiler to pessimise callers who try to abuse the
> ioctl in the hope of making the correct users faster.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I'm not that much of a fan of likely/unlikely really. If it helps with
documenting code then I'm ok, but mass-sprinkling looks like too much.

For this case here I think an unlikely on the return value of
validate_exec_list is about all I want to stomach. gcc should reach all
the other conclusions itself.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 36 +++++++++++++-----------------
>  1 file changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 4304d8b9e17c..0ba1e7bbd09d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -870,41 +870,38 @@ i915_gem_execbuffer_move_to_gpu(struct intel_engine_cs *ring,
>  	return intel_ring_invalidate_all_caches(ring);
>  }
>  
> -static bool
> -i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec)
> -{
> -	if (exec->flags & __I915_EXEC_UNKNOWN_FLAGS)
> -		return false;
> -
> -	return ((exec->batch_start_offset | exec->batch_len) & 0x7) == 0;
> -}
> -
>  static int
>  validate_exec_list(struct drm_device *dev,
> -		   struct drm_i915_gem_exec_object2 *exec,
> -		   int count)
> +		   const struct drm_i915_gem_execbuffer2 *args,
> +		   const struct drm_i915_gem_exec_object2 *exec)
>  {
>  	unsigned relocs_total = 0;
>  	unsigned relocs_max = UINT_MAX / sizeof(struct drm_i915_gem_relocation_entry);
>  	unsigned invalid_flags;
>  	int i;
>  
> +	if (unlikely(args->flags & __I915_EXEC_UNKNOWN_FLAGS))
> +		return -EINVAL;
> +
> +	if (unlikely((args->batch_start_offset | args->batch_len) & 0x7))
> +		return -EINVAL;
> +
>  	invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
>  	if (USES_FULL_PPGTT(dev))
>  		invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
>  
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < args->buffer_count; i++) {
>  		char __user *ptr = to_user_ptr(exec[i].relocs_ptr);
>  		int length; /* limited by fault_in_pages_readable() */
>  
> -		if (exec[i].flags & invalid_flags)
> +		if (unlikely(exec[i].flags & invalid_flags))
>  			return -EINVAL;
>  
>  		/* First check for malicious input causing overflow in
>  		 * the worst case where we need to allocate the entire
>  		 * relocation tree as a single array.
>  		 */
> -		if (exec[i].relocation_count > relocs_max - relocs_total)
> +		if (unlikely(exec[i].relocation_count > relocs_max - relocs_total))
>  			return -EINVAL;
>  		relocs_total += exec[i].relocation_count;
>  
> @@ -915,11 +912,11 @@ validate_exec_list(struct drm_device *dev,
>  		 * to read, but since we may need to update the presumed
>  		 * offsets during execution, check for full write access.
>  		 */
> -		if (!access_ok(VERIFY_WRITE, ptr, length))
> +		if (unlikely(!access_ok(VERIFY_WRITE, ptr, length)))
>  			return -EFAULT;
>  
>  		if (likely(!i915.prefault_disable)) {
> -			if (fault_in_multipages_readable(ptr, length))
> +			if (unlikely(fault_in_multipages_readable(ptr, length)))
>  				return -EFAULT;
>  		}
>  	}
> @@ -1256,11 +1253,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  	int ret;
>  	bool need_relocs;
>  
> -	if (!i915_gem_check_execbuffer(args))
> -		return -EINVAL;
> -
> -	ret = validate_exec_list(dev, exec, args->buffer_count);
> -	if (ret)
> +	ret = validate_exec_list(dev, args, exec);
> +	if (unlikely(ret))
>  		return ret;
>  
>  	flags = 0;
> -- 
> 2.1.0.rc1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch