From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maarten Lankhorst Subject: Re: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu Date: Fri, 11 Apr 2014 11:24:53 +0200 Message-ID: <5347B4E5.6090901@canonical.com> References: <20140409144239.26648.57918.stgit@patser> <20140409144831.26648.79163.stgit@patser> <53465A53.1090500@vmware.com> <53466D63.8080808@canonical.com> <53467B93.3000402@vmware.com> <5346B212.8050202@canonical.com> <5347A9FD.2070706@vmware.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5347A9FD.2070706@vmware.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Thomas Hellstrom Cc: linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, ccross@google.com, linux-media@vger.kernel.org List-Id: linux-arch.vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from youngberry.canonical.com ([91.189.89.112]:48870 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756250AbaDKJY5 (ORCPT ); Fri, 11 Apr 2014 05:24:57 -0400 Message-ID: <5347B4E5.6090901@canonical.com> Date: Fri, 11 Apr 2014 11:24:53 +0200 From: Maarten Lankhorst MIME-Version: 1.0 Subject: Re: [PATCH 2/2] [RFC v2 with seqcount] reservation: add suppport for read-only access using rcu References: <20140409144239.26648.57918.stgit@patser> <20140409144831.26648.79163.stgit@patser> <53465A53.1090500@vmware.com> <53466D63.8080808@canonical.com> <53467B93.3000402@vmware.com> <5346B212.8050202@canonical.com> <5347A9FD.2070706@vmware.com> In-Reply-To: <5347A9FD.2070706@vmware.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Thomas Hellstrom Cc: linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, ccross@google.com, linux-media@vger.kernel.org Message-ID: <20140411092453.QnuvsPGygWrLlnijvDNGMxe1j6QIB_3FhU8DiEvdYhM@z> 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