On Mon, Jul 06, 2015 at 05:34:22PM +0100, Tvrtko Ursulin wrote: > > 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. For reporting status it should grab a strong reference. I thought this was about i915-internal lists and stuff we use to keep track of our own requests, for which we'd need struct_mutex. struct_mutex won't protect any of the syncpt internal state. I guess I'm rather confused now what this is about ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
