[Intel-gfx] [RFC] drm/i915: Add sync framework support to execbuff IOCTL

[Tvrtko Ursulin]

On 07/06/2015 04:37 PM, Daniel Vetter wrote:
> On Mon, Jul 06, 2015 at 04:21:28PM +0100, Tvrtko Ursulin wrote:
>>
>> On 07/06/2015 04:12 PM, Daniel Vetter wrote:
>>> On Mon, Jul 06, 2015 at 03:46:49PM +0100, Tvrtko Ursulin wrote:
>>
>>> If you have weak references somewhere and need to prevent the object from
>>> disappearing untimely while chasing that weak reference then imo the
>>> better design pattern is to use kref_get_unless_zero. If you need the
>>> serialization the mutex provides for some other reason (someone is only
>>> hodling the mutex instead of grabbing a proper refernce when they really
>>> should grab one) then your refcounting scheme probably needs another kind
>>> of fixup patch.
>>
>> I don't see how weak references can work since if the request goes
>> information is lost, unless stored somewhere else.
>
> So weak refernce is a pointer which doesn't hold a reference to the object
> controlled with kref, but protected by some lock. Latest when the object
> is destroyed we need to clear that pointer in the free callback of
> kref_put (and so also grab the lock that protects that pointer). The
> problem is that anyone else chasing that weak reference might race with
> the final kref_put and increase the refcount from 0 to 1, which isn't
> good.
>
> There's two ways to do that:
> - kref_put_mutex on the release side.
> - in the acquire side do a kref_get_unless_zero _while_ holding that lock.
>
> Two upsides of the later approach:
> - You can have an unrestricted amount of weak references, each protected
>    with their own lock. kref_put_mutex only works up to one.
> - It doesn't serialize the final kref_put with the lock - doing that
>    allows folks to rely on the mutex instead of a proper refcount to make
>    the obj stick around, which is imo a design antipattern that I suffered
>    through trying to cleaning it up agian a lot.
>
> But that's really a tangent to the discussion here, I have no idea whether
> this applies here since I didn't read your patch in detail.

I think it is not about my patch but whether the Android native sync 
implementation handles losing the weak reference on a fence.

I don't think it does and I am not sure how easy or hard would it be to 
change it right now. (The whole area traditionally confuses me since 
there are too many things called sync something and fence something.)

I would need to handle it to be able at least report the status of a 
fence to userspace and gracefully fail other operations. And that is 
assuming that wouldn't break existing userspace.

Regards,

Tvrtko