[Intel-gfx] [PATCH] drm/i915: Rewrite some comments around RCU-deferred object free

[Tvrtko Ursulin]

On 15/01/2018 20:57, Chris Wilson wrote:
> Tvrtko noticed that the comments describing the interaction of RCU and
> the deferred worker for freeing drm_i915_gem_object were a little
> confusing, so attempt to bring some sense to them.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 87937c4f9dff..98bacaee2720 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4699,7 +4699,8 @@ static void __i915_gem_free_work(struct work_struct *work)
>   		container_of(work, struct drm_i915_private, mm.free_work);
>   	struct llist_node *freed;
>   
> -	/* All file-owned VMA should have been released by this point through
> +	/*
> +	 * All file-owned VMA should have been released by this point through
>   	 * i915_gem_close_object(), or earlier by i915_gem_context_close().
>   	 * However, the object may also be bound into the global GTT (e.g.
>   	 * older GPUs without per-process support, or for direct access through
> @@ -4726,10 +4727,15 @@ static void __i915_gem_free_object_rcu(struct rcu_head *head)
>   		container_of(head, typeof(*obj), rcu);
>   	struct drm_i915_private *i915 = to_i915(obj->base.dev);
>   
> -	/* We can't simply use call_rcu() from i915_gem_free_object()
> -	 * as we need to block whilst unbinding, and the call_rcu
> -	 * task may be called from softirq context. So we take a
> -	 * detour through a worker.
> +	/*
> +	 * Since we require blocking on struct_mutex to unbind the freed
> +	 * object from the GPU before releasing resources back to the
> +	 * system, we can not do that directly from the RCU callback (which may
> +	 * be a softirq context), but must intend then defer that work onto a
> +	 * kthread. We use the RCU callback rather than move the freed object
> +	 * directly onto the work queue so that we can mix between using the
> +	 * worker and performing frees directly from subsequent allocations for
> +	 * crude but effective memory throttling.
>   	 */
>   	if (llist_add(&obj->freed, &i915->mm.free_list))
>   		queue_work(i915->wq, &i915->mm.free_work);
> @@ -4745,7 +4751,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>   	if (discard_backing_storage(obj))
>   		obj->mm.madv = I915_MADV_DONTNEED;
>   
> -	/* Before we free the object, make sure any pure RCU-only
> +	/*
> +	 * Before we free the object, make sure any pure RCU-only
>   	 * read-side critical sections are complete, e.g.
>   	 * i915_gem_busy_ioctl(). For the corresponding synchronized
>   	 * lookup see i915_gem_object_lookup_rcu().
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko