On 04/14/2014 09:42 AM, Maarten Lankhorst wrote: > op 11-04-14 21:35, Thomas Hellstrom schreef: >> 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, doesn't attaching an exclusive fence clear all shared fence >> pointers from under a reader? >> > No, for that reason. It only resets shared_count to 0. Ah. OK. I guess I didn't read the code carefully enough. Thanks, 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
