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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>

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

Signed-off-by: Christian König <christian.koenig@xxxxxxx>
---
   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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux