[PATCH] amdgpu:fix incorrect use on the remap_mutex

[Xie, AlexBin]

Reviewed by:  Alex Xie <AlexBin.Xie at amd.com>


Please note that this is patch is not for all open libdrm.


There is no remap_mutex in master branch of all open libdrm.


Thanks,

Alex Bin Xie

________________________________
From: Liu, Monk
Sent: Monday, April 17, 2017 11:12 AM
To: 'amd-gfx at lists.freedesktop.org'
Cc: brahma_sw_dev
Subject: 答复: [PATCH] amdgpu:fix incorrect use on the remap_mutex

Anyone review it  ?

-----邮件原件-----
发件人: Liu, Monk
发送时间: 2017年4月17日 14:16
收件人: amd-gfx at lists.freedesktop.org
主题: [PATCH] amdgpu:fix incorrect use on the remap_mutex

[PATCH] amdgpu:fix incorrect use on the remap_mutex

this patch fixed a multi-thread race issue by expand the protection range of remap_mutex to the whole LIST walk through, by this fix the CTS test:
"dEQP-VK.api.object_management.multithreaded_per_thread_device.device_memory_smal"
can always pass now,
Previously it has 1/15 chance to fail with a high performance I7 CPU under virtualization envrionment.

Change-Id: If88b9d35e79fe1cb6eb0e32c680bc4996c7915bc
Signed-off-by: Monk Liu <Monk.Liu at amd.com>
---
 amdgpu/amdgpu_bo.c    | 29 ++++++++++++++++++-----------
 amdgpu/amdgpu_vamgr.c |  5 +++--
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c index 33cb705..8960728 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -989,6 +989,8 @@ int amdgpu_bo_va_op_refcounted(amdgpu_device_handle dev,
                         return -EINVAL;
         }

+       pthread_mutex_lock(&dev->remap_mutex);
+
         /* find the previous mapped va object and its bo and unmap it*/
         if (ops == AMDGPU_VA_OP_MAP) {
                 LIST_FOR_EACH_ENTRY(vahandle, &dev->remap_list, list) { @@ -998,15 +1000,15 @@ int amdgpu_bo_va_op_refcounted(amdgpu_device_handle dev,
                                 /* the overlap va mapping which need to be unmapped first */
                                 vao = vahandle;
                                 r = amdgpu_bo_va_op(vao->bo, vao->offset, vao->size, vao->address, flags, AMDGPU_VA_OP_UNMAP);
-                               if (r)
+                               if (r) {
+                                       pthread_mutex_unlock(&dev->remap_mutex);
                                         return -EINVAL;
+                               }

                                 /* Just drop the reference. */
                                 amdgpu_bo_reference(&vao->bo, NULL);
                                 /* remove the remap from list */
-                               pthread_mutex_lock(&dev->remap_mutex);
                                 list_del(&vao->list);
-                               pthread_mutex_unlock(&dev->remap_mutex);
                                 free(vao);
                         }
                 }
@@ -1021,12 +1023,18 @@ int amdgpu_bo_va_op_refcounted(amdgpu_device_handle dev,
                         }
                 }
                 if (vao) {
-                       if (bo && (bo != vao->bo))
+                       if (bo && (bo != vao->bo)) {
+                               pthread_mutex_unlock(&dev->remap_mutex);
                                 return -EINVAL;
-               } else
+                       }
+               } else {
+                       pthread_mutex_unlock(&dev->remap_mutex);
                         return -EINVAL;
-       } else
+               }
+       } else {
+               pthread_mutex_unlock(&dev->remap_mutex);
                 return -EINVAL;
+       }

         /* we only allow null bo for unmap operation */
         if (!bo)
@@ -1039,26 +1047,25 @@ int amdgpu_bo_va_op_refcounted(amdgpu_device_handle dev,
                         /* Just drop the reference. */
                         amdgpu_bo_reference(&bo, NULL);
                         /* remove the remap from list */
-                       pthread_mutex_lock(&dev->remap_mutex);
                         list_del(&vao->list);
-                       pthread_mutex_unlock(&dev->remap_mutex);
                         free(vao);
                 } else if (ops == AMDGPU_VA_OP_MAP) {
                         /* bump the refcount of bo! */
                         atomic_inc(&bo->refcount);
                         /* add the remap to list and vao should be NULL for map */
                         vao = (struct amdgpu_va_remap*)calloc(1, sizeof(struct amdgpu_va_remap));
-                       if (!vao)
+                       if (!vao) {
+                               pthread_mutex_unlock(&dev->remap_mutex);
                                 return -ENOMEM;
+                       }
                         vao->address = addr;
                         vao->size = size;
                         vao->offset = offset;
                         vao->bo = bo;
-                       pthread_mutex_lock(&dev->remap_mutex);
                         list_add(&vao->list, &dev->remap_list);
-                       pthread_mutex_unlock(&dev->remap_mutex);
                 }
         }
+       pthread_mutex_unlock(&dev->remap_mutex);
         return r;
 }

diff --git a/amdgpu/amdgpu_vamgr.c b/amdgpu/amdgpu_vamgr.c index b3f5397..c5a7f41 100644
--- a/amdgpu/amdgpu_vamgr.c
+++ b/amdgpu/amdgpu_vamgr.c
@@ -32,6 +32,7 @@
 #include "amdgpu_drm.h"
 #include "amdgpu_internal.h"
 #include "util_math.h"
+#include "stdio.h"

 /* Devices share SVM range. So a global SVM VAM manager is needed. */  static struct amdgpu_bo_va_mgr vamgr_svm; @@ -488,6 +489,7 @@ int amdgpu_va_range_free(amdgpu_va_handle va_range_handle)
         address = va_range_handle->address;
         size    = va_range_handle->size;

+       pthread_mutex_lock(&dev->remap_mutex);
         /* clean up previous mapping if it is used for virtual allocation */
         LIST_FOR_EACH_ENTRY(vahandle, &dev->remap_list, list) {
                 /* check whether the remap list alraedy have va that overlap with current request */ @@ -500,12 +502,11 @@ int amdgpu_va_range_free(amdgpu_va_handle va_range_handle)
                         /* Just drop the reference. */
                         amdgpu_bo_reference(&vahandle->bo, NULL);
                         /* remove the remap from list */
-                       pthread_mutex_lock(&dev->remap_mutex);
                         list_del(&vahandle->list);
-                       pthread_mutex_unlock(&dev->remap_mutex);
                         free(vahandle);
                 }
         }
+       pthread_mutex_unlock(&dev->remap_mutex);

         amdgpu_vamgr_free_va(va_range_handle->vamgr,
                         va_range_handle->address,
--
2.7.4

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170417/85cd0f01/attachment-0001.html>