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