Re: [RFC 6/9] drm/i915: Delay the freeing of requests until retire time

Tvrtko Ursulin
2015-10-28T13:42:47Z

On 28/10/15 13:00, John Harrison wrote:
> On 23/07/2015 15:25, Tvrtko Ursulin wrote:
>>
>> Hi,
>>
>> On 07/17/2015 03:31 PM, John.C.Harrison@Intel.com wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> The request structure is reference counted. When the count reached
>>> zero, the request was immediately freed and all associated objects
>>> were unrefereced/unallocated. This meant that the driver mutex lock
>>> must be held at the point where the count reaches zero. This was fine
>>> while all references were held internally to the driver. However, the
>>> plan is to allow the underlying fence object (and hence the request
>>> itself) to be returned to other drivers and to userland. External
>>> users cannot be expected to acquire a driver private mutex lock.
>>>
>>> Rather than attempt to disentangle the request structure from the
>>> driver mutex lock, the decsion was to defer the free code until a
>>> later (safer) point. Hence this patch changes the unreference callback
>>> to merely move the request onto a delayed free list. The driver's
>>> retire worker thread will then process the list and actually call the
>>> free function on the requests.
>>>
>>> [new patch in series]
>>>
>>> For: VIZ-5190
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_drv.h | 22 +++---------------
>>> drivers/gpu/drm/i915/i915_gem.c | 41
>>> +++++++++++++++++++++++++++++----
>>> drivers/gpu/drm/i915/intel_display.c | 2 +-
>>> drivers/gpu/drm/i915/intel_lrc.c | 2 ++
>>> drivers/gpu/drm/i915/intel_pm.c | 2 +-
>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 4 ++++
>>> 7 files changed, 50 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 88a4746..61c3db2 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2161,14 +2161,9 @@ void i915_gem_track_fb(struct
>>> drm_i915_gem_object *old,
>>> * initial reference taken using kref_init
>>> */
>>> struct drm_i915_gem_request {
>>> - /**
>>> - * Underlying object for implementing the signal/wait stuff.
>>> - * NB: Never return this fence object to user land! It is unsafe to
>>> - * let anything outside of the i915 driver get hold of the fence
>>> - * object as the clean up when decrementing the reference count
>>> - * requires holding the driver mutex lock.
>>> - */
>>> + /** Underlying object for implementing the signal/wait stuff. */
>>> struct fence fence;
>>> + struct list_head delay_free_list;
>>
>> Maybe call this delay_free_link to continue the established convention.
>>
>>>
>>> /** On Which ring this request was generated */
>>> struct drm_i915_private *i915;
>>> @@ -2281,21 +2276,10 @@ i915_gem_request_reference(struct
>>> drm_i915_gem_request *req)
>>> static inline void
>>> i915_gem_request_unreference(struct drm_i915_gem_request *req)
>>> {
>>> - WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
>>> - fence_put(&req->fence);
>>> -}
>>> -
>>> -static inline void
>>> -i915_gem_request_unreference__unlocked(struct drm_i915_gem_request
>>> *req)
>>> -{
>>> - struct drm_device *dev;
>>> -
>>> if (!req)
>>> return;
>>>
>>> - dev = req->ring->dev;
>>> - if (kref_put_mutex(&req->fence.refcount, fence_release,
>>> &dev->struct_mutex))
>>> - mutex_unlock(&dev->struct_mutex);
>>> + fence_put(&req->fence);
>>> }
>>>
>>> static inline void i915_gem_request_assign(struct
>>> drm_i915_gem_request **pdst,
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>>> b/drivers/gpu/drm/i915/i915_gem.c
>>> index af79716..482835a 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -2616,10 +2616,27 @@ static void i915_set_reset_status(struct
>>> drm_i915_private *dev_priv,
>>> }
>>> }
>>>
>>> -static void i915_gem_request_free(struct fence *req_fence)
>>> +static void i915_gem_request_release(struct fence *req_fence)
>>> {
>>> struct drm_i915_gem_request *req = container_of(req_fence,
>>> typeof(*req), fence);
>>> + struct intel_engine_cs *ring = req->ring;
>>> + struct drm_i915_private *dev_priv = to_i915(ring->dev);
>>> + unsigned long flags;
>>> +
>>> + /*
>>> + * Need to add the request to a deferred dereference list to be
>>> + * processed at a mutex lock safe time.
>>> + */
>>> + spin_lock_irqsave(&ring->delayed_free_lock, flags);
>>
>> At the moment there is no request unreferencing from irq handlers
>> right? Unless (or until) you plan to add that you could use simple
>> spin_lock here. (And in the i915_gem_retire_requests_ring.)
>
> I don't believe there is an unreference at IRQ time at this precise
> moment. However, there certainly have been in various other iterations
> of the code (including one on the display side that has since
> disappeared due to changes by others completely unrelated to this work).
> So I would be nervous about not making it IRQ compatible. It seems like
> a bug waiting to happen.

I think it is bad to take the cost of disabling interrupts for nothing.

Once the unthinkable happens and driver is re-designed so that it is
possible to unreference from IRQ context it could be added.

>>> @@ -2992,6 +3009,21 @@ i915_gem_retire_requests_ring(struct
>>> intel_engine_cs *ring)
>>> i915_gem_request_assign(&ring->trace_irq_req, NULL);
>>> }
>>>
>>> + while (!list_empty(&ring->delayed_free_list)) {
>>> + struct drm_i915_gem_request *request;
>>> + unsigned long flags;
>>> +
>>> + request = list_first_entry(&ring->delayed_free_list,
>>> + struct drm_i915_gem_request,
>>> + delay_free_list);
>>
>> Need a spinlock to sample list head here. Then maybe move it on a
>> temporary list and do the freeing afterwards.
>
> Not necessary. The only other usage of the list is to add to it. So this
> code can't pull an entry that gets removed beneath its feet. Either the
> list empty test will return true and nothing further happens or there is
> definitely a node on the list and list_first_entry() will return
> something sane. The spinlock is only required when actually deleting
> that node.

NAK! :D

It only works because you know how lists are implemented. Say if
list_empty checked for head->prev == head, and list_first_entry
obviously uses head->next, then depending on ordering in list_add, you
could grab some garbage, take the lock and dereference that garbage.

I see no gain in doing this trickery and it is fragile. And you still
lock/unlock once per loop.

Why not use the common pattern of replacing the list under the lock and
then operating on your local copy unrestricted?

Regards,

Tvrtko