Re: [PATCH 1/2] dma-buf: make reservation_object_copy_fences rcu save

Am 11.09.2017 um 15:56 schrieb Maarten Lankhorst:

Op 11-09-17 om 14:53 schreef Christian König:

...

Am 10.09.2017 um 09:30 schrieb Maarten Lankhorst:

...

Op 04-09-17 om 21:02 schreef Christian König:

...

From: Christian König christian.koenig@amd.com

Stop requiring that the src reservation object is locked for this operation.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/reservation.c | 56 ++++++++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 14 deletions(-)

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index dec3a81..b44d9d7 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -266,8 +266,7 @@ EXPORT_SYMBOL(reservation_object_add_excl_fence);

• @dst: the destination reservation object
• @src: the source reservation object

-* Copy all fences from src to dst. Both src->lock as well as dst-lock must be -* held. +* Copy all fences from src to dst. dst-lock must be held. */ int reservation_object_copy_fences(struct reservation_object *dst, struct reservation_object *src)

Could this be implemented using reservation_object_get_fences_rcu? You're essentially duplicating its functionality.

I've considered this as well, but reservation_object_get_fences_rcu() returns an array and here we need an reservation_object_list.

Doesn't seem too hard, below is the result from fiddling with the allocation function.. reservation_object_list is just a list of fences with some junk in front.

Here you go, but only tested with the compiler. O:) -----8<----- drivers/dma-buf/reservation.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------- 1 file changed, 109 insertions(+), 83 deletions(-)

To be honest that looks rather ugly to me for not much gain.

Additional to that we loose the optimization I've stolen from the wait function.

Regards, Christian.

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c index dec3a815455d..72ee91f34132 100644 --- a/drivers/dma-buf/reservation.c +++ b/drivers/dma-buf/reservation.c @@ -261,87 +261,43 @@ void reservation_object_add_excl_fence(struct reservation_object *obj, } EXPORT_SYMBOL(reservation_object_add_excl_fence);

-/** -* reservation_object_copy_fences - Copy all fences from src to dst. -* @dst: the destination reservation object -* @src: the source reservation object -* -* Copy all fences from src to dst. Both src->lock as well as dst-lock must be -* held. -*/ -int reservation_object_copy_fences(struct reservation_object *dst,

• 		   struct reservation_object *src)
  

+static bool realloc_shared(void **ptr, struct dma_fence ***pshared, unsigned shared_max, bool alloc_list, bool unlocked) {

• struct reservation_object_list *src_list, *dst_list;
• struct dma_fence *old, *new;
• size_t size;
• unsigned i;
• src_list = reservation_object_get_list(src);
• if (src_list) {

• size = offsetof(typeof(*src_list),
  

• 		shared[src_list->shared_count]);
  

• dst_list = kmalloc(size, GFP_KERNEL);
  

• if (!dst_list)
  

• 	return -ENOMEM;
  

• dst_list->shared_count = src_list->shared_count;
  

• dst_list->shared_max = src_list->shared_count;
  

• for (i = 0; i < src_list->shared_count; ++i)
  

• 	dst_list->shared[i] =
  

• 		dma_fence_get(src_list->shared[i]);
  

• } else {

• dst_list = NULL;
  

• }
• kfree(dst->staged);
• dst->staged = NULL;

• size_t sz;
• void *newptr;

• src_list = reservation_object_get_list(dst);

• if (alloc_list)

• sz = offsetof(struct reservation_object_list, shared[shared_max]);
  

• else

• sz = shared_max * sizeof(struct dma_fence *);
  
  

• old = reservation_object_get_excl(dst);
• new = reservation_object_get_excl(src);

• newptr = krealloc(*ptr, sz, unlocked ? GFP_KERNEL : GFP_NOWAIT | __GFP_NOWARN);
• if (!newptr)

• return false;
  
  

• dma_fence_get(new);

• *ptr = newptr;
• if (alloc_list) {

• struct reservation_object_list *fobj = newptr;
  
  

• preempt_disable();
• write_seqcount_begin(&dst->seq);
• /* write_seqcount_begin provides the necessary memory barrier */
• RCU_INIT_POINTER(dst->fence_excl, new);
• RCU_INIT_POINTER(dst->fence, dst_list);
• write_seqcount_end(&dst->seq);
• preempt_enable();
• if (src_list)

• kfree_rcu(src_list, rcu);
  

• dma_fence_put(old);

• fobj->shared_max = shared_max;
  

• *pshared = fobj->shared;
  

• } else

• *pshared = newptr;
  
  

• return 0;

• return true; }

-EXPORT_SYMBOL(reservation_object_copy_fences);

-/**

• • reservation_object_get_fences_rcu - Get an object's shared and exclusive
• • fences without update side lock held
• • @obj: the reservation object
• • @pfence_excl: the returned exclusive fence (or NULL)
• • @pshared_count: the number of shared fences returned
• • @pshared: the array of shared fence ptrs returned (array is krealloc'd to
• • the required size, and must be freed by caller)
• • RETURNS
• • Zero or -errno
• */

-int reservation_object_get_fences_rcu(struct reservation_object *obj, +static int __reservation_object_get_fences_rcu(struct reservation_object *obj, struct dma_fence **pfence_excl, unsigned *pshared_count,

• 		      struct dma_fence ***pshared)
  

• 		      struct dma_fence ***pshared,
  

• 		      struct reservation_object_list **list_shared)
  

  { struct dma_fence **shared = NULL; struct dma_fence *fence_excl; unsigned int shared_count; int ret = 1;

• void *ptr = NULL;

  do { struct reservation_object_list *fobj;

@@ -359,23 +315,16 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj,

fobj = rcu_dereference(obj->fence);
if (fobj) {

• 	struct dma_fence **nshared;
  

• 	size_t sz = sizeof(*shared) * fobj->shared_max;
  

• 	nshared = krealloc(shared, sz,
  

• 			   GFP_NOWAIT | __GFP_NOWARN);
  

• 	if (!nshared) {
  

• 	if (!realloc_shared(&ptr, &shared, fobj->shared_max, list_shared, false)) {
  	rcu_read_unlock();
  

• 		nshared = krealloc(shared, sz, GFP_KERNEL);
  

• 		if (nshared) {
  

• 			shared = nshared;
  

• 			continue;
  

• 		if (!realloc_shared(&ptr, &shared, fobj->shared_max, list_shared, true)) {
  

• 			ret = -ENOMEM;
  

• 			break;
  	}
  
  

• 		ret = -ENOMEM;
  

• 		break;
  

• 		/* need to acquire rcu lock again and retry */
  

• 		continue;
  }
  

• 	shared = nshared;
  shared_count = fobj->shared_count;
  
  for (i = 0; i < shared_count; ++i) {
  

@@ -398,16 +347,93 @@ int reservation_object_get_fences_rcu(struct reservation_object *obj, } while (ret);

if (!shared_count) {

• kfree(shared);
  

• kfree(ptr);
  

• ptr = NULL;
  

  shared = NULL; }

• *pshared_count = shared_count;
• *pshared = shared;
• *pfence_excl = fence_excl;

• if (!list_shared) {

• *pshared_count = shared_count;
  

• *pshared = shared;
  

• } else {

• *list_shared = ptr;
  

• if (ptr)
  

• 	(*list_shared)->shared_count = shared_count;
  

• }

• *pfence_excl = fence_excl; return ret; }

+/** +* reservation_object_copy_fences - Copy all fences from src to dst. +* @dst: the destination reservation object +* @src: the source reservation object +* +* Copy all fences from src to dst. dst-lock must be held. +*/ +int reservation_object_copy_fences(struct reservation_object *dst,

• 		   struct reservation_object *src)
  

+{

• struct reservation_object_list *old_list, *dst_list;
• struct dma_fence *excl, *old;
• unsigned i;
• int ret;
• ret = __reservation_object_get_fences_rcu(src, &excl, NULL,

• 				  NULL, &dst_list);
  

• if (ret)

• return ret;
  

• kfree(dst->staged);
• dst->staged = NULL;
• old = rcu_dereference_protected(dst->fence_excl,

• 			reservation_object_held(dst));
  

• old_list = rcu_dereference_protected(dst->fence,

• 			     reservation_object_held(dst));
  

• preempt_disable();
• write_seqcount_begin(&dst->seq);
• /* write_seqcount_begin provides the necessary memory barrier */
• RCU_INIT_POINTER(dst->fence_excl, excl);
• RCU_INIT_POINTER(dst->fence, dst_list);
• write_seqcount_end(&dst->seq);
• preempt_enable();
• if (old_list) {

• for (i = 0; i < old_list->shared_count; i++)
  

• 	dma_fence_put(old_list->shared[i]);
  

• kfree_rcu(old_list, rcu);
  

• }
• dma_fence_put(old);
• return 0;

+} +EXPORT_SYMBOL(reservation_object_copy_fences);

+/**

• • reservation_object_get_fences_rcu - Get an object's shared and exclusive
• • fences without update side lock held
• • @obj: the reservation object
• • @pfence_excl: the returned exclusive fence (or NULL)
• • @pshared_count: the number of shared fences returned
• • @pshared: the array of shared fence ptrs returned (array is krealloc'd to
• • the required size, and must be freed by caller)
• • RETURNS
• • Zero or -errno
• */

+int reservation_object_get_fences_rcu(struct reservation_object *obj,

• 		      struct dma_fence **pfence_excl,
  

• 		      unsigned *pshared_count,
  

• 		      struct dma_fence ***pshared)
  

+{

• return __reservation_object_get_fences_rcu(obj, pfence_excl, pshared_count, pshared, NULL);

+} EXPORT_SYMBOL_GPL(reservation_object_get_fences_rcu);

/**

---

amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx