amdgpu:fix incorrect use on the remap_mutex

Submitted by Xie, AlexBin on April 17, 2017, 9:12 p.m.

Details

Message ID MWHPR1201MB0045DC3C5C6F86F4CF9CDB56F2060@MWHPR1201MB0045.namprd12.prod.outlook.com
State New
Headers show
Series "amdgpu:fix incorrect use on the remap_mutex" ( rev: 4 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Xie, AlexBin April 17, 2017, 9:12 p.m.
Hi Monk,


Correction:


Please remove the following line in the patch:

+#include "stdio.h"


This patch does not need to use stdio.h.

With that fixed, Reviewed by:  Alex Xie <AlexBin.Xie@amd.com>

Thanks,
Alex Bin Xie

________________________________
From: Xie, AlexBin

Sent: Monday, April 17, 2017 2:58 PM
To: Liu, Monk; 'amd-gfx@lists.freedesktop.org'
Cc: brahma_sw_dev
Subject: Re: [PATCH] amdgpu:fix incorrect use on the remap_mutex


Reviewed by:  Alex Xie <AlexBin.Xie@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@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@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@amd.com>

---
 amdgpu/amdgpu_bo.c    | 29 ++++++++++++++++++-----------
 amdgpu/amdgpu_vamgr.c |  5 +++--
 2 files changed, 21 insertions(+), 13 deletions(-)

         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

Patch hide | download patch | download mbox

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;