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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx