RE: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.

[He, Roger]

I missed the Per-VM-BO share the reservation object with root bo. So context is 
not NULL here.
So,  this patch is:

Reviewed-by: Roger He <hongbo...@amd.com>

Thanks
Roger(Hongbo.He)
-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com]
Sent: Friday, February 23, 2018 8:06 PM
To: He, Roger <hongbo...@amd.com>; amd-gfx@lists.freedesktop.org; 
dri-de...@lists.freedesktop.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and 
swapout.

Am 23.02.2018 um 10:46 schrieb He, Roger:
>
> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On 
> Behalf Of Christian K?nig
> Sent: Tuesday, February 20, 2018 8:58 PM
> To: amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; 
> linux-ker...@vger.kernel.org
> Subject: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and 
> swapout.
>
> This solves the problem that when we swapout a BO from a domain we sometimes 
> couldn't make room for it because holding the lock blocks all other BOs with 
> this reservation object.
>
> Signed-off-by: Christian König <christian.koe...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 33 -
>   1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index d90b1cf10b27..3a44c2ee4155 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct 
> ttm_buffer_object *bo,  EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>   
>   /**
> - * Check the target bo is allowable to be evicted or swapout, including 
> cases:
> - *
> - * a. if share same reservation object with ctx->resv, have 
> assumption
> - * reservation objects should already be locked, so not lock again 
> and
> - * return true directly when either the opreation 
> allow_reserved_eviction
> - * or the target bo already is in delayed free list;
> - *
> - * b. Otherwise, trylock it.
> + * Check if the target bo is allowed to be evicted or swapedout.
>*/
>   static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
> - struct ttm_operation_ctx *ctx, bool *locked)
> +struct ttm_operation_ctx *ctx,
> +bool *locked)
>   {
> - bool ret = false;
> + /* First check if we can lock it */
> + *locked = reservation_object_trylock(bo->resv);
> + if (*locked)
> + return true;
>   
> - *locked = false;
> + /* Check if it's locked because it is part of the current operation 
> +*/
>   if (bo->resv == ctx->resv) {
>   reservation_object_assert_held(bo->resv);
> - if (ctx->allow_reserved_eviction || !list_empty(>ddestroy))
> - ret = true;
> - } else {
> - *locked = reservation_object_trylock(bo->resv);
> - ret = *locked;
> + return ctx->allow_reserved_eviction ||
> + !list_empty(>ddestroy);
>   }
>   
> - return ret;
> + /* Check if it's locked because it was already evicted */
> + if (ww_mutex_is_owned_by(>resv->lock, NULL))
> + return true;
>
> For the special case: when command submission with Per-VM-BO enabled, 
> All BOs  a/b/c are always valid BO. After the validation of BOs a and 
> b, when validation of BO c, is it possible to return true and then evict BO a 
> and b by mistake ?
> Because a/b/c share same task_struct.

No, that's why I check the context as well. BOs explicitly reserved 
have a non NULL context while BOs trylocked for swapout havea NULL 
context.

BOs have a non NULL context only when command submission and reserved 
by ttm_eu_re6serve_buffers  .
But for Per-VM-BO a/b/c they always are not in BO list, so they will be 
not reserved and have always NULL context.
So above case also can happen. Anything missing here?  

>
> + /* Some other thread is using it, don't touch it */
> + return false;
>   }
>   
>   static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> --
> 2.14.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.

[He, Roger]

-Original Message-
From: Christian König [mailto:ckoenig.leichtzumer...@gmail.com] 
Sent: Friday, February 23, 2018 8:06 PM
To: He, Roger <hongbo...@amd.com>; amd-gfx@lists.freedesktop.org; 
dri-de...@lists.freedesktop.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and 
swapout.

Am 23.02.2018 um 10:46 schrieb He, Roger:
>
> -Original Message-
> From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On 
> Behalf Of Christian K?nig
> Sent: Tuesday, February 20, 2018 8:58 PM
> To: amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; 
> linux-ker...@vger.kernel.org
> Subject: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and 
> swapout.
>
> This solves the problem that when we swapout a BO from a domain we sometimes 
> couldn't make room for it because holding the lock blocks all other BOs with 
> this reservation object.
>
> Signed-off-by: Christian König <christian.koe...@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 33 -
>   1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c index d90b1cf10b27..3a44c2ee4155 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct 
> ttm_buffer_object *bo,  EXPORT_SYMBOL(ttm_bo_eviction_valuable);
>   
>   /**
> - * Check the target bo is allowable to be evicted or swapout, including 
> cases:
> - *
> - * a. if share same reservation object with ctx->resv, have 
> assumption
> - * reservation objects should already be locked, so not lock again 
> and
> - * return true directly when either the opreation 
> allow_reserved_eviction
> - * or the target bo already is in delayed free list;
> - *
> - * b. Otherwise, trylock it.
> + * Check if the target bo is allowed to be evicted or swapedout.
>*/
>   static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
> - struct ttm_operation_ctx *ctx, bool *locked)
> +struct ttm_operation_ctx *ctx,
> +bool *locked)
>   {
> - bool ret = false;
> + /* First check if we can lock it */
> + *locked = reservation_object_trylock(bo->resv);
> + if (*locked)
> + return true;
>   
> - *locked = false;
> + /* Check if it's locked because it is part of the current operation 
> +*/
>   if (bo->resv == ctx->resv) {
>   reservation_object_assert_held(bo->resv);
> - if (ctx->allow_reserved_eviction || !list_empty(>ddestroy))
> - ret = true;
> - } else {
> - *locked = reservation_object_trylock(bo->resv);
> - ret = *locked;
> + return ctx->allow_reserved_eviction ||
> + !list_empty(>ddestroy);
>   }
>   
> - return ret;
> + /* Check if it's locked because it was already evicted */
> + if (ww_mutex_is_owned_by(>resv->lock, NULL))
> + return true;
>
> For the special case: when command submission with Per-VM-BO enabled, 
> All BOs  a/b/c are always valid BO. After the validation of BOs a and 
> b, when validation of BO c, is it possible to return true and then evict BO a 
> and b by mistake ?
> Because a/b/c share same task_struct.

No, that's why I check the context as well. BOs explicitly reserved 
have a non NULL context while BOs trylocked for swapout have a NULL context.

When BOs have a non NULL context only when command submission and reserved by 
ttm_eu_re6serve_buffers  .
But for Per-VM-BO a/b/c they always are not in BO list, so they will be not 
reserved and have always NULL context.
So above case also can happen. Anything missing here?  

Thanks
Roger(Hongbo.He)
>
> + /* Some other thread is using it, don't touch it */
> + return false;
>   }
>   
>   static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
> --
> 2.14.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.

[Christian König]

Am 23.02.2018 um 10:46 schrieb He, Roger:


-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Tuesday, February 20, 2018 8:58 PM
To: amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; 
linux-ker...@vger.kernel.org
Subject: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and 
swapout.

This solves the problem that when we swapout a BO from a domain we sometimes 
couldn't make room for it because holding the lock blocks all other BOs with 
this reservation object.

Signed-off-by: Christian König <christian.koe...@amd.com>
---
  drivers/gpu/drm/ttm/ttm_bo.c | 33 -
  1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
d90b1cf10b27..3a44c2ee4155 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object 
*bo,  EXPORT_SYMBOL(ttm_bo_eviction_valuable);
  
  /**

- * Check the target bo is allowable to be evicted or swapout, including cases:
- *
- * a. if share same reservation object with ctx->resv, have assumption
- * reservation objects should already be locked, so not lock again and
- * return true directly when either the opreation allow_reserved_eviction
- * or the target bo already is in delayed free list;
- *
- * b. Otherwise, trylock it.
+ * Check if the target bo is allowed to be evicted or swapedout.
   */
  static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
-   struct ttm_operation_ctx *ctx, bool *locked)
+  struct ttm_operation_ctx *ctx,
+  bool *locked)
  {
-   bool ret = false;
+   /* First check if we can lock it */
+   *locked = reservation_object_trylock(bo->resv);
+   if (*locked)
+   return true;
  
-	*locked = false;

+   /* Check if it's locked because it is part of the current operation */
if (bo->resv == ctx->resv) {
reservation_object_assert_held(bo->resv);
-   if (ctx->allow_reserved_eviction || !list_empty(>ddestroy))
-   ret = true;
-   } else {
-   *locked = reservation_object_trylock(bo->resv);
-   ret = *locked;
+   return ctx->allow_reserved_eviction ||
+   !list_empty(>ddestroy);
}
  
-	return ret;

+   /* Check if it's locked because it was already evicted */
+   if (ww_mutex_is_owned_by(>resv->lock, NULL))
+   return true;

For the special case: when command submission with Per-VM-BO enabled,
All BOs  a/b/c are always valid BO. After the validation of BOs a and b,
when validation of BO c, is it possible to return true and then evict BO a and 
b by mistake ?
Because a/b/c share same task_struct.


No, that's why I check the context as well. BOs explicitly reserved have 
a non NULL context while BOs trylocked for swapout have a NULL context.


Christian.



Thanks
Roger(Hongbo.He)

+   /* Some other thread is using it, don't touch it */
+   return false;
  }
  
  static int ttm_mem_evict_first(struct ttm_bo_device *bdev,

--
2.14.1

___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

RE: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.

[He, Roger]

-Original Message-
From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
Christian K?nig
Sent: Tuesday, February 20, 2018 8:58 PM
To: amd-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org; 
linux-ker...@vger.kernel.org
Subject: [PATCH 3/4] drm/ttm: handle already locked BOs during eviction and 
swapout.

This solves the problem that when we swapout a BO from a domain we sometimes 
couldn't make room for it because holding the lock blocks all other BOs with 
this reservation object.

Signed-off-by: Christian König <christian.koe...@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 33 -
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 
d90b1cf10b27..3a44c2ee4155 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object 
*bo,  EXPORT_SYMBOL(ttm_bo_eviction_valuable);
 
 /**
- * Check the target bo is allowable to be evicted or swapout, including cases:
- *
- * a. if share same reservation object with ctx->resv, have assumption
- * reservation objects should already be locked, so not lock again and
- * return true directly when either the opreation allow_reserved_eviction
- * or the target bo already is in delayed free list;
- *
- * b. Otherwise, trylock it.
+ * Check if the target bo is allowed to be evicted or swapedout.
  */
 static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
-   struct ttm_operation_ctx *ctx, bool *locked)
+  struct ttm_operation_ctx *ctx,
+  bool *locked)
 {
-   bool ret = false;
+   /* First check if we can lock it */
+   *locked = reservation_object_trylock(bo->resv);
+   if (*locked)
+   return true;
 
-   *locked = false;
+   /* Check if it's locked because it is part of the current operation */
if (bo->resv == ctx->resv) {
reservation_object_assert_held(bo->resv);
-   if (ctx->allow_reserved_eviction || !list_empty(>ddestroy))
-   ret = true;
-   } else {
-   *locked = reservation_object_trylock(bo->resv);
-   ret = *locked;
+   return ctx->allow_reserved_eviction ||
+   !list_empty(>ddestroy);
}
 
-   return ret;
+   /* Check if it's locked because it was already evicted */
+   if (ww_mutex_is_owned_by(>resv->lock, NULL))
+   return true;

For the special case: when command submission with Per-VM-BO enabled,
All BOs  a/b/c are always valid BO. After the validation of BOs a and b,  
when validation of BO c, is it possible to return true and then evict BO a and 
b by mistake ?
Because a/b/c share same task_struct.

Thanks
Roger(Hongbo.He)

+   /* Some other thread is using it, don't touch it */
+   return false;
 }
 
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
--
2.14.1

___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.

[Christian König]

This solves the problem that when we swapout a BO from a domain we
sometimes couldn't make room for it because holding the lock blocks all
other BOs with this reservation object.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/ttm/ttm_bo.c | 33 -
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index d90b1cf10b27..3a44c2ee4155 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object 
*bo,
 EXPORT_SYMBOL(ttm_bo_eviction_valuable);
 
 /**
- * Check the target bo is allowable to be evicted or swapout, including cases:
- *
- * a. if share same reservation object with ctx->resv, have assumption
- * reservation objects should already be locked, so not lock again and
- * return true directly when either the opreation allow_reserved_eviction
- * or the target bo already is in delayed free list;
- *
- * b. Otherwise, trylock it.
+ * Check if the target bo is allowed to be evicted or swapedout.
  */
 static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
-   struct ttm_operation_ctx *ctx, bool *locked)
+  struct ttm_operation_ctx *ctx,
+  bool *locked)
 {
-   bool ret = false;
+   /* First check if we can lock it */
+   *locked = reservation_object_trylock(bo->resv);
+   if (*locked)
+   return true;
 
-   *locked = false;
+   /* Check if it's locked because it is part of the current operation */
if (bo->resv == ctx->resv) {
reservation_object_assert_held(bo->resv);
-   if (ctx->allow_reserved_eviction || !list_empty(>ddestroy))
-   ret = true;
-   } else {
-   *locked = reservation_object_trylock(bo->resv);
-   ret = *locked;
+   return ctx->allow_reserved_eviction ||
+   !list_empty(>ddestroy);
}
 
-   return ret;
+   /* Check if it's locked because it was already evicted */
+   if (ww_mutex_is_owned_by(>resv->lock, NULL))
+   return true;
+
+   /* Some other thread is using it, don't touch it */
+   return false;
 }
 
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
-- 
2.14.1

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

[PATCH 3/4] drm/ttm: handle already locked BOs during eviction and swapout.

[Christian König]

This solves the problem that when we swapout a BO from a domain we
sometimes couldn't make room for it because holding the lock blocks all
other BOs with this reservation object.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/ttm/ttm_bo.c | 33 -
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index d90b1cf10b27..fba40e22d088 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object 
*bo,
 EXPORT_SYMBOL(ttm_bo_eviction_valuable);
 
 /**
- * Check the target bo is allowable to be evicted or swapout, including cases:
- *
- * a. if share same reservation object with ctx->resv, have assumption
- * reservation objects should already be locked, so not lock again and
- * return true directly when either the opreation allow_reserved_eviction
- * or the target bo already is in delayed free list;
- *
- * b. Otherwise, trylock it.
+ * Check if the target bo is allowed to be evicted or swapedout.
  */
 static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
-   struct ttm_operation_ctx *ctx, bool *locked)
+  struct ttm_operation_ctx *ctx,
+  bool *locked)
 {
-   bool ret = false;
+   /* First check if we can lock it */
+   *locked = reservation_object_trylock(bo->resv);
+   if (*locked)
+   return true;
 
-   *locked = false;
+   /* Check if it's locked because it is part of the current operation */
if (bo->resv == ctx->resv) {
reservation_object_assert_held(bo->resv);
-   if (ctx->allow_reserved_eviction || !list_empty(>ddestroy))
-   ret = true;
-   } else {
-   *locked = reservation_object_trylock(bo->resv);
-   ret = *locked;
+   return ctx->allow_reserved_eviction ||
+   !list_empty(>ddestroy);
}
 
-   return ret;
+   /* Check if it's locked because it was already evicted */
+   if (ww_mutex_is_owned_by(>resv->lock, current, NULL))
+   return true;
+
+   /* Some other thread is using it, don't touch it */
+   return false;
 }
 
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
-- 
2.14.1

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