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.
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. > diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c > index d89a98d2c37b..ca6ef0c4b358 100644 > --- a/drivers/base/dma-buf.c > +++ b/drivers/base/dma-buf.c > > +int reservation_object_get_fences_rcu(struct reservation_object *obj, > + struct fence **pfence_excl, > + unsigned *pshared_count, > + struct fence ***pshared) > +{ > + unsigned shared_count = 0; > + unsigned retry = 1; > + struct fence **shared = NULL, *fence_excl = NULL; > + int ret = 0; > + > + while (retry) { > + struct reservation_object_list *fobj; > + unsigned seq, retry; > You're shadowing retry? Oops. > >> + >> + seq = read_seqcount_begin(&obj->seq); >> + >> + rcu_read_lock(); >> + >> + fobj = ACCESS_ONCE(obj->fence); >> + if (fobj) { >> + struct fence **nshared; >> + >> + shared_count = ACCESS_ONCE(fobj->shared_count); >> + nshared = krealloc(shared, sizeof(*shared) * >> shared_count, GFP_KERNEL); > krealloc inside rcu_read_lock(). Better to put this first in the loop. Except that shared_count isn't known until the rcu_read_lock is taken. > Thanks, > Thomas ~Maarten