Hi! On 04/11/2014 08:09 PM, Maarten Lankhorst wrote: > op 11-04-14 12:11, Thomas Hellstrom schreef: >> On 04/11/2014 11:24 AM, Maarten Lankhorst wrote: >>> op 11-04-14 10:38, Thomas Hellstrom schreef: >>>> Hi, Maarten. >>>> >>>> Here I believe we encounter a lot of locking inconsistencies. >>>> >>>> First, it seems you're use a number of pointers as RCU pointers >>>> without >>>> annotating them as such and use the correct rcu >>>> macros when assigning those pointers. >>>> >>>> Some pointers (like the pointers in the shared fence list) are both >>>> used >>>> as RCU pointers (in dma_buf_poll()) for example, >>>> or considered protected by the seqlock >>>> (reservation_object_get_fences_rcu()), which I believe is OK, but then >>>> the pointers must >>>> be assigned using the correct rcu macros. In the memcpy in >>>> reservation_object_get_fences_rcu() we might get away with an >>>> ugly typecast, but with a verbose comment that the pointers are >>>> considered protected by the seqlock at that location. >>>> >>>> So I've updated (attached) the headers with proper __rcu annotation >>>> and >>>> locking comments according to how they are being used in the various >>>> reading functions. >>>> I believe if we want to get rid of this we need to validate those >>>> pointers using the seqlock as well. >>>> This will generate a lot of sparse warnings in those places needing >>>> rcu_dereference() >>>> rcu_assign_pointer() >>>> rcu_dereference_protected() >>>> >>>> With this I think we can get rid of all ACCESS_ONCE macros: It's not >>>> needed when the rcu_x() macros are used, and >>>> it's never needed for the members protected by the seqlock, (provided >>>> that the seq is tested). The only place where I think that's >>>> *not* the case is at the krealloc in >>>> reservation_object_get_fences_rcu(). >>>> >>>> Also I have some more comments in the >>>> reservation_object_get_fences_rcu() function below: >>> I felt that the barriers needed for rcu were already provided by >>> checking the seqcount lock. >>> But looking at rcu_dereference makes it seem harmless to add it in >>> more places, it handles >>> the ACCESS_ONCE and barrier() for us. >> And it makes the code more maintainable, and helps sparse doing a lot of >> checking for us. I guess >> we can tolerate a couple of extra barriers for that. >> >>> We could probably get away with using RCU_INIT_POINTER on the writer >>> side, >>> because the smp_wmb is already done by arranging seqcount updates >>> correctly. >> Hmm. yes, probably. At least in the replace function. I think if we do >> it in other places, we should add comments as to where >> the smp_wmb() is located, for future reference. >> >> >> Also I saw in a couple of places where you're checking the shared >> pointers, you're not checking for NULL pointers, which I guess may >> happen if shared_count and pointers are not in full sync? >> > No, because shared_count is protected with seqcount. I only allow > appending to the array, so when > shared_count is validated by seqcount it means that the > [0...shared_count) indexes are valid and non-null. > What could happen though is that the fence at a specific index is > updated with another one from the same > context, but that's harmless. Hmm. Shouldn't we have a way to clean signaled fences from reservation objects? Perhaps when we attach a new fence, or after a wait with ww_mutex held? Otherwise we'd have a lot of completely unused fence objects hanging around for no reason. I don't think we need to be as picky as TTM, but I think we should do something? /Thomas > > ~Maarten -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
