Messages in this thread | | | Date | Mon, 14 Apr 2014 09:42:29 +0200 | From | Maarten Lankhorst <> | Subject | Re: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu |
| |
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. This is harmless because the shared fence pointers are still valid long enough because of RCU delayed deletion. fence_get_rcu will fail when the refcount has dropped to zero. This is enough of a check to prevent errors, so there's no need to explicitly clear the fence pointers.
~Maarten
| |