[PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Chunming Zhou]

there is a scheduling balance issue about get node like:
a. process A allocates full memory and use it for submission.
b. process B tries to allocates memory, will wait for process A BO idle in eviction.
c. process A completes the job, process B eviction will put process A BO node,
but in the meantime, process C is comming to allocate BO, whill directly get node successfully, and do submission,
process B will again wait for process C BO idle.
d. repeat the above setps, process B could be delayed much more.

later allocation must not be ahead of front in same place.

Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c    | 69 +++++++++++++++++++++++++++++++++++++++--
 include/drm/ttm/ttm_bo_api.h    |  7 +++++
 include/drm/ttm/ttm_bo_driver.h |  7 +++++
 3 files changed, 80 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index d33a6bb742a1..558ec2cf465d 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
 	return 0;
 }
 
+static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
+				struct ttm_buffer_object *bo,
+				const struct ttm_place *place)
+{
+	waiter->tbo = bo;
+	memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
+	INIT_LIST_HEAD(&waiter->list);
+}
+
+static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
+			       struct ttm_bo_waiter *waiter)
+{
+	if (!waiter)
+		return;
+	spin_lock(&man->wait_lock);
+	list_add_tail(&waiter->list, &man->waiter_list);
+	spin_unlock(&man->wait_lock);
+}
+
+static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
+			       struct ttm_bo_waiter *waiter)
+{
+	if (!waiter)
+		return;
+	spin_lock(&man->wait_lock);
+	if (!list_empty(&waiter->list))
+		list_del(&waiter->list);
+	spin_unlock(&man->wait_lock);
+	kfree(waiter);
+}
+
+int ttm_man_check_bo(struct ttm_mem_type_manager *man,
+		     struct ttm_buffer_object *bo,
+		     const struct ttm_place *place)
+{
+	struct ttm_bo_waiter *waiter, *tmp;
+
+	spin_lock(&man->wait_lock);
+	list_for_each_entry_safe(waiter, tmp, &man->waiter_list, list) {
+		if ((bo != waiter->tbo) &&
+		    ((place->fpfn >= waiter->place.fpfn &&
+		      place->fpfn <= waiter->place.lpfn) ||
+		     (place->lpfn <= waiter->place.lpfn && place->lpfn >=
+		      waiter->place.fpfn)))
+		    goto later_bo;
+	}
+	spin_unlock(&man->wait_lock);
+	return true;
+later_bo:
+	spin_unlock(&man->wait_lock);
+	return false;
+}
 /**
  * Repeatedly evict memory from the LRU for @mem_type until we create enough
  * space, or we've evicted everything and there isn't enough space.
@@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
 {
 	struct ttm_bo_device *bdev = bo->bdev;
 	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
+	struct ttm_bo_waiter waiter;
 	int ret;
 
+	ttm_man_init_waiter(&waiter, bo, place);
+	ttm_man_add_waiter(man, &waiter);
 	do {
 		ret = (*man->func->get_node)(man, bo, place, mem);
-		if (unlikely(ret != 0))
+		if (unlikely(ret != 0)) {
+			ttm_man_del_waiter(man, &waiter);
 			return ret;
-		if (mem->mm_node)
+		}
+		if (mem->mm_node) {
+			ttm_man_del_waiter(man, &waiter);
 			break;
+		}
 		ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
-		if (unlikely(ret != 0))
+		if (unlikely(ret != 0)) {
+			ttm_man_del_waiter(man, &waiter);
 			return ret;
+		}
 	} while (1);
 	mem->mem_type = mem_type;
 	return ttm_bo_add_move_fence(bo, man, mem);
@@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
 	man->use_io_reserve_lru = false;
 	mutex_init(&man->io_reserve_mutex);
 	spin_lock_init(&man->move_lock);
+	spin_lock_init(&man->wait_lock);
+	INIT_LIST_HEAD(&man->waiter_list);
 	INIT_LIST_HEAD(&man->io_reserve_lru);
 
 	ret = bdev->driver->init_mem_type(bdev, type, man);
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 2cd025c2abe7..0fce4dbd02e7 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -40,6 +40,7 @@
 #include <linux/mm.h>
 #include <linux/bitmap.h>
 #include <linux/reservation.h>
+#include <drm/ttm/ttm_placement.h>
 
 struct ttm_bo_device;
 
@@ -232,6 +233,12 @@ struct ttm_buffer_object {
 	struct mutex wu_mutex;
 };
 
+struct ttm_bo_waiter {
+	struct ttm_buffer_object *tbo;
+	struct ttm_place place;
+	struct list_head list;
+};
+
 /**
  * struct ttm_bo_kmap_obj
  *
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 9b417eb2df20..dc6b8b4c9e06 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
 	bool io_reserve_fastpath;
 	spinlock_t move_lock;
 
+	/* waiters in list */
+	spinlock_t wait_lock;
+	struct list_head waiter_list;
+
 	/*
 	 * Protected by @io_reserve_mutex:
 	 */
@@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
 		     struct ttm_mem_reg *mem,
 		     struct ttm_operation_ctx *ctx);
 
+int ttm_man_check_bo(struct ttm_mem_type_manager *man,
+		     struct ttm_buffer_object *bo,
+		     const struct ttm_place *place);
 void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem);
 void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
 			   struct ttm_mem_reg *mem);
-- 
2.14.1

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

[PATCH 2/2] [WIP]drm/amdgpu: fix scheduling balance

[Chunming Zhou]

Change-Id: I6535023db57d4ff5e79f7928595bf8ff1f60f23f
Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c  | 4 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
index 7abc1f3251ea..5bed3d8e364a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
@@ -173,6 +173,10 @@ static int amdgpu_gtt_mgr_new(struct ttm_mem_type_manager *man,
 		spin_unlock(&mgr->lock);
 		return 0;
 	}
+	if (!ttm_man_check_bo(man, tbo, place)) {
+		spin_unlock(&mgr->lock);
+		return 0;
+	}
 	atomic64_sub(mem->num_pages, &mgr->available);
 	spin_unlock(&mgr->lock);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 9aca653bec07..7202599bb67f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -121,6 +121,9 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager *man,
 	unsigned i;
 	int r;
 
+	if (!ttm_man_check_bo(man, tbo, place))
+		return 0;
+
 	lpfn = place->lpfn;
 	if (!lpfn)
 		lpfn = man->size;
-- 
2.14.1

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Christian König]

Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
> there is a scheduling balance issue about get node like:
> a. process A allocates full memory and use it for submission.
> b. process B tries to allocates memory, will wait for process A BO idle in eviction.
> c. process A completes the job, process B eviction will put process A BO node,
> but in the meantime, process C is comming to allocate BO, whill directly get node successfully, and do submission,
> process B will again wait for process C BO idle.
> d. repeat the above setps, process B could be delayed much more.
>
> later allocation must not be ahead of front in same place.

Again NAK to the whole approach.

At least with amdgpu the problem you described above never occurs 
because evictions are pipelined operations. We could only block for 
deleted regions to become free.

But independent of that incoming memory requests while we make room for 
eviction are intended to be served first.

Changing that is certainly a no-go cause that would favor memory hungry 
applications over small clients.

Regards,
Christian.

>
> Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c    | 69 +++++++++++++++++++++++++++++++++++++++--
>   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>   3 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index d33a6bb742a1..558ec2cf465d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
>   	return 0;
>   }
>   
> +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
> +				struct ttm_buffer_object *bo,
> +				const struct ttm_place *place)
> +{
> +	waiter->tbo = bo;
> +	memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
> +	INIT_LIST_HEAD(&waiter->list);
> +}
> +
> +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
> +			       struct ttm_bo_waiter *waiter)
> +{
> +	if (!waiter)
> +		return;
> +	spin_lock(&man->wait_lock);
> +	list_add_tail(&waiter->list, &man->waiter_list);
> +	spin_unlock(&man->wait_lock);
> +}
> +
> +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
> +			       struct ttm_bo_waiter *waiter)
> +{
> +	if (!waiter)
> +		return;
> +	spin_lock(&man->wait_lock);
> +	if (!list_empty(&waiter->list))
> +		list_del(&waiter->list);
> +	spin_unlock(&man->wait_lock);
> +	kfree(waiter);
> +}
> +
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +		     struct ttm_buffer_object *bo,
> +		     const struct ttm_place *place)
> +{
> +	struct ttm_bo_waiter *waiter, *tmp;
> +
> +	spin_lock(&man->wait_lock);
> +	list_for_each_entry_safe(waiter, tmp, &man->waiter_list, list) {
> +		if ((bo != waiter->tbo) &&
> +		    ((place->fpfn >= waiter->place.fpfn &&
> +		      place->fpfn <= waiter->place.lpfn) ||
> +		     (place->lpfn <= waiter->place.lpfn && place->lpfn >=
> +		      waiter->place.fpfn)))
> +		    goto later_bo;
> +	}
> +	spin_unlock(&man->wait_lock);
> +	return true;
> +later_bo:
> +	spin_unlock(&man->wait_lock);
> +	return false;
> +}
>   /**
>    * Repeatedly evict memory from the LRU for @mem_type until we create enough
>    * space, or we've evicted everything and there isn't enough space.
> @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>   {
>   	struct ttm_bo_device *bdev = bo->bdev;
>   	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> +	struct ttm_bo_waiter waiter;
>   	int ret;
>   
> +	ttm_man_init_waiter(&waiter, bo, place);
> +	ttm_man_add_waiter(man, &waiter);
>   	do {
>   		ret = (*man->func->get_node)(man, bo, place, mem);
> -		if (unlikely(ret != 0))
> +		if (unlikely(ret != 0)) {
> +			ttm_man_del_waiter(man, &waiter);
>   			return ret;
> -		if (mem->mm_node)
> +		}
> +		if (mem->mm_node) {
> +			ttm_man_del_waiter(man, &waiter);
>   			break;
> +		}
>   		ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
> -		if (unlikely(ret != 0))
> +		if (unlikely(ret != 0)) {
> +			ttm_man_del_waiter(man, &waiter);
>   			return ret;
> +		}
>   	} while (1);
>   	mem->mem_type = mem_type;
>   	return ttm_bo_add_move_fence(bo, man, mem);
> @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
>   	man->use_io_reserve_lru = false;
>   	mutex_init(&man->io_reserve_mutex);
>   	spin_lock_init(&man->move_lock);
> +	spin_lock_init(&man->wait_lock);
> +	INIT_LIST_HEAD(&man->waiter_list);
>   	INIT_LIST_HEAD(&man->io_reserve_lru);
>   
>   	ret = bdev->driver->init_mem_type(bdev, type, man);
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 2cd025c2abe7..0fce4dbd02e7 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -40,6 +40,7 @@
>   #include <linux/mm.h>
>   #include <linux/bitmap.h>
>   #include <linux/reservation.h>
> +#include <drm/ttm/ttm_placement.h>
>   
>   struct ttm_bo_device;
>   
> @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>   	struct mutex wu_mutex;
>   };
>   
> +struct ttm_bo_waiter {
> +	struct ttm_buffer_object *tbo;
> +	struct ttm_place place;
> +	struct list_head list;
> +};
> +
>   /**
>    * struct ttm_bo_kmap_obj
>    *
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 9b417eb2df20..dc6b8b4c9e06 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>   	bool io_reserve_fastpath;
>   	spinlock_t move_lock;
>   
> +	/* waiters in list */
> +	spinlock_t wait_lock;
> +	struct list_head waiter_list;
> +
>   	/*
>   	 * Protected by @io_reserve_mutex:
>   	 */
> @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>   		     struct ttm_mem_reg *mem,
>   		     struct ttm_operation_ctx *ctx);
>   
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +		     struct ttm_buffer_object *bo,
> +		     const struct ttm_place *place);
>   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem);
>   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>   			   struct ttm_mem_reg *mem);

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Zhou, David(ChunMing)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1: Type: text/plain; charset="gb2312", Size: 7917 bytes --]

I am off work, so reply mail by phone, the format could not be text.

back to topic itself:
the problem indeed happen on amdgpu driver, someone reports me that application runs with two instances, the performance are different.
I also reproduced the issue with unit test(bo_eviction_test). They always think our scheduler isn't working as expected.

After my investigation, this issue should be detect of TTM design self, which breaks scheduling balance.

Further, if we run containers for our gpu, container A could run high score, container B runs low score with same benchmark.

So this is bug that we need fix.

Regards,
David Zhou


·¢×Ô¼á¹û Pro

Christian König <ckoenig.leichtzumerken@gmail.com> ÓÚ 2018Äê1ÔÂ26ÈÕ ÏÂÎç6:31дµÀ£º

Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
> there is a scheduling balance issue about get node like:
> a. process A allocates full memory and use it for submission.
> b. process B tries to allocates memory, will wait for process A BO idle in eviction.
> c. process A completes the job, process B eviction will put process A BO node,
> but in the meantime, process C is comming to allocate BO, whill directly get node successfully, and do submission,
> process B will again wait for process C BO idle.
> d. repeat the above setps, process B could be delayed much more.
>
> later allocation must not be ahead of front in same place.

Again NAK to the whole approach.

At least with amdgpu the problem you described above never occurs
because evictions are pipelined operations. We could only block for
deleted regions to become free.

But independent of that incoming memory requests while we make room for
eviction are intended to be served first.

Changing that is certainly a no-go cause that would favor memory hungry
applications over small clients.

Regards,
Christian.

>
> Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c    | 69 +++++++++++++++++++++++++++++++++++++++--
>   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>   3 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index d33a6bb742a1..558ec2cf465d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
>        return 0;
>   }
>
> +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
> +                             struct ttm_buffer_object *bo,
> +                             const struct ttm_place *place)
> +{
> +     waiter->tbo = bo;
> +     memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
> +     INIT_LIST_HEAD(&waiter->list);
> +}
> +
> +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     list_add_tail(&waiter->list, &man->waiter_list);
> +     spin_unlock(&man->wait_lock);
> +}
> +
> +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     if (!list_empty(&waiter->list))
> +             list_del(&waiter->list);
> +     spin_unlock(&man->wait_lock);
> +     kfree(waiter);
> +}
> +
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place)
> +{
> +     struct ttm_bo_waiter *waiter, *tmp;
> +
> +     spin_lock(&man->wait_lock);
> +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, list) {
> +             if ((bo != waiter->tbo) &&
> +                 ((place->fpfn >= waiter->place.fpfn &&
> +                   place->fpfn <= waiter->place.lpfn) ||
> +                  (place->lpfn <= waiter->place.lpfn && place->lpfn >=
> +                   waiter->place.fpfn)))
> +                 goto later_bo;
> +     }
> +     spin_unlock(&man->wait_lock);
> +     return true;
> +later_bo:
> +     spin_unlock(&man->wait_lock);
> +     return false;
> +}
>   /**
>    * Repeatedly evict memory from the LRU for @mem_type until we create enough
>    * space, or we've evicted everything and there isn't enough space.
> @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>   {
>        struct ttm_bo_device *bdev = bo->bdev;
>        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> +     struct ttm_bo_waiter waiter;
>        int ret;
>
> +     ttm_man_init_waiter(&waiter, bo, place);
> +     ttm_man_add_waiter(man, &waiter);
>        do {
>                ret = (*man->func->get_node)(man, bo, place, mem);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> -             if (mem->mm_node)
> +             }
> +             if (mem->mm_node) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        break;
> +             }
>                ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> +             }
>        } while (1);
>        mem->mem_type = mem_type;
>        return ttm_bo_add_move_fence(bo, man, mem);
> @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
>        man->use_io_reserve_lru = false;
>        mutex_init(&man->io_reserve_mutex);
>        spin_lock_init(&man->move_lock);
> +     spin_lock_init(&man->wait_lock);
> +     INIT_LIST_HEAD(&man->waiter_list);
>        INIT_LIST_HEAD(&man->io_reserve_lru);
>
>        ret = bdev->driver->init_mem_type(bdev, type, man);
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 2cd025c2abe7..0fce4dbd02e7 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -40,6 +40,7 @@
>   #include <linux/mm.h>
>   #include <linux/bitmap.h>
>   #include <linux/reservation.h>
> +#include <drm/ttm/ttm_placement.h>
>
>   struct ttm_bo_device;
>
> @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>        struct mutex wu_mutex;
>   };
>
> +struct ttm_bo_waiter {
> +     struct ttm_buffer_object *tbo;
> +     struct ttm_place place;
> +     struct list_head list;
> +};
> +
>   /**
>    * struct ttm_bo_kmap_obj
>    *
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 9b417eb2df20..dc6b8b4c9e06 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>        bool io_reserve_fastpath;
>        spinlock_t move_lock;
>
> +     /* waiters in list */
> +     spinlock_t wait_lock;
> +     struct list_head waiter_list;
> +
>        /*
>         * Protected by @io_reserve_mutex:
>         */
> @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>                     struct ttm_mem_reg *mem,
>                     struct ttm_operation_ctx *ctx);
>
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place);
>   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem);
>   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>                           struct ttm_mem_reg *mem);


[-- Attachment #1.2: Type: text/html, Size: 16277 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Zhou, David(ChunMing)]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1: Type: text/plain; charset="gb2312", Size: 7917 bytes --]

I am off work, so reply mail by phone, the format could not be text.

back to topic itself:
the problem indeed happen on amdgpu driver, someone reports me that application runs with two instances, the performance are different.
I also reproduced the issue with unit test(bo_eviction_test). They always think our scheduler isn't working as expected.

After my investigation, this issue should be detect of TTM design self, which breaks scheduling balance.

Further, if we run containers for our gpu, container A could run high score, container B runs low score with same benchmark.

So this is bug that we need fix.

Regards,
David Zhou


·¢×Ô¼á¹û Pro

Christian König <ckoenig.leichtzumerken@gmail.com> ÓÚ 2018Äê1ÔÂ26ÈÕ ÏÂÎç6:31дµÀ£º

Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
> there is a scheduling balance issue about get node like:
> a. process A allocates full memory and use it for submission.
> b. process B tries to allocates memory, will wait for process A BO idle in eviction.
> c. process A completes the job, process B eviction will put process A BO node,
> but in the meantime, process C is comming to allocate BO, whill directly get node successfully, and do submission,
> process B will again wait for process C BO idle.
> d. repeat the above setps, process B could be delayed much more.
>
> later allocation must not be ahead of front in same place.

Again NAK to the whole approach.

At least with amdgpu the problem you described above never occurs
because evictions are pipelined operations. We could only block for
deleted regions to become free.

But independent of that incoming memory requests while we make room for
eviction are intended to be served first.

Changing that is certainly a no-go cause that would favor memory hungry
applications over small clients.

Regards,
Christian.

>
> Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c    | 69 +++++++++++++++++++++++++++++++++++++++--
>   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>   3 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index d33a6bb742a1..558ec2cf465d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
>        return 0;
>   }
>
> +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
> +                             struct ttm_buffer_object *bo,
> +                             const struct ttm_place *place)
> +{
> +     waiter->tbo = bo;
> +     memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
> +     INIT_LIST_HEAD(&waiter->list);
> +}
> +
> +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     list_add_tail(&waiter->list, &man->waiter_list);
> +     spin_unlock(&man->wait_lock);
> +}
> +
> +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     if (!list_empty(&waiter->list))
> +             list_del(&waiter->list);
> +     spin_unlock(&man->wait_lock);
> +     kfree(waiter);
> +}
> +
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place)
> +{
> +     struct ttm_bo_waiter *waiter, *tmp;
> +
> +     spin_lock(&man->wait_lock);
> +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, list) {
> +             if ((bo != waiter->tbo) &&
> +                 ((place->fpfn >= waiter->place.fpfn &&
> +                   place->fpfn <= waiter->place.lpfn) ||
> +                  (place->lpfn <= waiter->place.lpfn && place->lpfn >=
> +                   waiter->place.fpfn)))
> +                 goto later_bo;
> +     }
> +     spin_unlock(&man->wait_lock);
> +     return true;
> +later_bo:
> +     spin_unlock(&man->wait_lock);
> +     return false;
> +}
>   /**
>    * Repeatedly evict memory from the LRU for @mem_type until we create enough
>    * space, or we've evicted everything and there isn't enough space.
> @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>   {
>        struct ttm_bo_device *bdev = bo->bdev;
>        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> +     struct ttm_bo_waiter waiter;
>        int ret;
>
> +     ttm_man_init_waiter(&waiter, bo, place);
> +     ttm_man_add_waiter(man, &waiter);
>        do {
>                ret = (*man->func->get_node)(man, bo, place, mem);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> -             if (mem->mm_node)
> +             }
> +             if (mem->mm_node) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        break;
> +             }
>                ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> +             }
>        } while (1);
>        mem->mem_type = mem_type;
>        return ttm_bo_add_move_fence(bo, man, mem);
> @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
>        man->use_io_reserve_lru = false;
>        mutex_init(&man->io_reserve_mutex);
>        spin_lock_init(&man->move_lock);
> +     spin_lock_init(&man->wait_lock);
> +     INIT_LIST_HEAD(&man->waiter_list);
>        INIT_LIST_HEAD(&man->io_reserve_lru);
>
>        ret = bdev->driver->init_mem_type(bdev, type, man);
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 2cd025c2abe7..0fce4dbd02e7 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -40,6 +40,7 @@
>   #include <linux/mm.h>
>   #include <linux/bitmap.h>
>   #include <linux/reservation.h>
> +#include <drm/ttm/ttm_placement.h>
>
>   struct ttm_bo_device;
>
> @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>        struct mutex wu_mutex;
>   };
>
> +struct ttm_bo_waiter {
> +     struct ttm_buffer_object *tbo;
> +     struct ttm_place place;
> +     struct list_head list;
> +};
> +
>   /**
>    * struct ttm_bo_kmap_obj
>    *
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 9b417eb2df20..dc6b8b4c9e06 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>        bool io_reserve_fastpath;
>        spinlock_t move_lock;
>
> +     /* waiters in list */
> +     spinlock_t wait_lock;
> +     struct list_head waiter_list;
> +
>        /*
>         * Protected by @io_reserve_mutex:
>         */
> @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>                     struct ttm_mem_reg *mem,
>                     struct ttm_operation_ctx *ctx);
>
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place);
>   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem);
>   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>                           struct ttm_mem_reg *mem);


[-- Attachment #1.2: Type: text/html, Size: 16298 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 9616 bytes --]

> After my investigation, this issue should be detect of TTM design 
> self, which breaks scheduling balance.
Yeah, but again. This is indented design we can't change easily.

Regards,
Christian.

Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
> I am off work, so reply mail by phone, the format could not be text.
>
> back to topic itself:
> the problem indeed happen on amdgpu driver, someone reports me that 
> application runs with two instances, the performance are different.
> I also reproduced the issue with unit test(bo_eviction_test). They 
> always think our scheduler isn't working as expected.
>
> After my investigation, this issue should be detect of TTM design 
> self, which breaks scheduling balance.
>
> Further, if we run containers for our gpu, container A could run high 
> score, container B runs low score with same benchmark.
>
> So this is bug that we need fix.
>
> Regards,
> David Zhou
>
> 发自坚果 Pro
>
> Christian K鰊ig <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 于 2018年1月26日 
> 下午6:31写道：
>
> Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
> > there is a scheduling balance issue about get node like:
> > a. process A allocates full memory and use it for submission.
> > b. process B tries to allocates memory, will wait for process A BO 
> idle in eviction.
> > c. process A completes the job, process B eviction will put process 
> A BO node,
> > but in the meantime, process C is comming to allocate BO, whill 
> directly get node successfully, and do submission,
> > process B will again wait for process C BO idle.
> > d. repeat the above setps, process B could be delayed much more.
> >
> > later allocation must not be ahead of front in same place.
>
> Again NAK to the whole approach.
>
> At least with amdgpu the problem you described above never occurs
> because evictions are pipelined operations. We could only block for
> deleted regions to become free.
>
> But independent of that incoming memory requests while we make room for
> eviction are intended to be served first.
>
> Changing that is certainly a no-go cause that would favor memory hungry
> applications over small clients.
>
> Regards,
> Christian.
>
> >
> > Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
> > Signed-off-by: Chunming Zhou <david1.zhou-5C7GfCeVMHo@public.gmane.org>
> > ---
> >   drivers/gpu/drm/ttm/ttm_bo.c    | 69 
> +++++++++++++++++++++++++++++++++++++++--
> >   include/drm/ttm/ttm_bo_api.h    |  7 +++++
> >   include/drm/ttm/ttm_bo_driver.h |  7 +++++
> >   3 files changed, 80 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> > index d33a6bb742a1..558ec2cf465d 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct 
> ttm_buffer_object *bo,
> >        return 0;
> >   }
> >
> > +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
> > +                             struct ttm_buffer_object *bo,
> > +                             const struct ttm_place *place)
> > +{
> > +     waiter->tbo = bo;
> > +     memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
> > +     INIT_LIST_HEAD(&waiter->list);
> > +}
> > +
> > +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
> > +                            struct ttm_bo_waiter *waiter)
> > +{
> > +     if (!waiter)
> > +             return;
> > +     spin_lock(&man->wait_lock);
> > +     list_add_tail(&waiter->list, &man->waiter_list);
> > +     spin_unlock(&man->wait_lock);
> > +}
> > +
> > +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
> > +                            struct ttm_bo_waiter *waiter)
> > +{
> > +     if (!waiter)
> > +             return;
> > +     spin_lock(&man->wait_lock);
> > +     if (!list_empty(&waiter->list))
> > +             list_del(&waiter->list);
> > +     spin_unlock(&man->wait_lock);
> > +     kfree(waiter);
> > +}
> > +
> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> > +                  struct ttm_buffer_object *bo,
> > +                  const struct ttm_place *place)
> > +{
> > +     struct ttm_bo_waiter *waiter, *tmp;
> > +
> > +     spin_lock(&man->wait_lock);
> > +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, list) {
> > +             if ((bo != waiter->tbo) &&
> > +                 ((place->fpfn >= waiter->place.fpfn &&
> > +                   place->fpfn <= waiter->place.lpfn) ||
> > +                  (place->lpfn <= waiter->place.lpfn && place->lpfn >=
> > +                   waiter->place.fpfn)))
> > +                 goto later_bo;
> > +     }
> > +     spin_unlock(&man->wait_lock);
> > +     return true;
> > +later_bo:
> > +     spin_unlock(&man->wait_lock);
> > +     return false;
> > +}
> >   /**
> >    * Repeatedly evict memory from the LRU for @mem_type until we 
> create enough
> >    * space, or we've evicted everything and there isn't enough space.
> > @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct 
> ttm_buffer_object *bo,
> >   {
> >        struct ttm_bo_device *bdev = bo->bdev;
> >        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> > +     struct ttm_bo_waiter waiter;
> >        int ret;
> >
> > +     ttm_man_init_waiter(&waiter, bo, place);
> > +     ttm_man_add_waiter(man, &waiter);
> >        do {
> >                ret = (*man->func->get_node)(man, bo, place, mem);
> > -             if (unlikely(ret != 0))
> > +             if (unlikely(ret != 0)) {
> > +                     ttm_man_del_waiter(man, &waiter);
> >                        return ret;
> > -             if (mem->mm_node)
> > +             }
> > +             if (mem->mm_node) {
> > +                     ttm_man_del_waiter(man, &waiter);
> >                        break;
> > +             }
> >                ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
> > -             if (unlikely(ret != 0))
> > +             if (unlikely(ret != 0)) {
> > +                     ttm_man_del_waiter(man, &waiter);
> >                        return ret;
> > +             }
> >        } while (1);
> >        mem->mem_type = mem_type;
> >        return ttm_bo_add_move_fence(bo, man, mem);
> > @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, 
> unsigned type,
> >        man->use_io_reserve_lru = false;
> >        mutex_init(&man->io_reserve_mutex);
> >        spin_lock_init(&man->move_lock);
> > +     spin_lock_init(&man->wait_lock);
> > +     INIT_LIST_HEAD(&man->waiter_list);
> >        INIT_LIST_HEAD(&man->io_reserve_lru);
> >
> >        ret = bdev->driver->init_mem_type(bdev, type, man);
> > diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> > index 2cd025c2abe7..0fce4dbd02e7 100644
> > --- a/include/drm/ttm/ttm_bo_api.h
> > +++ b/include/drm/ttm/ttm_bo_api.h
> > @@ -40,6 +40,7 @@
> >   #include <linux/mm.h>
> >   #include <linux/bitmap.h>
> >   #include <linux/reservation.h>
> > +#include <drm/ttm/ttm_placement.h>
> >
> >   struct ttm_bo_device;
> >
> > @@ -232,6 +233,12 @@ struct ttm_buffer_object {
> >        struct mutex wu_mutex;
> >   };
> >
> > +struct ttm_bo_waiter {
> > +     struct ttm_buffer_object *tbo;
> > +     struct ttm_place place;
> > +     struct list_head list;
> > +};
> > +
> >   /**
> >    * struct ttm_bo_kmap_obj
> >    *
> > diff --git a/include/drm/ttm/ttm_bo_driver.h 
> b/include/drm/ttm/ttm_bo_driver.h
> > index 9b417eb2df20..dc6b8b4c9e06 100644
> > --- a/include/drm/ttm/ttm_bo_driver.h
> > +++ b/include/drm/ttm/ttm_bo_driver.h
> > @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
> >        bool io_reserve_fastpath;
> >        spinlock_t move_lock;
> >
> > +     /* waiters in list */
> > +     spinlock_t wait_lock;
> > +     struct list_head waiter_list;
> > +
> >        /*
> >         * Protected by @io_reserve_mutex:
> >         */
> > @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
> >                     struct ttm_mem_reg *mem,
> >                     struct ttm_operation_ctx *ctx);
> >
> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> > +                  struct ttm_buffer_object *bo,
> > +                  const struct ttm_place *place);
> >   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct 
> ttm_mem_reg *mem);
> >   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
> >                           struct ttm_mem_reg *mem);
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 16276 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 10031 bytes --]

Attached is what you actually want to do cleanly implemented. But as I 
said this is a NO-GO.

Regards,
Christian.

Am 26.01.2018 um 13:43 schrieb Christian König:
>> After my investigation, this issue should be detect of TTM design 
>> self, which breaks scheduling balance.
> Yeah, but again. This is indented design we can't change easily.
>
> Regards,
> Christian.
>
> Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
>> I am off work, so reply mail by phone, the format could not be text.
>>
>> back to topic itself:
>> the problem indeed happen on amdgpu driver, someone reports me that 
>> application runs with two instances, the performance are different.
>> I also reproduced the issue with unit test(bo_eviction_test). They 
>> always think our scheduler isn't working as expected.
>>
>> After my investigation, this issue should be detect of TTM design 
>> self, which breaks scheduling balance.
>>
>> Further, if we run containers for our gpu, container A could run high 
>> score, container B runs low score with same benchmark.
>>
>> So this is bug that we need fix.
>>
>> Regards,
>> David Zhou
>>
>> 发自坚果 Pro
>>
>> Christian K鰊ig <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 于 2018年1月26日 
>> 下午6:31写道：
>>
>> Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
>> > there is a scheduling balance issue about get node like:
>> > a. process A allocates full memory and use it for submission.
>> > b. process B tries to allocates memory, will wait for process A BO 
>> idle in eviction.
>> > c. process A completes the job, process B eviction will put process 
>> A BO node,
>> > but in the meantime, process C is comming to allocate BO, whill 
>> directly get node successfully, and do submission,
>> > process B will again wait for process C BO idle.
>> > d. repeat the above setps, process B could be delayed much more.
>> >
>> > later allocation must not be ahead of front in same place.
>>
>> Again NAK to the whole approach.
>>
>> At least with amdgpu the problem you described above never occurs
>> because evictions are pipelined operations. We could only block for
>> deleted regions to become free.
>>
>> But independent of that incoming memory requests while we make room for
>> eviction are intended to be served first.
>>
>> Changing that is certainly a no-go cause that would favor memory hungry
>> applications over small clients.
>>
>> Regards,
>> Christian.
>>
>> >
>> > Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
>> > Signed-off-by: Chunming Zhou <david1.zhou-5C7GfCeVMHo@public.gmane.org>
>> > ---
>> >   drivers/gpu/drm/ttm/ttm_bo.c    | 69 
>> +++++++++++++++++++++++++++++++++++++++--
>> >   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>> >   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>> >   3 files changed, 80 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
>> b/drivers/gpu/drm/ttm/ttm_bo.c
>> > index d33a6bb742a1..558ec2cf465d 100644
>> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> > @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct 
>> ttm_buffer_object *bo,
>> >        return 0;
>> >   }
>> >
>> > +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
>> > +                             struct ttm_buffer_object *bo,
>> > +                             const struct ttm_place *place)
>> > +{
>> > +     waiter->tbo = bo;
>> > +     memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
>> > +     INIT_LIST_HEAD(&waiter->list);
>> > +}
>> > +
>> > +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
>> > +                            struct ttm_bo_waiter *waiter)
>> > +{
>> > +     if (!waiter)
>> > +             return;
>> > +     spin_lock(&man->wait_lock);
>> > +     list_add_tail(&waiter->list, &man->waiter_list);
>> > +     spin_unlock(&man->wait_lock);
>> > +}
>> > +
>> > +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
>> > +                            struct ttm_bo_waiter *waiter)
>> > +{
>> > +     if (!waiter)
>> > +             return;
>> > +     spin_lock(&man->wait_lock);
>> > +     if (!list_empty(&waiter->list))
>> > +             list_del(&waiter->list);
>> > +     spin_unlock(&man->wait_lock);
>> > +     kfree(waiter);
>> > +}
>> > +
>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>> > +                  struct ttm_buffer_object *bo,
>> > +                  const struct ttm_place *place)
>> > +{
>> > +     struct ttm_bo_waiter *waiter, *tmp;
>> > +
>> > +     spin_lock(&man->wait_lock);
>> > +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, list) {
>> > +             if ((bo != waiter->tbo) &&
>> > +                 ((place->fpfn >= waiter->place.fpfn &&
>> > +                   place->fpfn <= waiter->place.lpfn) ||
>> > +                  (place->lpfn <= waiter->place.lpfn && place->lpfn >=
>> > +                   waiter->place.fpfn)))
>> > +                 goto later_bo;
>> > +     }
>> > +     spin_unlock(&man->wait_lock);
>> > +     return true;
>> > +later_bo:
>> > +     spin_unlock(&man->wait_lock);
>> > +     return false;
>> > +}
>> >   /**
>> >    * Repeatedly evict memory from the LRU for @mem_type until we 
>> create enough
>> >    * space, or we've evicted everything and there isn't enough space.
>> > @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct 
>> ttm_buffer_object *bo,
>> >   {
>> >        struct ttm_bo_device *bdev = bo->bdev;
>> >        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>> > +     struct ttm_bo_waiter waiter;
>> >        int ret;
>> >
>> > +     ttm_man_init_waiter(&waiter, bo, place);
>> > +     ttm_man_add_waiter(man, &waiter);
>> >        do {
>> >                ret = (*man->func->get_node)(man, bo, place, mem);
>> > -             if (unlikely(ret != 0))
>> > +             if (unlikely(ret != 0)) {
>> > +                     ttm_man_del_waiter(man, &waiter);
>> >                        return ret;
>> > -             if (mem->mm_node)
>> > +             }
>> > +             if (mem->mm_node) {
>> > +                     ttm_man_del_waiter(man, &waiter);
>> >                        break;
>> > +             }
>> >                ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
>> > -             if (unlikely(ret != 0))
>> > +             if (unlikely(ret != 0)) {
>> > +                     ttm_man_del_waiter(man, &waiter);
>> >                        return ret;
>> > +             }
>> >        } while (1);
>> >        mem->mem_type = mem_type;
>> >        return ttm_bo_add_move_fence(bo, man, mem);
>> > @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device 
>> *bdev, unsigned type,
>> >        man->use_io_reserve_lru = false;
>> >        mutex_init(&man->io_reserve_mutex);
>> >        spin_lock_init(&man->move_lock);
>> > +     spin_lock_init(&man->wait_lock);
>> > +     INIT_LIST_HEAD(&man->waiter_list);
>> >        INIT_LIST_HEAD(&man->io_reserve_lru);
>> >
>> >        ret = bdev->driver->init_mem_type(bdev, type, man);
>> > diff --git a/include/drm/ttm/ttm_bo_api.h 
>> b/include/drm/ttm/ttm_bo_api.h
>> > index 2cd025c2abe7..0fce4dbd02e7 100644
>> > --- a/include/drm/ttm/ttm_bo_api.h
>> > +++ b/include/drm/ttm/ttm_bo_api.h
>> > @@ -40,6 +40,7 @@
>> >   #include <linux/mm.h>
>> >   #include <linux/bitmap.h>
>> >   #include <linux/reservation.h>
>> > +#include <drm/ttm/ttm_placement.h>
>> >
>> >   struct ttm_bo_device;
>> >
>> > @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>> >        struct mutex wu_mutex;
>> >   };
>> >
>> > +struct ttm_bo_waiter {
>> > +     struct ttm_buffer_object *tbo;
>> > +     struct ttm_place place;
>> > +     struct list_head list;
>> > +};
>> > +
>> >   /**
>> >    * struct ttm_bo_kmap_obj
>> >    *
>> > diff --git a/include/drm/ttm/ttm_bo_driver.h 
>> b/include/drm/ttm/ttm_bo_driver.h
>> > index 9b417eb2df20..dc6b8b4c9e06 100644
>> > --- a/include/drm/ttm/ttm_bo_driver.h
>> > +++ b/include/drm/ttm/ttm_bo_driver.h
>> > @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>> >        bool io_reserve_fastpath;
>> >        spinlock_t move_lock;
>> >
>> > +     /* waiters in list */
>> > +     spinlock_t wait_lock;
>> > +     struct list_head waiter_list;
>> > +
>> >        /*
>> >         * Protected by @io_reserve_mutex:
>> >         */
>> > @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>> >                     struct ttm_mem_reg *mem,
>> >                     struct ttm_operation_ctx *ctx);
>> >
>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>> > +                  struct ttm_buffer_object *bo,
>> > +                  const struct ttm_place *place);
>> >   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct 
>> ttm_mem_reg *mem);
>> >   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>> >                           struct ttm_mem_reg *mem);
>>
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>


[-- Attachment #1.2: Type: text/html, Size: 17574 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-drm-ttm-alloc-only-one-memory-allocation-operation-a.patch --]
[-- Type: text/x-patch; name="0001-drm-ttm-alloc-only-one-memory-allocation-operation-a.patch", Size: 1729 bytes --]

>From 12295de757156469edf37f41e28da656d3e51844 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20K=C3=B6nig?= <christian.koenig-5C7GfCeVMHo@public.gmane.org>
Date: Fri, 26 Jan 2018 14:01:51 +0100
Subject: [PATCH] drm/ttm: alloc only one memory allocation operation at a time
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
---
 drivers/gpu/drm/ttm/ttm_bo.c    | 3 +++
 include/drm/ttm/ttm_bo_driver.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index d33a6bb742a1..b783884eca2b 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1053,7 +1053,9 @@ static int ttm_bo_move_buffer(struct ttm_buffer_object *bo,
 	/*
 	 * Determine where to move the buffer.
 	 */
+	mutex_lock(&bo->bdev->mem_alloc);
 	ret = ttm_bo_mem_space(bo, placement, &mem, ctx);
+	mutex_unlock(&bo->bdev->mem_alloc);
 	if (ret)
 		goto out_unlock;
 	ret = ttm_bo_handle_move_mem(bo, &mem, false, ctx);
@@ -1594,6 +1596,7 @@ int ttm_bo_device_init(struct ttm_bo_device *bdev,
 				    0x10000000);
 	INIT_DELAYED_WORK(&bdev->wq, ttm_bo_delayed_workqueue);
 	INIT_LIST_HEAD(&bdev->ddestroy);
+	mutex_init(&bdev->mem_alloc);
 	bdev->dev_mapping = mapping;
 	bdev->glob = glob;
 	bdev->need_dma32 = need_dma32;
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 9b417eb2df20..a23179d20c25 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -596,6 +596,8 @@ struct ttm_bo_device {
 	bool need_dma32;
 
 	bool no_retry;
+
+	struct mutex mem_alloc;
 };
 
 /**
-- 
2.14.1


[-- Attachment #3: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Zhou, David(ChunMing)]

[-- Attachment #1.1: Type: text/plain, Size: 8851 bytes --]

You patch will prevent concurrent allocation, and will result in allocation performance drop much.


发自坚果 Pro

Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 下午9:04写道：

Attached is what you actually want to do cleanly implemented. But as I said this is a NO-GO.

Regards,
Christian.

Am 26.01.2018 um 13:43 schrieb Christian König:
After my investigation, this issue should be detect of TTM design self, which breaks scheduling balance.
Yeah, but again. This is indented design we can't change easily.

Regards,
Christian.

Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
I am off work, so reply mail by phone, the format could not be text.

back to topic itself:
the problem indeed happen on amdgpu driver, someone reports me that application runs with two instances, the performance are different.
I also reproduced the issue with unit test(bo_eviction_test). They always think our scheduler isn't working as expected.

After my investigation, this issue should be detect of TTM design self, which breaks scheduling balance.

Further, if we run containers for our gpu, container A could run high score, container B runs low score with same benchmark.

So this is bug that we need fix.

Regards,
David Zhou


发自坚果 Pro

Christian K鰊ig <ckoenig.leichtzumerken@gmail.com><mailto:ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 下午6:31写道：

Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
> there is a scheduling balance issue about get node like:
> a. process A allocates full memory and use it for submission.
> b. process B tries to allocates memory, will wait for process A BO idle in eviction.
> c. process A completes the job, process B eviction will put process A BO node,
> but in the meantime, process C is comming to allocate BO, whill directly get node successfully, and do submission,
> process B will again wait for process C BO idle.
> d. repeat the above setps, process B could be delayed much more.
>
> later allocation must not be ahead of front in same place.

Again NAK to the whole approach.

At least with amdgpu the problem you described above never occurs
because evictions are pipelined operations. We could only block for
deleted regions to become free.

But independent of that incoming memory requests while we make room for
eviction are intended to be served first.

Changing that is certainly a no-go cause that would favor memory hungry
applications over small clients.

Regards,
Christian.

>
> Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com><mailto:david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c    | 69 +++++++++++++++++++++++++++++++++++++++--
>   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>   3 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index d33a6bb742a1..558ec2cf465d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
>        return 0;
>   }
>
> +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
> +                             struct ttm_buffer_object *bo,
> +                             const struct ttm_place *place)
> +{
> +     waiter->tbo = bo;
> +     memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
> +     INIT_LIST_HEAD(&waiter->list);
> +}
> +
> +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     list_add_tail(&waiter->list, &man->waiter_list);
> +     spin_unlock(&man->wait_lock);
> +}
> +
> +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     if (!list_empty(&waiter->list))
> +             list_del(&waiter->list);
> +     spin_unlock(&man->wait_lock);
> +     kfree(waiter);
> +}
> +
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place)
> +{
> +     struct ttm_bo_waiter *waiter, *tmp;
> +
> +     spin_lock(&man->wait_lock);
> +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, list) {
> +             if ((bo != waiter->tbo) &&
> +                 ((place->fpfn >= waiter->place.fpfn &&
> +                   place->fpfn <= waiter->place.lpfn) ||
> +                  (place->lpfn <= waiter->place.lpfn && place->lpfn >=
> +                   waiter->place.fpfn)))
> +                 goto later_bo;
> +     }
> +     spin_unlock(&man->wait_lock);
> +     return true;
> +later_bo:
> +     spin_unlock(&man->wait_lock);
> +     return false;
> +}
>   /**
>    * Repeatedly evict memory from the LRU for @mem_type until we create enough
>    * space, or we've evicted everything and there isn't enough space.
> @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>   {
>        struct ttm_bo_device *bdev = bo->bdev;
>        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> +     struct ttm_bo_waiter waiter;
>        int ret;
>
> +     ttm_man_init_waiter(&waiter, bo, place);
> +     ttm_man_add_waiter(man, &waiter);
>        do {
>                ret = (*man->func->get_node)(man, bo, place, mem);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> -             if (mem->mm_node)
> +             }
> +             if (mem->mm_node) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        break;
> +             }
>                ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> +             }
>        } while (1);
>        mem->mem_type = mem_type;
>        return ttm_bo_add_move_fence(bo, man, mem);
> @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
>        man->use_io_reserve_lru = false;
>        mutex_init(&man->io_reserve_mutex);
>        spin_lock_init(&man->move_lock);
> +     spin_lock_init(&man->wait_lock);
> +     INIT_LIST_HEAD(&man->waiter_list);
>        INIT_LIST_HEAD(&man->io_reserve_lru);
>
>        ret = bdev->driver->init_mem_type(bdev, type, man);
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 2cd025c2abe7..0fce4dbd02e7 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -40,6 +40,7 @@
>   #include <linux/mm.h>
>   #include <linux/bitmap.h>
>   #include <linux/reservation.h>
> +#include <drm/ttm/ttm_placement.h>
>
>   struct ttm_bo_device;
>
> @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>        struct mutex wu_mutex;
>   };
>
> +struct ttm_bo_waiter {
> +     struct ttm_buffer_object *tbo;
> +     struct ttm_place place;
> +     struct list_head list;
> +};
> +
>   /**
>    * struct ttm_bo_kmap_obj
>    *
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 9b417eb2df20..dc6b8b4c9e06 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>        bool io_reserve_fastpath;
>        spinlock_t move_lock;
>
> +     /* waiters in list */
> +     spinlock_t wait_lock;
> +     struct list_head waiter_list;
> +
>        /*
>         * Protected by @io_reserve_mutex:
>         */
> @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>                     struct ttm_mem_reg *mem,
>                     struct ttm_operation_ctx *ctx);
>
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place);
>   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem);
>   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>                           struct ttm_mem_reg *mem);




_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[-- Attachment #1.2: Type: text/html, Size: 18689 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Zhou, David(ChunMing)]

[-- Attachment #1.1: Type: text/plain, Size: 8851 bytes --]

You patch will prevent concurrent allocation, and will result in allocation performance drop much.


发自坚果 Pro

Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 下午9:04写道：

Attached is what you actually want to do cleanly implemented. But as I said this is a NO-GO.

Regards,
Christian.

Am 26.01.2018 um 13:43 schrieb Christian König:
After my investigation, this issue should be detect of TTM design self, which breaks scheduling balance.
Yeah, but again. This is indented design we can't change easily.

Regards,
Christian.

Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
I am off work, so reply mail by phone, the format could not be text.

back to topic itself:
the problem indeed happen on amdgpu driver, someone reports me that application runs with two instances, the performance are different.
I also reproduced the issue with unit test(bo_eviction_test). They always think our scheduler isn't working as expected.

After my investigation, this issue should be detect of TTM design self, which breaks scheduling balance.

Further, if we run containers for our gpu, container A could run high score, container B runs low score with same benchmark.

So this is bug that we need fix.

Regards,
David Zhou


发自坚果 Pro

Christian K鰊ig <ckoenig.leichtzumerken@gmail.com><mailto:ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 下午6:31写道：

Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
> there is a scheduling balance issue about get node like:
> a. process A allocates full memory and use it for submission.
> b. process B tries to allocates memory, will wait for process A BO idle in eviction.
> c. process A completes the job, process B eviction will put process A BO node,
> but in the meantime, process C is comming to allocate BO, whill directly get node successfully, and do submission,
> process B will again wait for process C BO idle.
> d. repeat the above setps, process B could be delayed much more.
>
> later allocation must not be ahead of front in same place.

Again NAK to the whole approach.

At least with amdgpu the problem you described above never occurs
because evictions are pipelined operations. We could only block for
deleted regions to become free.

But independent of that incoming memory requests while we make room for
eviction are intended to be served first.

Changing that is certainly a no-go cause that would favor memory hungry
applications over small clients.

Regards,
Christian.

>
> Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com><mailto:david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c    | 69 +++++++++++++++++++++++++++++++++++++++--
>   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>   3 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index d33a6bb742a1..558ec2cf465d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
>        return 0;
>   }
>
> +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
> +                             struct ttm_buffer_object *bo,
> +                             const struct ttm_place *place)
> +{
> +     waiter->tbo = bo;
> +     memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
> +     INIT_LIST_HEAD(&waiter->list);
> +}
> +
> +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     list_add_tail(&waiter->list, &man->waiter_list);
> +     spin_unlock(&man->wait_lock);
> +}
> +
> +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     if (!list_empty(&waiter->list))
> +             list_del(&waiter->list);
> +     spin_unlock(&man->wait_lock);
> +     kfree(waiter);
> +}
> +
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place)
> +{
> +     struct ttm_bo_waiter *waiter, *tmp;
> +
> +     spin_lock(&man->wait_lock);
> +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, list) {
> +             if ((bo != waiter->tbo) &&
> +                 ((place->fpfn >= waiter->place.fpfn &&
> +                   place->fpfn <= waiter->place.lpfn) ||
> +                  (place->lpfn <= waiter->place.lpfn && place->lpfn >=
> +                   waiter->place.fpfn)))
> +                 goto later_bo;
> +     }
> +     spin_unlock(&man->wait_lock);
> +     return true;
> +later_bo:
> +     spin_unlock(&man->wait_lock);
> +     return false;
> +}
>   /**
>    * Repeatedly evict memory from the LRU for @mem_type until we create enough
>    * space, or we've evicted everything and there isn't enough space.
> @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>   {
>        struct ttm_bo_device *bdev = bo->bdev;
>        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> +     struct ttm_bo_waiter waiter;
>        int ret;
>
> +     ttm_man_init_waiter(&waiter, bo, place);
> +     ttm_man_add_waiter(man, &waiter);
>        do {
>                ret = (*man->func->get_node)(man, bo, place, mem);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> -             if (mem->mm_node)
> +             }
> +             if (mem->mm_node) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        break;
> +             }
>                ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> +             }
>        } while (1);
>        mem->mem_type = mem_type;
>        return ttm_bo_add_move_fence(bo, man, mem);
> @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
>        man->use_io_reserve_lru = false;
>        mutex_init(&man->io_reserve_mutex);
>        spin_lock_init(&man->move_lock);
> +     spin_lock_init(&man->wait_lock);
> +     INIT_LIST_HEAD(&man->waiter_list);
>        INIT_LIST_HEAD(&man->io_reserve_lru);
>
>        ret = bdev->driver->init_mem_type(bdev, type, man);
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 2cd025c2abe7..0fce4dbd02e7 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -40,6 +40,7 @@
>   #include <linux/mm.h>
>   #include <linux/bitmap.h>
>   #include <linux/reservation.h>
> +#include <drm/ttm/ttm_placement.h>
>
>   struct ttm_bo_device;
>
> @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>        struct mutex wu_mutex;
>   };
>
> +struct ttm_bo_waiter {
> +     struct ttm_buffer_object *tbo;
> +     struct ttm_place place;
> +     struct list_head list;
> +};
> +
>   /**
>    * struct ttm_bo_kmap_obj
>    *
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 9b417eb2df20..dc6b8b4c9e06 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>        bool io_reserve_fastpath;
>        spinlock_t move_lock;
>
> +     /* waiters in list */
> +     spinlock_t wait_lock;
> +     struct list_head waiter_list;
> +
>        /*
>         * Protected by @io_reserve_mutex:
>         */
> @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>                     struct ttm_mem_reg *mem,
>                     struct ttm_operation_ctx *ctx);
>
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place);
>   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem);
>   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>                           struct ttm_mem_reg *mem);




_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[-- Attachment #1.2: Type: text/html, Size: 18689 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 10910 bytes --]

Yes, exactly that's the problem.

See when you want to prevent a process B from allocating the memory 
process A has evicted, you need to prevent all concurrent allocation.

And we don't do that because it causes a major performance drop.

Regards,
Christian.

Am 26.01.2018 um 14:21 schrieb Zhou, David(ChunMing):
> You patch will prevent concurrent allocation, and will result in 
> allocation performance drop much.
>
> 发自坚果 Pro
>
> Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 
> 下午9:04写道：
>
> Attached is what you actually want to do cleanly implemented. But as I 
> said this is a NO-GO.
>
> Regards,
> Christian.
>
> Am 26.01.2018 um 13:43 schrieb Christian König:
>>> After my investigation, this issue should be detect of TTM design 
>>> self, which breaks scheduling balance.
>> Yeah, but again. This is indented design we can't change easily.
>>
>> Regards,
>> Christian.
>>
>> Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
>>> I am off work, so reply mail by phone, the format could not be text.
>>>
>>> back to topic itself:
>>> the problem indeed happen on amdgpu driver, someone reports me that 
>>> application runs with two instances, the performance are different.
>>> I also reproduced the issue with unit test(bo_eviction_test). They 
>>> always think our scheduler isn't working as expected.
>>>
>>> After my investigation, this issue should be detect of TTM design 
>>> self, which breaks scheduling balance.
>>>
>>> Further, if we run containers for our gpu, container A could run 
>>> high score, container B runs low score with same benchmark.
>>>
>>> So this is bug that we need fix.
>>>
>>> Regards,
>>> David Zhou
>>>
>>> 发自坚果 Pro
>>>
>>> Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 
>>> 下午6:31写道：
>>>
>>> Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
>>> > there is a scheduling balance issue about get node like:
>>> > a. process A allocates full memory and use it for submission.
>>> > b. process B tries to allocates memory, will wait for process A BO 
>>> idle in eviction.
>>> > c. process A completes the job, process B eviction will put 
>>> process A BO node,
>>> > but in the meantime, process C is comming to allocate BO, whill 
>>> directly get node successfully, and do submission,
>>> > process B will again wait for process C BO idle.
>>> > d. repeat the above setps, process B could be delayed much more.
>>> >
>>> > later allocation must not be ahead of front in same place.
>>>
>>> Again NAK to the whole approach.
>>>
>>> At least with amdgpu the problem you described above never occurs
>>> because evictions are pipelined operations. We could only block for
>>> deleted regions to become free.
>>>
>>> But independent of that incoming memory requests while we make room for
>>> eviction are intended to be served first.
>>>
>>> Changing that is certainly a no-go cause that would favor memory hungry
>>> applications over small clients.
>>>
>>> Regards,
>>> Christian.
>>>
>>> >
>>> > Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
>>> > Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>> > ---
>>> >   drivers/gpu/drm/ttm/ttm_bo.c    | 69 
>>> +++++++++++++++++++++++++++++++++++++++--
>>> >   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>>> >   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>>> >   3 files changed, 80 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
>>> b/drivers/gpu/drm/ttm/ttm_bo.c
>>> > index d33a6bb742a1..558ec2cf465d 100644
>>> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>> > @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct 
>>> ttm_buffer_object *bo,
>>> >        return 0;
>>> >   }
>>> >
>>> > +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
>>> > +                             struct ttm_buffer_object *bo,
>>> > +                             const struct ttm_place *place)
>>> > +{
>>> > +     waiter->tbo = bo;
>>> > +     memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
>>> > +     INIT_LIST_HEAD(&waiter->list);
>>> > +}
>>> > +
>>> > +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
>>> > +                            struct ttm_bo_waiter *waiter)
>>> > +{
>>> > +     if (!waiter)
>>> > +             return;
>>> > +     spin_lock(&man->wait_lock);
>>> > +     list_add_tail(&waiter->list, &man->waiter_list);
>>> > +     spin_unlock(&man->wait_lock);
>>> > +}
>>> > +
>>> > +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
>>> > +                            struct ttm_bo_waiter *waiter)
>>> > +{
>>> > +     if (!waiter)
>>> > +             return;
>>> > +     spin_lock(&man->wait_lock);
>>> > +     if (!list_empty(&waiter->list))
>>> > +             list_del(&waiter->list);
>>> > +     spin_unlock(&man->wait_lock);
>>> > +     kfree(waiter);
>>> > +}
>>> > +
>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>> > +                  struct ttm_buffer_object *bo,
>>> > +                  const struct ttm_place *place)
>>> > +{
>>> > +     struct ttm_bo_waiter *waiter, *tmp;
>>> > +
>>> > +     spin_lock(&man->wait_lock);
>>> > +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, list) {
>>> > +             if ((bo != waiter->tbo) &&
>>> > +                 ((place->fpfn >= waiter->place.fpfn &&
>>> > +                   place->fpfn <= waiter->place.lpfn) ||
>>> > +                  (place->lpfn <= waiter->place.lpfn && 
>>> place->lpfn >=
>>> > +                   waiter->place.fpfn)))
>>> > +                 goto later_bo;
>>> > +     }
>>> > +     spin_unlock(&man->wait_lock);
>>> > +     return true;
>>> > +later_bo:
>>> > +     spin_unlock(&man->wait_lock);
>>> > +     return false;
>>> > +}
>>> >   /**
>>> >    * Repeatedly evict memory from the LRU for @mem_type until we 
>>> create enough
>>> >    * space, or we've evicted everything and there isn't enough space.
>>> > @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct 
>>> ttm_buffer_object *bo,
>>> >   {
>>> >        struct ttm_bo_device *bdev = bo->bdev;
>>> >        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>> > +     struct ttm_bo_waiter waiter;
>>> >        int ret;
>>> >
>>> > +     ttm_man_init_waiter(&waiter, bo, place);
>>> > +     ttm_man_add_waiter(man, &waiter);
>>> >        do {
>>> >                ret = (*man->func->get_node)(man, bo, place, mem);
>>> > -             if (unlikely(ret != 0))
>>> > +             if (unlikely(ret != 0)) {
>>> > +                     ttm_man_del_waiter(man, &waiter);
>>> >                        return ret;
>>> > -             if (mem->mm_node)
>>> > +             }
>>> > +             if (mem->mm_node) {
>>> > +                     ttm_man_del_waiter(man, &waiter);
>>> >                        break;
>>> > +             }
>>> >                ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
>>> > -             if (unlikely(ret != 0))
>>> > +             if (unlikely(ret != 0)) {
>>> > +                     ttm_man_del_waiter(man, &waiter);
>>> >                        return ret;
>>> > +             }
>>> >        } while (1);
>>> >        mem->mem_type = mem_type;
>>> >        return ttm_bo_add_move_fence(bo, man, mem);
>>> > @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device 
>>> *bdev, unsigned type,
>>> >        man->use_io_reserve_lru = false;
>>> >        mutex_init(&man->io_reserve_mutex);
>>> >        spin_lock_init(&man->move_lock);
>>> > +     spin_lock_init(&man->wait_lock);
>>> > +     INIT_LIST_HEAD(&man->waiter_list);
>>> > INIT_LIST_HEAD(&man->io_reserve_lru);
>>> >
>>> >        ret = bdev->driver->init_mem_type(bdev, type, man);
>>> > diff --git a/include/drm/ttm/ttm_bo_api.h 
>>> b/include/drm/ttm/ttm_bo_api.h
>>> > index 2cd025c2abe7..0fce4dbd02e7 100644
>>> > --- a/include/drm/ttm/ttm_bo_api.h
>>> > +++ b/include/drm/ttm/ttm_bo_api.h
>>> > @@ -40,6 +40,7 @@
>>> >   #include <linux/mm.h>
>>> >   #include <linux/bitmap.h>
>>> >   #include <linux/reservation.h>
>>> > +#include <drm/ttm/ttm_placement.h>
>>> >
>>> >   struct ttm_bo_device;
>>> >
>>> > @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>>> >        struct mutex wu_mutex;
>>> >   };
>>> >
>>> > +struct ttm_bo_waiter {
>>> > +     struct ttm_buffer_object *tbo;
>>> > +     struct ttm_place place;
>>> > +     struct list_head list;
>>> > +};
>>> > +
>>> >   /**
>>> >    * struct ttm_bo_kmap_obj
>>> >    *
>>> > diff --git a/include/drm/ttm/ttm_bo_driver.h 
>>> b/include/drm/ttm/ttm_bo_driver.h
>>> > index 9b417eb2df20..dc6b8b4c9e06 100644
>>> > --- a/include/drm/ttm/ttm_bo_driver.h
>>> > +++ b/include/drm/ttm/ttm_bo_driver.h
>>> > @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>>> >        bool io_reserve_fastpath;
>>> >        spinlock_t move_lock;
>>> >
>>> > +     /* waiters in list */
>>> > +     spinlock_t wait_lock;
>>> > +     struct list_head waiter_list;
>>> > +
>>> >        /*
>>> >         * Protected by @io_reserve_mutex:
>>> >         */
>>> > @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>>> >                     struct ttm_mem_reg *mem,
>>> >                     struct ttm_operation_ctx *ctx);
>>> >
>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>> > +                  struct ttm_buffer_object *bo,
>>> > +                  const struct ttm_place *place);
>>> >   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct 
>>> ttm_mem_reg *mem);
>>> >   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>>> >                           struct ttm_mem_reg *mem);
>>>
>>>
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


[-- Attachment #1.2: Type: text/html, Size: 21029 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Zhou, David(ChunMing)]

[-- Attachment #1.1: Type: text/plain, Size: 9721 bytes --]

I don't want to prevent all, my new approach is to prevent the later allocation is trying and ahead of front to get the memory space that the front made from eviction.



发自坚果 Pro

Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 下午9:24写道：

Yes, exactly that's the problem.

See when you want to prevent a process B from allocating the memory process A has evicted, you need to prevent all concurrent allocation.

And we don't do that because it causes a major performance drop.

Regards,
Christian.

Am 26.01.2018 um 14:21 schrieb Zhou, David(ChunMing):
You patch will prevent concurrent allocation, and will result in allocation performance drop much.


发自坚果 Pro

Christian K鰊ig <ckoenig.leichtzumerken@gmail.com><mailto:ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 下午9:04写道：

Attached is what you actually want to do cleanly implemented. But as I said this is a NO-GO.

Regards,
Christian.

Am 26.01.2018 um 13:43 schrieb Christian König:
After my investigation, this issue should be detect of TTM design self, which breaks scheduling balance.
Yeah, but again. This is indented design we can't change easily.

Regards,
Christian.

Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
I am off work, so reply mail by phone, the format could not be text.

back to topic itself:
the problem indeed happen on amdgpu driver, someone reports me that application runs with two instances, the performance are different.
I also reproduced the issue with unit test(bo_eviction_test). They always think our scheduler isn't working as expected.

After my investigation, this issue should be detect of TTM design self, which breaks scheduling balance.

Further, if we run containers for our gpu, container A could run high score, container B runs low score with same benchmark.

So this is bug that we need fix.

Regards,
David Zhou


发自坚果 Pro

Christian K鰊ig <ckoenig.leichtzumerken@gmail.com><mailto:ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 下午6:31写道：

Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
> there is a scheduling balance issue about get node like:
> a. process A allocates full memory and use it for submission.
> b. process B tries to allocates memory, will wait for process A BO idle in eviction.
> c. process A completes the job, process B eviction will put process A BO node,
> but in the meantime, process C is comming to allocate BO, whill directly get node successfully, and do submission,
> process B will again wait for process C BO idle.
> d. repeat the above setps, process B could be delayed much more.
>
> later allocation must not be ahead of front in same place.

Again NAK to the whole approach.

At least with amdgpu the problem you described above never occurs
because evictions are pipelined operations. We could only block for
deleted regions to become free.

But independent of that incoming memory requests while we make room for
eviction are intended to be served first.

Changing that is certainly a no-go cause that would favor memory hungry
applications over small clients.

Regards,
Christian.

>
> Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com><mailto:david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c    | 69 +++++++++++++++++++++++++++++++++++++++--
>   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>   3 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index d33a6bb742a1..558ec2cf465d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
>        return 0;
>   }
>
> +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
> +                             struct ttm_buffer_object *bo,
> +                             const struct ttm_place *place)
> +{
> +     waiter->tbo = bo;
> +     memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
> +     INIT_LIST_HEAD(&waiter->list);
> +}
> +
> +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     list_add_tail(&waiter->list, &man->waiter_list);
> +     spin_unlock(&man->wait_lock);
> +}
> +
> +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     if (!list_empty(&waiter->list))
> +             list_del(&waiter->list);
> +     spin_unlock(&man->wait_lock);
> +     kfree(waiter);
> +}
> +
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place)
> +{
> +     struct ttm_bo_waiter *waiter, *tmp;
> +
> +     spin_lock(&man->wait_lock);
> +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, list) {
> +             if ((bo != waiter->tbo) &&
> +                 ((place->fpfn >= waiter->place.fpfn &&
> +                   place->fpfn <= waiter->place.lpfn) ||
> +                  (place->lpfn <= waiter->place.lpfn && place->lpfn >=
> +                   waiter->place.fpfn)))
> +                 goto later_bo;
> +     }
> +     spin_unlock(&man->wait_lock);
> +     return true;
> +later_bo:
> +     spin_unlock(&man->wait_lock);
> +     return false;
> +}
>   /**
>    * Repeatedly evict memory from the LRU for @mem_type until we create enough
>    * space, or we've evicted everything and there isn't enough space.
> @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>   {
>        struct ttm_bo_device *bdev = bo->bdev;
>        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> +     struct ttm_bo_waiter waiter;
>        int ret;
>
> +     ttm_man_init_waiter(&waiter, bo, place);
> +     ttm_man_add_waiter(man, &waiter);
>        do {
>                ret = (*man->func->get_node)(man, bo, place, mem);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> -             if (mem->mm_node)
> +             }
> +             if (mem->mm_node) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        break;
> +             }
>                ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> +             }
>        } while (1);
>        mem->mem_type = mem_type;
>        return ttm_bo_add_move_fence(bo, man, mem);
> @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
>        man->use_io_reserve_lru = false;
>        mutex_init(&man->io_reserve_mutex);
>        spin_lock_init(&man->move_lock);
> +     spin_lock_init(&man->wait_lock);
> +     INIT_LIST_HEAD(&man->waiter_list);
>        INIT_LIST_HEAD(&man->io_reserve_lru);
>
>        ret = bdev->driver->init_mem_type(bdev, type, man);
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 2cd025c2abe7..0fce4dbd02e7 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -40,6 +40,7 @@
>   #include <linux/mm.h>
>   #include <linux/bitmap.h>
>   #include <linux/reservation.h>
> +#include <drm/ttm/ttm_placement.h>
>
>   struct ttm_bo_device;
>
> @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>        struct mutex wu_mutex;
>   };
>
> +struct ttm_bo_waiter {
> +     struct ttm_buffer_object *tbo;
> +     struct ttm_place place;
> +     struct list_head list;
> +};
> +
>   /**
>    * struct ttm_bo_kmap_obj
>    *
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 9b417eb2df20..dc6b8b4c9e06 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>        bool io_reserve_fastpath;
>        spinlock_t move_lock;
>
> +     /* waiters in list */
> +     spinlock_t wait_lock;
> +     struct list_head waiter_list;
> +
>        /*
>         * Protected by @io_reserve_mutex:
>         */
> @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>                     struct ttm_mem_reg *mem,
>                     struct ttm_operation_ctx *ctx);
>
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place);
>   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem);
>   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>                           struct ttm_mem_reg *mem);




_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx






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



[-- Attachment #1.2: Type: text/html, Size: 20877 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Zhou, David(ChunMing)]

[-- Attachment #1.1: Type: text/plain, Size: 9721 bytes --]

I don't want to prevent all, my new approach is to prevent the later allocation is trying and ahead of front to get the memory space that the front made from eviction.



发自坚果 Pro

Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 下午9:24写道：

Yes, exactly that's the problem.

See when you want to prevent a process B from allocating the memory process A has evicted, you need to prevent all concurrent allocation.

And we don't do that because it causes a major performance drop.

Regards,
Christian.

Am 26.01.2018 um 14:21 schrieb Zhou, David(ChunMing):
You patch will prevent concurrent allocation, and will result in allocation performance drop much.


发自坚果 Pro

Christian K鰊ig <ckoenig.leichtzumerken@gmail.com><mailto:ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 下午9:04写道：

Attached is what you actually want to do cleanly implemented. But as I said this is a NO-GO.

Regards,
Christian.

Am 26.01.2018 um 13:43 schrieb Christian König:
After my investigation, this issue should be detect of TTM design self, which breaks scheduling balance.
Yeah, but again. This is indented design we can't change easily.

Regards,
Christian.

Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
I am off work, so reply mail by phone, the format could not be text.

back to topic itself:
the problem indeed happen on amdgpu driver, someone reports me that application runs with two instances, the performance are different.
I also reproduced the issue with unit test(bo_eviction_test). They always think our scheduler isn't working as expected.

After my investigation, this issue should be detect of TTM design self, which breaks scheduling balance.

Further, if we run containers for our gpu, container A could run high score, container B runs low score with same benchmark.

So this is bug that we need fix.

Regards,
David Zhou


发自坚果 Pro

Christian K鰊ig <ckoenig.leichtzumerken@gmail.com><mailto:ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 下午6:31写道：

Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
> there is a scheduling balance issue about get node like:
> a. process A allocates full memory and use it for submission.
> b. process B tries to allocates memory, will wait for process A BO idle in eviction.
> c. process A completes the job, process B eviction will put process A BO node,
> but in the meantime, process C is comming to allocate BO, whill directly get node successfully, and do submission,
> process B will again wait for process C BO idle.
> d. repeat the above setps, process B could be delayed much more.
>
> later allocation must not be ahead of front in same place.

Again NAK to the whole approach.

At least with amdgpu the problem you described above never occurs
because evictions are pipelined operations. We could only block for
deleted regions to become free.

But independent of that incoming memory requests while we make room for
eviction are intended to be served first.

Changing that is certainly a no-go cause that would favor memory hungry
applications over small clients.

Regards,
Christian.

>
> Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
> Signed-off-by: Chunming Zhou <david1.zhou@amd.com><mailto:david1.zhou@amd.com>
> ---
>   drivers/gpu/drm/ttm/ttm_bo.c    | 69 +++++++++++++++++++++++++++++++++++++++--
>   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>   3 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index d33a6bb742a1..558ec2cf465d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct ttm_buffer_object *bo,
>        return 0;
>   }
>
> +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
> +                             struct ttm_buffer_object *bo,
> +                             const struct ttm_place *place)
> +{
> +     waiter->tbo = bo;
> +     memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
> +     INIT_LIST_HEAD(&waiter->list);
> +}
> +
> +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     list_add_tail(&waiter->list, &man->waiter_list);
> +     spin_unlock(&man->wait_lock);
> +}
> +
> +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
> +                            struct ttm_bo_waiter *waiter)
> +{
> +     if (!waiter)
> +             return;
> +     spin_lock(&man->wait_lock);
> +     if (!list_empty(&waiter->list))
> +             list_del(&waiter->list);
> +     spin_unlock(&man->wait_lock);
> +     kfree(waiter);
> +}
> +
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place)
> +{
> +     struct ttm_bo_waiter *waiter, *tmp;
> +
> +     spin_lock(&man->wait_lock);
> +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, list) {
> +             if ((bo != waiter->tbo) &&
> +                 ((place->fpfn >= waiter->place.fpfn &&
> +                   place->fpfn <= waiter->place.lpfn) ||
> +                  (place->lpfn <= waiter->place.lpfn && place->lpfn >=
> +                   waiter->place.fpfn)))
> +                 goto later_bo;
> +     }
> +     spin_unlock(&man->wait_lock);
> +     return true;
> +later_bo:
> +     spin_unlock(&man->wait_lock);
> +     return false;
> +}
>   /**
>    * Repeatedly evict memory from the LRU for @mem_type until we create enough
>    * space, or we've evicted everything and there isn't enough space.
> @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>   {
>        struct ttm_bo_device *bdev = bo->bdev;
>        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
> +     struct ttm_bo_waiter waiter;
>        int ret;
>
> +     ttm_man_init_waiter(&waiter, bo, place);
> +     ttm_man_add_waiter(man, &waiter);
>        do {
>                ret = (*man->func->get_node)(man, bo, place, mem);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> -             if (mem->mm_node)
> +             }
> +             if (mem->mm_node) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        break;
> +             }
>                ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
> -             if (unlikely(ret != 0))
> +             if (unlikely(ret != 0)) {
> +                     ttm_man_del_waiter(man, &waiter);
>                        return ret;
> +             }
>        } while (1);
>        mem->mem_type = mem_type;
>        return ttm_bo_add_move_fence(bo, man, mem);
> @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type,
>        man->use_io_reserve_lru = false;
>        mutex_init(&man->io_reserve_mutex);
>        spin_lock_init(&man->move_lock);
> +     spin_lock_init(&man->wait_lock);
> +     INIT_LIST_HEAD(&man->waiter_list);
>        INIT_LIST_HEAD(&man->io_reserve_lru);
>
>        ret = bdev->driver->init_mem_type(bdev, type, man);
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index 2cd025c2abe7..0fce4dbd02e7 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -40,6 +40,7 @@
>   #include <linux/mm.h>
>   #include <linux/bitmap.h>
>   #include <linux/reservation.h>
> +#include <drm/ttm/ttm_placement.h>
>
>   struct ttm_bo_device;
>
> @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>        struct mutex wu_mutex;
>   };
>
> +struct ttm_bo_waiter {
> +     struct ttm_buffer_object *tbo;
> +     struct ttm_place place;
> +     struct list_head list;
> +};
> +
>   /**
>    * struct ttm_bo_kmap_obj
>    *
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index 9b417eb2df20..dc6b8b4c9e06 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>        bool io_reserve_fastpath;
>        spinlock_t move_lock;
>
> +     /* waiters in list */
> +     spinlock_t wait_lock;
> +     struct list_head waiter_list;
> +
>        /*
>         * Protected by @io_reserve_mutex:
>         */
> @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>                     struct ttm_mem_reg *mem,
>                     struct ttm_operation_ctx *ctx);
>
> +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
> +                  struct ttm_buffer_object *bo,
> +                  const struct ttm_place *place);
>   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem);
>   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>                           struct ttm_mem_reg *mem);




_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx






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



[-- Attachment #1.2: Type: text/html, Size: 20877 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 12005 bytes --]

I know, but this has the same effect. You prevent concurrent allocation 
from happening.

What we could do is to pipeline reusing of deleted memory as well, this 
makes it less likely to cause the problem you are seeing because the 
evicting processes doesn't need to block for deleted BOs any more.

But that other processes can grab memory during eviction is intentional. 
Otherwise greedy processes would completely dominate command submission.

Regards,
Christian.

Am 26.01.2018 um 14:50 schrieb Zhou, David(ChunMing):
> I don't want to prevent all, my new approach is to prevent the later 
> allocation is trying and ahead of front to get the memory space that 
> the front made from eviction.
>
>
> 发自坚果 Pro
>
> Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 
> 下午9:24写道：
>
> Yes, exactly that's the problem.
>
> See when you want to prevent a process B from allocating the memory 
> process A has evicted, you need to prevent all concurrent allocation.
>
> And we don't do that because it causes a major performance drop.
>
> Regards,
> Christian.
>
> Am 26.01.2018 um 14:21 schrieb Zhou, David(ChunMing):
>> You patch will prevent concurrent allocation, and will result in 
>> allocation performance drop much.
>>
>> 发自坚果 Pro
>>
>> Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 
>> 下午9:04写道：
>>
>> Attached is what you actually want to do cleanly implemented. But as 
>> I said this is a NO-GO.
>>
>> Regards,
>> Christian.
>>
>> Am 26.01.2018 um 13:43 schrieb Christian König:
>>>> After my investigation, this issue should be detect of TTM design 
>>>> self, which breaks scheduling balance.
>>> Yeah, but again. This is indented design we can't change easily.
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
>>>> I am off work, so reply mail by phone, the format could not be text.
>>>>
>>>> back to topic itself:
>>>> the problem indeed happen on amdgpu driver, someone reports me that 
>>>> application runs with two instances, the performance are different.
>>>> I also reproduced the issue with unit test(bo_eviction_test). They 
>>>> always think our scheduler isn't working as expected.
>>>>
>>>> After my investigation, this issue should be detect of TTM design 
>>>> self, which breaks scheduling balance.
>>>>
>>>> Further, if we run containers for our gpu, container A could run 
>>>> high score, container B runs low score with same benchmark.
>>>>
>>>> So this is bug that we need fix.
>>>>
>>>> Regards,
>>>> David Zhou
>>>>
>>>> 发自坚果 Pro
>>>>
>>>> Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 
>>>> 下午6:31写道：
>>>>
>>>> Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
>>>> > there is a scheduling balance issue about get node like:
>>>> > a. process A allocates full memory and use it for submission.
>>>> > b. process B tries to allocates memory, will wait for process A 
>>>> BO idle in eviction.
>>>> > c. process A completes the job, process B eviction will put 
>>>> process A BO node,
>>>> > but in the meantime, process C is comming to allocate BO, whill 
>>>> directly get node successfully, and do submission,
>>>> > process B will again wait for process C BO idle.
>>>> > d. repeat the above setps, process B could be delayed much more.
>>>> >
>>>> > later allocation must not be ahead of front in same place.
>>>>
>>>> Again NAK to the whole approach.
>>>>
>>>> At least with amdgpu the problem you described above never occurs
>>>> because evictions are pipelined operations. We could only block for
>>>> deleted regions to become free.
>>>>
>>>> But independent of that incoming memory requests while we make room 
>>>> for
>>>> eviction are intended to be served first.
>>>>
>>>> Changing that is certainly a no-go cause that would favor memory 
>>>> hungry
>>>> applications over small clients.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> >
>>>> > Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
>>>> > Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>>> > ---
>>>> >   drivers/gpu/drm/ttm/ttm_bo.c    | 69 
>>>> +++++++++++++++++++++++++++++++++++++++--
>>>> >   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>>>> >   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>>>> >   3 files changed, 80 insertions(+), 3 deletions(-)
>>>> >
>>>> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
>>>> b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> > index d33a6bb742a1..558ec2cf465d 100644
>>>> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> > @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct 
>>>> ttm_buffer_object *bo,
>>>> >        return 0;
>>>> >   }
>>>> >
>>>> > +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
>>>> > +                             struct ttm_buffer_object *bo,
>>>> > +                             const struct ttm_place *place)
>>>> > +{
>>>> > +     waiter->tbo = bo;
>>>> > +     memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
>>>> > +     INIT_LIST_HEAD(&waiter->list);
>>>> > +}
>>>> > +
>>>> > +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
>>>> > +                            struct ttm_bo_waiter *waiter)
>>>> > +{
>>>> > +     if (!waiter)
>>>> > +             return;
>>>> > +     spin_lock(&man->wait_lock);
>>>> > +     list_add_tail(&waiter->list, &man->waiter_list);
>>>> > +     spin_unlock(&man->wait_lock);
>>>> > +}
>>>> > +
>>>> > +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
>>>> > +                            struct ttm_bo_waiter *waiter)
>>>> > +{
>>>> > +     if (!waiter)
>>>> > +             return;
>>>> > +     spin_lock(&man->wait_lock);
>>>> > +     if (!list_empty(&waiter->list))
>>>> > +             list_del(&waiter->list);
>>>> > +     spin_unlock(&man->wait_lock);
>>>> > +     kfree(waiter);
>>>> > +}
>>>> > +
>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>> > +                  struct ttm_buffer_object *bo,
>>>> > +                  const struct ttm_place *place)
>>>> > +{
>>>> > +     struct ttm_bo_waiter *waiter, *tmp;
>>>> > +
>>>> > +     spin_lock(&man->wait_lock);
>>>> > +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, 
>>>> list) {
>>>> > +             if ((bo != waiter->tbo) &&
>>>> > +                 ((place->fpfn >= waiter->place.fpfn &&
>>>> > +                   place->fpfn <= waiter->place.lpfn) ||
>>>> > +                  (place->lpfn <= waiter->place.lpfn && 
>>>> place->lpfn >=
>>>> > +                   waiter->place.fpfn)))
>>>> > +                 goto later_bo;
>>>> > +     }
>>>> > +     spin_unlock(&man->wait_lock);
>>>> > +     return true;
>>>> > +later_bo:
>>>> > +     spin_unlock(&man->wait_lock);
>>>> > +     return false;
>>>> > +}
>>>> >   /**
>>>> >    * Repeatedly evict memory from the LRU for @mem_type until we 
>>>> create enough
>>>> >    * space, or we've evicted everything and there isn't enough space.
>>>> > @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct 
>>>> ttm_buffer_object *bo,
>>>> >   {
>>>> >        struct ttm_bo_device *bdev = bo->bdev;
>>>> >        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>>> > +     struct ttm_bo_waiter waiter;
>>>> >        int ret;
>>>> >
>>>> > +     ttm_man_init_waiter(&waiter, bo, place);
>>>> > +     ttm_man_add_waiter(man, &waiter);
>>>> >        do {
>>>> >                ret = (*man->func->get_node)(man, bo, place, mem);
>>>> > -             if (unlikely(ret != 0))
>>>> > +             if (unlikely(ret != 0)) {
>>>> > +                     ttm_man_del_waiter(man, &waiter);
>>>> >                        return ret;
>>>> > -             if (mem->mm_node)
>>>> > +             }
>>>> > +             if (mem->mm_node) {
>>>> > +                     ttm_man_del_waiter(man, &waiter);
>>>> >                        break;
>>>> > +             }
>>>> >                ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
>>>> > -             if (unlikely(ret != 0))
>>>> > +             if (unlikely(ret != 0)) {
>>>> > +                     ttm_man_del_waiter(man, &waiter);
>>>> >                        return ret;
>>>> > +             }
>>>> >        } while (1);
>>>> >        mem->mem_type = mem_type;
>>>> >        return ttm_bo_add_move_fence(bo, man, mem);
>>>> > @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device 
>>>> *bdev, unsigned type,
>>>> >        man->use_io_reserve_lru = false;
>>>> > mutex_init(&man->io_reserve_mutex);
>>>> > spin_lock_init(&man->move_lock);
>>>> > +     spin_lock_init(&man->wait_lock);
>>>> > + INIT_LIST_HEAD(&man->waiter_list);
>>>> > INIT_LIST_HEAD(&man->io_reserve_lru);
>>>> >
>>>> >        ret = bdev->driver->init_mem_type(bdev, type, man);
>>>> > diff --git a/include/drm/ttm/ttm_bo_api.h 
>>>> b/include/drm/ttm/ttm_bo_api.h
>>>> > index 2cd025c2abe7..0fce4dbd02e7 100644
>>>> > --- a/include/drm/ttm/ttm_bo_api.h
>>>> > +++ b/include/drm/ttm/ttm_bo_api.h
>>>> > @@ -40,6 +40,7 @@
>>>> >   #include <linux/mm.h>
>>>> >   #include <linux/bitmap.h>
>>>> >   #include <linux/reservation.h>
>>>> > +#include <drm/ttm/ttm_placement.h>
>>>> >
>>>> >   struct ttm_bo_device;
>>>> >
>>>> > @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>>>> >        struct mutex wu_mutex;
>>>> >   };
>>>> >
>>>> > +struct ttm_bo_waiter {
>>>> > +     struct ttm_buffer_object *tbo;
>>>> > +     struct ttm_place place;
>>>> > +     struct list_head list;
>>>> > +};
>>>> > +
>>>> >   /**
>>>> >    * struct ttm_bo_kmap_obj
>>>> >    *
>>>> > diff --git a/include/drm/ttm/ttm_bo_driver.h 
>>>> b/include/drm/ttm/ttm_bo_driver.h
>>>> > index 9b417eb2df20..dc6b8b4c9e06 100644
>>>> > --- a/include/drm/ttm/ttm_bo_driver.h
>>>> > +++ b/include/drm/ttm/ttm_bo_driver.h
>>>> > @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>>>> >        bool io_reserve_fastpath;
>>>> >        spinlock_t move_lock;
>>>> >
>>>> > +     /* waiters in list */
>>>> > +     spinlock_t wait_lock;
>>>> > +     struct list_head waiter_list;
>>>> > +
>>>> >        /*
>>>> >         * Protected by @io_reserve_mutex:
>>>> >         */
>>>> > @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct ttm_buffer_object 
>>>> *bo,
>>>> >                     struct ttm_mem_reg *mem,
>>>> >                     struct ttm_operation_ctx *ctx);
>>>> >
>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>> > +                  struct ttm_buffer_object *bo,
>>>> > +                  const struct ttm_place *place);
>>>> >   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct 
>>>> ttm_mem_reg *mem);
>>>> >   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>>>> >                           struct ttm_mem_reg *mem);
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>


[-- Attachment #1.2: Type: text/html, Size: 25194 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 13743 bytes --]

I just realized that a change I'm thinking about for a while would solve 
your problem as well, but keep concurrent allocation possible.

See ttm_mem_evict_first() unlocks the BO after evicting it:
>         ttm_bo_del_from_lru(bo);
>         spin_unlock(&glob->lru_lock);
>
>         ret = ttm_bo_evict(bo, ctx);
>         if (locked) {
>                 ttm_bo_unreserve(bo); <-------- here
>         } else {
>                 spin_lock(&glob->lru_lock);
>                 ttm_bo_add_to_lru(bo);
>                 spin_unlock(&glob->lru_lock);
>         }
>
>         kref_put(&bo->list_kref, ttm_bo_release_list);

The effect is that in your example process C can not only beat process B 
once, but many many times because we run into a ping/pong situation 
where B evicts resources while C moves them back in.

For a while now I'm thinking about dropping those reservations only 
after the original allocation succeeded.

The effect would be that process C can still beat process B initially, 
but sooner or process B would evict some resources from process C as 
well and then it can succeed with its allocation.

The problem is for this approach to work we need to core change to the 
ww_mutexes to be able to handle this efficiently.

Regards,
Christian.

Am 26.01.2018 um 14:59 schrieb Christian König:
> I know, but this has the same effect. You prevent concurrent 
> allocation from happening.
>
> What we could do is to pipeline reusing of deleted memory as well, 
> this makes it less likely to cause the problem you are seeing because 
> the evicting processes doesn't need to block for deleted BOs any more.
>
> But that other processes can grab memory during eviction is 
> intentional. Otherwise greedy processes would completely dominate 
> command submission.
>
> Regards,
> Christian.
>
> Am 26.01.2018 um 14:50 schrieb Zhou, David(ChunMing):
>> I don't want to prevent all, my new approach is to prevent the later 
>> allocation is trying and ahead of front to get the memory space that 
>> the front made from eviction.
>>
>>
>> 发自坚果 Pro
>>
>> Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 
>> 下午9:24写道：
>>
>> Yes, exactly that's the problem.
>>
>> See when you want to prevent a process B from allocating the memory 
>> process A has evicted, you need to prevent all concurrent allocation.
>>
>> And we don't do that because it causes a major performance drop.
>>
>> Regards,
>> Christian.
>>
>> Am 26.01.2018 um 14:21 schrieb Zhou, David(ChunMing):
>>> You patch will prevent concurrent allocation, and will result in 
>>> allocation performance drop much.
>>>
>>> 发自坚果 Pro
>>>
>>> Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 
>>> 下午9:04写道：
>>>
>>> Attached is what you actually want to do cleanly implemented. But as 
>>> I said this is a NO-GO.
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 26.01.2018 um 13:43 schrieb Christian König:
>>>>> After my investigation, this issue should be detect of TTM design 
>>>>> self, which breaks scheduling balance.
>>>> Yeah, but again. This is indented design we can't change easily.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
>>>>> I am off work, so reply mail by phone, the format could not be text.
>>>>>
>>>>> back to topic itself:
>>>>> the problem indeed happen on amdgpu driver, someone reports me 
>>>>> that application runs with two instances, the performance are 
>>>>> different.
>>>>> I also reproduced the issue with unit test(bo_eviction_test). They 
>>>>> always think our scheduler isn't working as expected.
>>>>>
>>>>> After my investigation, this issue should be detect of TTM design 
>>>>> self, which breaks scheduling balance.
>>>>>
>>>>> Further, if we run containers for our gpu, container A could run 
>>>>> high score, container B runs low score with same benchmark.
>>>>>
>>>>> So this is bug that we need fix.
>>>>>
>>>>> Regards,
>>>>> David Zhou
>>>>>
>>>>> 发自坚果 Pro
>>>>>
>>>>> Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 
>>>>> 下午6:31写道：
>>>>>
>>>>> Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
>>>>> > there is a scheduling balance issue about get node like:
>>>>> > a. process A allocates full memory and use it for submission.
>>>>> > b. process B tries to allocates memory, will wait for process A 
>>>>> BO idle in eviction.
>>>>> > c. process A completes the job, process B eviction will put 
>>>>> process A BO node,
>>>>> > but in the meantime, process C is comming to allocate BO, whill 
>>>>> directly get node successfully, and do submission,
>>>>> > process B will again wait for process C BO idle.
>>>>> > d. repeat the above setps, process B could be delayed much more.
>>>>> >
>>>>> > later allocation must not be ahead of front in same place.
>>>>>
>>>>> Again NAK to the whole approach.
>>>>>
>>>>> At least with amdgpu the problem you described above never occurs
>>>>> because evictions are pipelined operations. We could only block for
>>>>> deleted regions to become free.
>>>>>
>>>>> But independent of that incoming memory requests while we make 
>>>>> room for
>>>>> eviction are intended to be served first.
>>>>>
>>>>> Changing that is certainly a no-go cause that would favor memory 
>>>>> hungry
>>>>> applications over small clients.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>> >
>>>>> > Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
>>>>> > Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>>>> > ---
>>>>> >   drivers/gpu/drm/ttm/ttm_bo.c    | 69 
>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>> >   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>>>>> >   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>>>>> >   3 files changed, 80 insertions(+), 3 deletions(-)
>>>>> >
>>>>> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
>>>>> b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>> > index d33a6bb742a1..558ec2cf465d 100644
>>>>> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>>> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>> > @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct 
>>>>> ttm_buffer_object *bo,
>>>>> >        return 0;
>>>>> >   }
>>>>> >
>>>>> > +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
>>>>> > +                             struct ttm_buffer_object *bo,
>>>>> > +                             const struct ttm_place *place)
>>>>> > +{
>>>>> > +     waiter->tbo = bo;
>>>>> > +     memcpy((void *)&waiter->place, (void *)place, sizeof(*place));
>>>>> > +     INIT_LIST_HEAD(&waiter->list);
>>>>> > +}
>>>>> > +
>>>>> > +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
>>>>> > +                            struct ttm_bo_waiter *waiter)
>>>>> > +{
>>>>> > +     if (!waiter)
>>>>> > +             return;
>>>>> > +     spin_lock(&man->wait_lock);
>>>>> > +     list_add_tail(&waiter->list, &man->waiter_list);
>>>>> > +     spin_unlock(&man->wait_lock);
>>>>> > +}
>>>>> > +
>>>>> > +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
>>>>> > +                            struct ttm_bo_waiter *waiter)
>>>>> > +{
>>>>> > +     if (!waiter)
>>>>> > +             return;
>>>>> > +     spin_lock(&man->wait_lock);
>>>>> > +     if (!list_empty(&waiter->list))
>>>>> > + list_del(&waiter->list);
>>>>> > +     spin_unlock(&man->wait_lock);
>>>>> > +     kfree(waiter);
>>>>> > +}
>>>>> > +
>>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>>> > +                  struct ttm_buffer_object *bo,
>>>>> > +                  const struct ttm_place *place)
>>>>> > +{
>>>>> > +     struct ttm_bo_waiter *waiter, *tmp;
>>>>> > +
>>>>> > +     spin_lock(&man->wait_lock);
>>>>> > +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, 
>>>>> list) {
>>>>> > +             if ((bo != waiter->tbo) &&
>>>>> > +                 ((place->fpfn >= waiter->place.fpfn &&
>>>>> > +                   place->fpfn <= waiter->place.lpfn) ||
>>>>> > +                  (place->lpfn <= waiter->place.lpfn && 
>>>>> place->lpfn >=
>>>>> > + waiter->place.fpfn)))
>>>>> > +                 goto later_bo;
>>>>> > +     }
>>>>> > +     spin_unlock(&man->wait_lock);
>>>>> > +     return true;
>>>>> > +later_bo:
>>>>> > +     spin_unlock(&man->wait_lock);
>>>>> > +     return false;
>>>>> > +}
>>>>> >   /**
>>>>> >    * Repeatedly evict memory from the LRU for @mem_type until we 
>>>>> create enough
>>>>> >    * space, or we've evicted everything and there isn't enough 
>>>>> space.
>>>>> > @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct 
>>>>> ttm_buffer_object *bo,
>>>>> >   {
>>>>> >        struct ttm_bo_device *bdev = bo->bdev;
>>>>> >        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>>>> > +     struct ttm_bo_waiter waiter;
>>>>> >        int ret;
>>>>> >
>>>>> > +     ttm_man_init_waiter(&waiter, bo, place);
>>>>> > +     ttm_man_add_waiter(man, &waiter);
>>>>> >        do {
>>>>> >                ret = (*man->func->get_node)(man, bo, place, mem);
>>>>> > -             if (unlikely(ret != 0))
>>>>> > +             if (unlikely(ret != 0)) {
>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>> >                        return ret;
>>>>> > -             if (mem->mm_node)
>>>>> > +             }
>>>>> > +             if (mem->mm_node) {
>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>> >                        break;
>>>>> > +             }
>>>>> >                ret = ttm_mem_evict_first(bdev, mem_type, place, 
>>>>> ctx);
>>>>> > -             if (unlikely(ret != 0))
>>>>> > +             if (unlikely(ret != 0)) {
>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>> >                        return ret;
>>>>> > +             }
>>>>> >        } while (1);
>>>>> >        mem->mem_type = mem_type;
>>>>> >        return ttm_bo_add_move_fence(bo, man, mem);
>>>>> > @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device 
>>>>> *bdev, unsigned type,
>>>>> >        man->use_io_reserve_lru = false;
>>>>> > mutex_init(&man->io_reserve_mutex);
>>>>> > spin_lock_init(&man->move_lock);
>>>>> > + spin_lock_init(&man->wait_lock);
>>>>> > + INIT_LIST_HEAD(&man->waiter_list);
>>>>> > INIT_LIST_HEAD(&man->io_reserve_lru);
>>>>> >
>>>>> >        ret = bdev->driver->init_mem_type(bdev, type, man);
>>>>> > diff --git a/include/drm/ttm/ttm_bo_api.h 
>>>>> b/include/drm/ttm/ttm_bo_api.h
>>>>> > index 2cd025c2abe7..0fce4dbd02e7 100644
>>>>> > --- a/include/drm/ttm/ttm_bo_api.h
>>>>> > +++ b/include/drm/ttm/ttm_bo_api.h
>>>>> > @@ -40,6 +40,7 @@
>>>>> >   #include <linux/mm.h>
>>>>> >   #include <linux/bitmap.h>
>>>>> >   #include <linux/reservation.h>
>>>>> > +#include <drm/ttm/ttm_placement.h>
>>>>> >
>>>>> >   struct ttm_bo_device;
>>>>> >
>>>>> > @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>>>>> >        struct mutex wu_mutex;
>>>>> >   };
>>>>> >
>>>>> > +struct ttm_bo_waiter {
>>>>> > +     struct ttm_buffer_object *tbo;
>>>>> > +     struct ttm_place place;
>>>>> > +     struct list_head list;
>>>>> > +};
>>>>> > +
>>>>> >   /**
>>>>> >    * struct ttm_bo_kmap_obj
>>>>> >    *
>>>>> > diff --git a/include/drm/ttm/ttm_bo_driver.h 
>>>>> b/include/drm/ttm/ttm_bo_driver.h
>>>>> > index 9b417eb2df20..dc6b8b4c9e06 100644
>>>>> > --- a/include/drm/ttm/ttm_bo_driver.h
>>>>> > +++ b/include/drm/ttm/ttm_bo_driver.h
>>>>> > @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>>>>> >        bool io_reserve_fastpath;
>>>>> >        spinlock_t move_lock;
>>>>> >
>>>>> > +     /* waiters in list */
>>>>> > +     spinlock_t wait_lock;
>>>>> > +     struct list_head waiter_list;
>>>>> > +
>>>>> >        /*
>>>>> >         * Protected by @io_reserve_mutex:
>>>>> >         */
>>>>> > @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct 
>>>>> ttm_buffer_object *bo,
>>>>> >                     struct ttm_mem_reg *mem,
>>>>> >                     struct ttm_operation_ctx *ctx);
>>>>> >
>>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>>> > +                  struct ttm_buffer_object *bo,
>>>>> > +                  const struct ttm_place *place);
>>>>> >   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct 
>>>>> ttm_mem_reg *mem);
>>>>> >   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>>>>> >                           struct ttm_mem_reg *mem);
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@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


[-- Attachment #1.2: Type: text/html, Size: 28825 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Chunming Zhou]

[-- Attachment #1.1: Type: text/plain, Size: 15001 bytes --]



On 2018年01月26日 22:35, Christian König wrote:
> I just realized that a change I'm thinking about for a while would 
> solve your problem as well, but keep concurrent allocation possible.
>
> See ttm_mem_evict_first() unlocks the BO after evicting it:
>>         ttm_bo_del_from_lru(bo);
>>         spin_unlock(&glob->lru_lock);
>>
>>         ret = ttm_bo_evict(bo, ctx);
>>         if (locked) {
>>                 ttm_bo_unreserve(bo); <-------- here
>>         } else {
>>                 spin_lock(&glob->lru_lock);
>>                 ttm_bo_add_to_lru(bo);
>>                 spin_unlock(&glob->lru_lock);
>>         }
>>
>>         kref_put(&bo->list_kref, ttm_bo_release_list);
>
> The effect is that in your example process C can not only beat process 
> B once, but many many times because we run into a ping/pong situation 
> where B evicts resources while C moves them back in.
For ping/pong case, I want to disable busy placement for allocation 
period, only enable it for cs bo validation.

>
> For a while now I'm thinking about dropping those reservations only 
> after the original allocation succeeded.
>
> The effect would be that process C can still beat process B initially, 
> but sooner or process B would evict some resources from process C as 
> well and then it can succeed with its allocation.
If it is from process C cs validation, process B still need evict the 
resource only after process C command submission completion.

>
> The problem is for this approach to work we need to core change to the 
> ww_mutexes to be able to handle this efficiently.
Yes, ww_mutex doesn't support this net lock, which easily deadlock 
without ticket and class.

So I think preventing validation on same place is a simpler way:
process B bo's place is fpfn~lpfn, it will only try to evict LRU BOs in 
that range, while eviction, we just prevent those validation to this 
range(fpfn~lpfn), if out of this range, the allocation/validation still 
can be go on.

Any negative?

Regards,
David Zhou
>
> Regards,
> Christian.
>
> Am 26.01.2018 um 14:59 schrieb Christian König:
>> I know, but this has the same effect. You prevent concurrent 
>> allocation from happening.
>>
>> What we could do is to pipeline reusing of deleted memory as well, 
>> this makes it less likely to cause the problem you are seeing because 
>> the evicting processes doesn't need to block for deleted BOs any more.
>>
>> But that other processes can grab memory during eviction is 
>> intentional. Otherwise greedy processes would completely dominate 
>> command submission.
>>
>> Regards,
>> Christian.
>>
>> Am 26.01.2018 um 14:50 schrieb Zhou, David(ChunMing):
>>> I don't want to prevent all, my new approach is to prevent the later 
>>> allocation is trying and ahead of front to get the memory space that 
>>> the front made from eviction.
>>>
>>>
>>> 发自坚果 Pro
>>>
>>> Christian K鰊ig <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 于 2018年1月26日 
>>> 下午9:24写道：
>>>
>>> Yes, exactly that's the problem.
>>>
>>> See when you want to prevent a process B from allocating the memory 
>>> process A has evicted, you need to prevent all concurrent allocation.
>>>
>>> And we don't do that because it causes a major performance drop.
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 26.01.2018 um 14:21 schrieb Zhou, David(ChunMing):
>>>> You patch will prevent concurrent allocation, and will result in 
>>>> allocation performance drop much.
>>>>
>>>> 发自坚果 Pro
>>>>
>>>> Christian K鰊ig <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 于 2018年1月26日 
>>>> 下午9:04写道：
>>>>
>>>> Attached is what you actually want to do cleanly implemented. But 
>>>> as I said this is a NO-GO.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 26.01.2018 um 13:43 schrieb Christian König:
>>>>>> After my investigation, this issue should be detect of TTM design 
>>>>>> self, which breaks scheduling balance.
>>>>> Yeah, but again. This is indented design we can't change easily.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>> Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
>>>>>> I am off work, so reply mail by phone, the format could not be text.
>>>>>>
>>>>>> back to topic itself:
>>>>>> the problem indeed happen on amdgpu driver, someone reports me 
>>>>>> that application runs with two instances, the performance are 
>>>>>> different.
>>>>>> I also reproduced the issue with unit test(bo_eviction_test). 
>>>>>> They always think our scheduler isn't working as expected.
>>>>>>
>>>>>> After my investigation, this issue should be detect of TTM design 
>>>>>> self, which breaks scheduling balance.
>>>>>>
>>>>>> Further, if we run containers for our gpu, container A could run 
>>>>>> high score, container B runs low score with same benchmark.
>>>>>>
>>>>>> So this is bug that we need fix.
>>>>>>
>>>>>> Regards,
>>>>>> David Zhou
>>>>>>
>>>>>> 发自坚果 Pro
>>>>>>
>>>>>> Christian K鰊ig <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 于 2018年1月26日 
>>>>>> 下午6:31写道：
>>>>>>
>>>>>> Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
>>>>>> > there is a scheduling balance issue about get node like:
>>>>>> > a. process A allocates full memory and use it for submission.
>>>>>> > b. process B tries to allocates memory, will wait for process A 
>>>>>> BO idle in eviction.
>>>>>> > c. process A completes the job, process B eviction will put 
>>>>>> process A BO node,
>>>>>> > but in the meantime, process C is comming to allocate BO, whill 
>>>>>> directly get node successfully, and do submission,
>>>>>> > process B will again wait for process C BO idle.
>>>>>> > d. repeat the above setps, process B could be delayed much more.
>>>>>> >
>>>>>> > later allocation must not be ahead of front in same place.
>>>>>>
>>>>>> Again NAK to the whole approach.
>>>>>>
>>>>>> At least with amdgpu the problem you described above never occurs
>>>>>> because evictions are pipelined operations. We could only block for
>>>>>> deleted regions to become free.
>>>>>>
>>>>>> But independent of that incoming memory requests while we make 
>>>>>> room for
>>>>>> eviction are intended to be served first.
>>>>>>
>>>>>> Changing that is certainly a no-go cause that would favor memory 
>>>>>> hungry
>>>>>> applications over small clients.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>> >
>>>>>> > Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
>>>>>> > Signed-off-by: Chunming Zhou <david1.zhou-5C7GfCeVMHo@public.gmane.org>
>>>>>> > ---
>>>>>> >   drivers/gpu/drm/ttm/ttm_bo.c    | 69 
>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>> >   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>>>>>> >   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>>>>>> >   3 files changed, 80 insertions(+), 3 deletions(-)
>>>>>> >
>>>>>> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
>>>>>> b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>> > index d33a6bb742a1..558ec2cf465d 100644
>>>>>> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>> > @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct 
>>>>>> ttm_buffer_object *bo,
>>>>>> >        return 0;
>>>>>> >   }
>>>>>> >
>>>>>> > +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
>>>>>> > +                             struct ttm_buffer_object *bo,
>>>>>> > +                             const struct ttm_place *place)
>>>>>> > +{
>>>>>> > +     waiter->tbo = bo;
>>>>>> > +     memcpy((void *)&waiter->place, (void *)place, 
>>>>>> sizeof(*place));
>>>>>> > + INIT_LIST_HEAD(&waiter->list);
>>>>>> > +}
>>>>>> > +
>>>>>> > +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
>>>>>> > +                            struct ttm_bo_waiter *waiter)
>>>>>> > +{
>>>>>> > +     if (!waiter)
>>>>>> > +             return;
>>>>>> > +     spin_lock(&man->wait_lock);
>>>>>> > +     list_add_tail(&waiter->list, &man->waiter_list);
>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>> > +}
>>>>>> > +
>>>>>> > +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
>>>>>> > +                            struct ttm_bo_waiter *waiter)
>>>>>> > +{
>>>>>> > +     if (!waiter)
>>>>>> > +             return;
>>>>>> > +     spin_lock(&man->wait_lock);
>>>>>> > +     if (!list_empty(&waiter->list))
>>>>>> > + list_del(&waiter->list);
>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>> > +     kfree(waiter);
>>>>>> > +}
>>>>>> > +
>>>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>>>> > +                  struct ttm_buffer_object *bo,
>>>>>> > +                  const struct ttm_place *place)
>>>>>> > +{
>>>>>> > +     struct ttm_bo_waiter *waiter, *tmp;
>>>>>> > +
>>>>>> > +     spin_lock(&man->wait_lock);
>>>>>> > +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, 
>>>>>> list) {
>>>>>> > +             if ((bo != waiter->tbo) &&
>>>>>> > +                 ((place->fpfn >= waiter->place.fpfn &&
>>>>>> > +                   place->fpfn <= waiter->place.lpfn) ||
>>>>>> > +                  (place->lpfn <= waiter->place.lpfn && 
>>>>>> place->lpfn >=
>>>>>> > + waiter->place.fpfn)))
>>>>>> > +                 goto later_bo;
>>>>>> > +     }
>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>> > +     return true;
>>>>>> > +later_bo:
>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>> > +     return false;
>>>>>> > +}
>>>>>> >   /**
>>>>>> >    * Repeatedly evict memory from the LRU for @mem_type until 
>>>>>> we create enough
>>>>>> >    * space, or we've evicted everything and there isn't enough 
>>>>>> space.
>>>>>> > @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct 
>>>>>> ttm_buffer_object *bo,
>>>>>> >   {
>>>>>> >        struct ttm_bo_device *bdev = bo->bdev;
>>>>>> >        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>>>>> > +     struct ttm_bo_waiter waiter;
>>>>>> >        int ret;
>>>>>> >
>>>>>> > +     ttm_man_init_waiter(&waiter, bo, place);
>>>>>> > +     ttm_man_add_waiter(man, &waiter);
>>>>>> >        do {
>>>>>> >                ret = (*man->func->get_node)(man, bo, place, mem);
>>>>>> > -             if (unlikely(ret != 0))
>>>>>> > +             if (unlikely(ret != 0)) {
>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>> >                        return ret;
>>>>>> > -             if (mem->mm_node)
>>>>>> > +             }
>>>>>> > +             if (mem->mm_node) {
>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>> >                        break;
>>>>>> > +             }
>>>>>> >                ret = ttm_mem_evict_first(bdev, mem_type, place, 
>>>>>> ctx);
>>>>>> > -             if (unlikely(ret != 0))
>>>>>> > +             if (unlikely(ret != 0)) {
>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>> >                        return ret;
>>>>>> > +             }
>>>>>> >        } while (1);
>>>>>> >        mem->mem_type = mem_type;
>>>>>> >        return ttm_bo_add_move_fence(bo, man, mem);
>>>>>> > @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device 
>>>>>> *bdev, unsigned type,
>>>>>> >        man->use_io_reserve_lru = false;
>>>>>> > mutex_init(&man->io_reserve_mutex);
>>>>>> > spin_lock_init(&man->move_lock);
>>>>>> > + spin_lock_init(&man->wait_lock);
>>>>>> > + INIT_LIST_HEAD(&man->waiter_list);
>>>>>> > INIT_LIST_HEAD(&man->io_reserve_lru);
>>>>>> >
>>>>>> >        ret = bdev->driver->init_mem_type(bdev, type, man);
>>>>>> > diff --git a/include/drm/ttm/ttm_bo_api.h 
>>>>>> b/include/drm/ttm/ttm_bo_api.h
>>>>>> > index 2cd025c2abe7..0fce4dbd02e7 100644
>>>>>> > --- a/include/drm/ttm/ttm_bo_api.h
>>>>>> > +++ b/include/drm/ttm/ttm_bo_api.h
>>>>>> > @@ -40,6 +40,7 @@
>>>>>> >   #include <linux/mm.h>
>>>>>> >   #include <linux/bitmap.h>
>>>>>> >   #include <linux/reservation.h>
>>>>>> > +#include <drm/ttm/ttm_placement.h>
>>>>>> >
>>>>>> >   struct ttm_bo_device;
>>>>>> >
>>>>>> > @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>>>>>> >        struct mutex wu_mutex;
>>>>>> >   };
>>>>>> >
>>>>>> > +struct ttm_bo_waiter {
>>>>>> > +     struct ttm_buffer_object *tbo;
>>>>>> > +     struct ttm_place place;
>>>>>> > +     struct list_head list;
>>>>>> > +};
>>>>>> > +
>>>>>> >   /**
>>>>>> >    * struct ttm_bo_kmap_obj
>>>>>> >    *
>>>>>> > diff --git a/include/drm/ttm/ttm_bo_driver.h 
>>>>>> b/include/drm/ttm/ttm_bo_driver.h
>>>>>> > index 9b417eb2df20..dc6b8b4c9e06 100644
>>>>>> > --- a/include/drm/ttm/ttm_bo_driver.h
>>>>>> > +++ b/include/drm/ttm/ttm_bo_driver.h
>>>>>> > @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>>>>>> >        bool io_reserve_fastpath;
>>>>>> >        spinlock_t move_lock;
>>>>>> >
>>>>>> > +     /* waiters in list */
>>>>>> > +     spinlock_t wait_lock;
>>>>>> > +     struct list_head waiter_list;
>>>>>> > +
>>>>>> >        /*
>>>>>> >         * Protected by @io_reserve_mutex:
>>>>>> >         */
>>>>>> > @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct 
>>>>>> ttm_buffer_object *bo,
>>>>>> >                     struct ttm_mem_reg *mem,
>>>>>> >                     struct ttm_operation_ctx *ctx);
>>>>>> >
>>>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>>>> > +                  struct ttm_buffer_object *bo,
>>>>>> > +                  const struct ttm_place *place);
>>>>>> >   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct 
>>>>>> ttm_mem_reg *mem);
>>>>>> >   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>>>>>> >                           struct ttm_mem_reg *mem);
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>


[-- Attachment #1.2: Type: text/html, Size: 32106 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 16049 bytes --]

> So I think preventing validation on same place is a simpler way:
> process B bo's place is fpfn~lpfn, it will only try to evict LRU BOs 
> in that range, while eviction, we just prevent those validation to 
> this range(fpfn~lpfn), if out of this range, the allocation/validation 
> still can be go on.
>
> Any negative?
That won't work either. The most common use of fpfn~lpfn range is to 
limit a BO to visible VRAM, the other use cases are to fullfil hardware 
limitations.

So blocking this would result in blocking all normal GTT and VRAM 
allocations, adding a mutex to validate would have the same effect.

Regards,
Christian.

Am 31.01.2018 um 11:30 schrieb Chunming Zhou:
>
>
>
> On 2018年01月26日 22:35, Christian König wrote:
>> I just realized that a change I'm thinking about for a while would 
>> solve your problem as well, but keep concurrent allocation possible.
>>
>> See ttm_mem_evict_first() unlocks the BO after evicting it:
>>>         ttm_bo_del_from_lru(bo);
>>>         spin_unlock(&glob->lru_lock);
>>>
>>>         ret = ttm_bo_evict(bo, ctx);
>>>         if (locked) {
>>>                 ttm_bo_unreserve(bo); <-------- here
>>>         } else {
>>>                 spin_lock(&glob->lru_lock);
>>>                 ttm_bo_add_to_lru(bo);
>>>                 spin_unlock(&glob->lru_lock);
>>>         }
>>>
>>>         kref_put(&bo->list_kref, ttm_bo_release_list);
>>
>> The effect is that in your example process C can not only beat 
>> process B once, but many many times because we run into a ping/pong 
>> situation where B evicts resources while C moves them back in.
> For ping/pong case, I want to disable busy placement for allocation 
> period, only enable it for cs bo validation.
>
>>
>> For a while now I'm thinking about dropping those reservations only 
>> after the original allocation succeeded.
>>
>> The effect would be that process C can still beat process B 
>> initially, but sooner or process B would evict some resources from 
>> process C as well and then it can succeed with its allocation.
> If it is from process C cs validation, process B still need evict the 
> resource only after process C command submission completion.
>
>>
>> The problem is for this approach to work we need to core change to 
>> the ww_mutexes to be able to handle this efficiently.
> Yes, ww_mutex doesn't support this net lock, which easily deadlock 
> without ticket and class.
>
> So I think preventing validation on same place is a simpler way:
> process B bo's place is fpfn~lpfn, it will only try to evict LRU BOs 
> in that range, while eviction, we just prevent those validation to 
> this range(fpfn~lpfn), if out of this range, the allocation/validation 
> still can be go on.
>
> Any negative?
>
> Regards,
> David Zhou
>>
>> Regards,
>> Christian.
>>
>> Am 26.01.2018 um 14:59 schrieb Christian König:
>>> I know, but this has the same effect. You prevent concurrent 
>>> allocation from happening.
>>>
>>> What we could do is to pipeline reusing of deleted memory as well, 
>>> this makes it less likely to cause the problem you are seeing 
>>> because the evicting processes doesn't need to block for deleted BOs 
>>> any more.
>>>
>>> But that other processes can grab memory during eviction is 
>>> intentional. Otherwise greedy processes would completely dominate 
>>> command submission.
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 26.01.2018 um 14:50 schrieb Zhou, David(ChunMing):
>>>> I don't want to prevent all, my new approach is to prevent the 
>>>> later allocation is trying and ahead of front to get the memory 
>>>> space that the front made from eviction.
>>>>
>>>>
>>>> 发自坚果 Pro
>>>>
>>>> Christian K鰊ig <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 于 2018年1月26日 
>>>> 下午9:24写道：
>>>>
>>>> Yes, exactly that's the problem.
>>>>
>>>> See when you want to prevent a process B from allocating the memory 
>>>> process A has evicted, you need to prevent all concurrent allocation.
>>>>
>>>> And we don't do that because it causes a major performance drop.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 26.01.2018 um 14:21 schrieb Zhou, David(ChunMing):
>>>>> You patch will prevent concurrent allocation, and will result in 
>>>>> allocation performance drop much.
>>>>>
>>>>> 发自坚果 Pro
>>>>>
>>>>> Christian K鰊ig <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 于 2018年1月26日 
>>>>> 下午9:04写道：
>>>>>
>>>>> Attached is what you actually want to do cleanly implemented. But 
>>>>> as I said this is a NO-GO.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>> Am 26.01.2018 um 13:43 schrieb Christian König:
>>>>>>> After my investigation, this issue should be detect of TTM 
>>>>>>> design self, which breaks scheduling balance.
>>>>>> Yeah, but again. This is indented design we can't change easily.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>> Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
>>>>>>> I am off work, so reply mail by phone, the format could not be text.
>>>>>>>
>>>>>>> back to topic itself:
>>>>>>> the problem indeed happen on amdgpu driver, someone reports me 
>>>>>>> that application runs with two instances, the performance are 
>>>>>>> different.
>>>>>>> I also reproduced the issue with unit test(bo_eviction_test). 
>>>>>>> They always think our scheduler isn't working as expected.
>>>>>>>
>>>>>>> After my investigation, this issue should be detect of TTM 
>>>>>>> design self, which breaks scheduling balance.
>>>>>>>
>>>>>>> Further, if we run containers for our gpu, container A could run 
>>>>>>> high score, container B runs low score with same benchmark.
>>>>>>>
>>>>>>> So this is bug that we need fix.
>>>>>>>
>>>>>>> Regards,
>>>>>>> David Zhou
>>>>>>>
>>>>>>> 发自坚果 Pro
>>>>>>>
>>>>>>> Christian K鰊ig <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 于 2018年1月26日 
>>>>>>> 下午6:31写道：
>>>>>>>
>>>>>>> Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
>>>>>>> > there is a scheduling balance issue about get node like:
>>>>>>> > a. process A allocates full memory and use it for submission.
>>>>>>> > b. process B tries to allocates memory, will wait for process 
>>>>>>> A BO idle in eviction.
>>>>>>> > c. process A completes the job, process B eviction will put 
>>>>>>> process A BO node,
>>>>>>> > but in the meantime, process C is comming to allocate BO, 
>>>>>>> whill directly get node successfully, and do submission,
>>>>>>> > process B will again wait for process C BO idle.
>>>>>>> > d. repeat the above setps, process B could be delayed much more.
>>>>>>> >
>>>>>>> > later allocation must not be ahead of front in same place.
>>>>>>>
>>>>>>> Again NAK to the whole approach.
>>>>>>>
>>>>>>> At least with amdgpu the problem you described above never occurs
>>>>>>> because evictions are pipelined operations. We could only block for
>>>>>>> deleted regions to become free.
>>>>>>>
>>>>>>> But independent of that incoming memory requests while we make 
>>>>>>> room for
>>>>>>> eviction are intended to be served first.
>>>>>>>
>>>>>>> Changing that is certainly a no-go cause that would favor memory 
>>>>>>> hungry
>>>>>>> applications over small clients.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>> >
>>>>>>> > Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
>>>>>>> > Signed-off-by: Chunming Zhou <david1.zhou-5C7GfCeVMHo@public.gmane.org>
>>>>>>> > ---
>>>>>>> >   drivers/gpu/drm/ttm/ttm_bo.c    | 69 
>>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>>> >   include/drm/ttm/ttm_bo_api.h    |  7 +++++
>>>>>>> >   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>>>>>>> >   3 files changed, 80 insertions(+), 3 deletions(-)
>>>>>>> >
>>>>>>> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
>>>>>>> b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>>> > index d33a6bb742a1..558ec2cf465d 100644
>>>>>>> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>>> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>>> > @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct 
>>>>>>> ttm_buffer_object *bo,
>>>>>>> >        return 0;
>>>>>>> >   }
>>>>>>> >
>>>>>>> > +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
>>>>>>> > +                             struct ttm_buffer_object *bo,
>>>>>>> > +                             const struct ttm_place *place)
>>>>>>> > +{
>>>>>>> > +     waiter->tbo = bo;
>>>>>>> > +     memcpy((void *)&waiter->place, (void *)place, 
>>>>>>> sizeof(*place));
>>>>>>> > + INIT_LIST_HEAD(&waiter->list);
>>>>>>> > +}
>>>>>>> > +
>>>>>>> > +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
>>>>>>> > +                            struct ttm_bo_waiter *waiter)
>>>>>>> > +{
>>>>>>> > +     if (!waiter)
>>>>>>> > +             return;
>>>>>>> > + spin_lock(&man->wait_lock);
>>>>>>> > + list_add_tail(&waiter->list, &man->waiter_list);
>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>> > +}
>>>>>>> > +
>>>>>>> > +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
>>>>>>> > +                            struct ttm_bo_waiter *waiter)
>>>>>>> > +{
>>>>>>> > +     if (!waiter)
>>>>>>> > +             return;
>>>>>>> > + spin_lock(&man->wait_lock);
>>>>>>> > +     if (!list_empty(&waiter->list))
>>>>>>> > + list_del(&waiter->list);
>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>> > +     kfree(waiter);
>>>>>>> > +}
>>>>>>> > +
>>>>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>>>>> > +                  struct ttm_buffer_object *bo,
>>>>>>> > +                  const struct ttm_place *place)
>>>>>>> > +{
>>>>>>> > +     struct ttm_bo_waiter *waiter, *tmp;
>>>>>>> > +
>>>>>>> > + spin_lock(&man->wait_lock);
>>>>>>> > +     list_for_each_entry_safe(waiter, tmp, &man->waiter_list, 
>>>>>>> list) {
>>>>>>> > +             if ((bo != waiter->tbo) &&
>>>>>>> > +                 ((place->fpfn >= waiter->place.fpfn &&
>>>>>>> > +                   place->fpfn <= waiter->place.lpfn) ||
>>>>>>> > +                  (place->lpfn <= waiter->place.lpfn && 
>>>>>>> place->lpfn >=
>>>>>>> > + waiter->place.fpfn)))
>>>>>>> > +                 goto later_bo;
>>>>>>> > +     }
>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>> > +     return true;
>>>>>>> > +later_bo:
>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>> > +     return false;
>>>>>>> > +}
>>>>>>> >   /**
>>>>>>> >    * Repeatedly evict memory from the LRU for @mem_type until 
>>>>>>> we create enough
>>>>>>> >    * space, or we've evicted everything and there isn't enough 
>>>>>>> space.
>>>>>>> > @@ -853,17 +905,26 @@ static int ttm_bo_mem_force_space(struct 
>>>>>>> ttm_buffer_object *bo,
>>>>>>> >   {
>>>>>>> >        struct ttm_bo_device *bdev = bo->bdev;
>>>>>>> >        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>>>>>> > +     struct ttm_bo_waiter waiter;
>>>>>>> >        int ret;
>>>>>>> >
>>>>>>> > +     ttm_man_init_waiter(&waiter, bo, place);
>>>>>>> > +     ttm_man_add_waiter(man, &waiter);
>>>>>>> >        do {
>>>>>>> >                ret = (*man->func->get_node)(man, bo, place, mem);
>>>>>>> > -             if (unlikely(ret != 0))
>>>>>>> > +             if (unlikely(ret != 0)) {
>>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>>> >                        return ret;
>>>>>>> > -             if (mem->mm_node)
>>>>>>> > +             }
>>>>>>> > +             if (mem->mm_node) {
>>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>>> >                        break;
>>>>>>> > +             }
>>>>>>> >                ret = ttm_mem_evict_first(bdev, mem_type, 
>>>>>>> place, ctx);
>>>>>>> > -             if (unlikely(ret != 0))
>>>>>>> > +             if (unlikely(ret != 0)) {
>>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>>> >                        return ret;
>>>>>>> > +             }
>>>>>>> >        } while (1);
>>>>>>> >        mem->mem_type = mem_type;
>>>>>>> >        return ttm_bo_add_move_fence(bo, man, mem);
>>>>>>> > @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device 
>>>>>>> *bdev, unsigned type,
>>>>>>> >        man->use_io_reserve_lru = false;
>>>>>>> > mutex_init(&man->io_reserve_mutex);
>>>>>>> > spin_lock_init(&man->move_lock);
>>>>>>> > + spin_lock_init(&man->wait_lock);
>>>>>>> > + INIT_LIST_HEAD(&man->waiter_list);
>>>>>>> > INIT_LIST_HEAD(&man->io_reserve_lru);
>>>>>>> >
>>>>>>> >        ret = bdev->driver->init_mem_type(bdev, type, man);
>>>>>>> > diff --git a/include/drm/ttm/ttm_bo_api.h 
>>>>>>> b/include/drm/ttm/ttm_bo_api.h
>>>>>>> > index 2cd025c2abe7..0fce4dbd02e7 100644
>>>>>>> > --- a/include/drm/ttm/ttm_bo_api.h
>>>>>>> > +++ b/include/drm/ttm/ttm_bo_api.h
>>>>>>> > @@ -40,6 +40,7 @@
>>>>>>> >   #include <linux/mm.h>
>>>>>>> >   #include <linux/bitmap.h>
>>>>>>> >   #include <linux/reservation.h>
>>>>>>> > +#include <drm/ttm/ttm_placement.h>
>>>>>>> >
>>>>>>> >   struct ttm_bo_device;
>>>>>>> >
>>>>>>> > @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>>>>>>> >        struct mutex wu_mutex;
>>>>>>> >   };
>>>>>>> >
>>>>>>> > +struct ttm_bo_waiter {
>>>>>>> > +     struct ttm_buffer_object *tbo;
>>>>>>> > +     struct ttm_place place;
>>>>>>> > +     struct list_head list;
>>>>>>> > +};
>>>>>>> > +
>>>>>>> >   /**
>>>>>>> >    * struct ttm_bo_kmap_obj
>>>>>>> >    *
>>>>>>> > diff --git a/include/drm/ttm/ttm_bo_driver.h 
>>>>>>> b/include/drm/ttm/ttm_bo_driver.h
>>>>>>> > index 9b417eb2df20..dc6b8b4c9e06 100644
>>>>>>> > --- a/include/drm/ttm/ttm_bo_driver.h
>>>>>>> > +++ b/include/drm/ttm/ttm_bo_driver.h
>>>>>>> > @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>>>>>>> >        bool io_reserve_fastpath;
>>>>>>> >        spinlock_t move_lock;
>>>>>>> >
>>>>>>> > +     /* waiters in list */
>>>>>>> > +     spinlock_t wait_lock;
>>>>>>> > +     struct list_head waiter_list;
>>>>>>> > +
>>>>>>> >        /*
>>>>>>> >         * Protected by @io_reserve_mutex:
>>>>>>> >         */
>>>>>>> > @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct 
>>>>>>> ttm_buffer_object *bo,
>>>>>>> >                     struct ttm_mem_reg *mem,
>>>>>>> >                     struct ttm_operation_ctx *ctx);
>>>>>>> >
>>>>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>>>>> > +                  struct ttm_buffer_object *bo,
>>>>>>> > +                  const struct ttm_place *place);
>>>>>>> >   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct 
>>>>>>> ttm_mem_reg *mem);
>>>>>>> >   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>>>>>>> >                           struct ttm_mem_reg *mem);
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> amd-gfx mailing list
>>>>>>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>


[-- Attachment #1.2: Type: text/html, Size: 34496 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Chunming Zhou]

[-- Attachment #1.1: Type: text/plain, Size: 16567 bytes --]



On 2018年01月31日 18:54, Christian König wrote:
>> So I think preventing validation on same place is a simpler way:
>> process B bo's place is fpfn~lpfn, it will only try to evict LRU BOs 
>> in that range, while eviction, we just prevent those validation to 
>> this range(fpfn~lpfn), if out of this range, the 
>> allocation/validation still can be go on.
>>
>> Any negative?
> That won't work either. The most common use of fpfn~lpfn range is to 
> limit a BO to visible VRAM, the other use cases are to fullfil 
> hardware limitations.
>
> So blocking this would result in blocking all normal GTT and VRAM 
> allocations, adding a mutex to validate would have the same effect.
No, different effect, mutex will make the every allocation serialized. 
My approach only effects busy case, that is said, only when space is 
used up, the allocation is serialized in that range, otherwise still in 
parallel.

Regards,
David Zhou


>
> Regards,
> Christian.
>
> Am 31.01.2018 um 11:30 schrieb Chunming Zhou:
>>
>>
>>
>> On 2018年01月26日 22:35, Christian König wrote:
>>> I just realized that a change I'm thinking about for a while would 
>>> solve your problem as well, but keep concurrent allocation possible.
>>>
>>> See ttm_mem_evict_first() unlocks the BO after evicting it:
>>>>         ttm_bo_del_from_lru(bo);
>>>>         spin_unlock(&glob->lru_lock);
>>>>
>>>>         ret = ttm_bo_evict(bo, ctx);
>>>>         if (locked) {
>>>>                 ttm_bo_unreserve(bo); <-------- here
>>>>         } else {
>>>>                 spin_lock(&glob->lru_lock);
>>>>                 ttm_bo_add_to_lru(bo);
>>>>                 spin_unlock(&glob->lru_lock);
>>>>         }
>>>>
>>>>         kref_put(&bo->list_kref, ttm_bo_release_list);
>>>
>>> The effect is that in your example process C can not only beat 
>>> process B once, but many many times because we run into a ping/pong 
>>> situation where B evicts resources while C moves them back in.
>> For ping/pong case, I want to disable busy placement for allocation 
>> period, only enable it for cs bo validation.
>>
>>>
>>> For a while now I'm thinking about dropping those reservations only 
>>> after the original allocation succeeded.
>>>
>>> The effect would be that process C can still beat process B 
>>> initially, but sooner or process B would evict some resources from 
>>> process C as well and then it can succeed with its allocation.
>> If it is from process C cs validation, process B still need evict the 
>> resource only after process C command submission completion.
>>
>>>
>>> The problem is for this approach to work we need to core change to 
>>> the ww_mutexes to be able to handle this efficiently.
>> Yes, ww_mutex doesn't support this net lock, which easily deadlock 
>> without ticket and class.
>>
>> So I think preventing validation on same place is a simpler way:
>> process B bo's place is fpfn~lpfn, it will only try to evict LRU BOs 
>> in that range, while eviction, we just prevent those validation to 
>> this range(fpfn~lpfn), if out of this range, the 
>> allocation/validation still can be go on.
>>
>> Any negative?
>>
>> Regards,
>> David Zhou
>>>
>>> Regards,
>>> Christian.
>>>
>>> Am 26.01.2018 um 14:59 schrieb Christian König:
>>>> I know, but this has the same effect. You prevent concurrent 
>>>> allocation from happening.
>>>>
>>>> What we could do is to pipeline reusing of deleted memory as well, 
>>>> this makes it less likely to cause the problem you are seeing 
>>>> because the evicting processes doesn't need to block for deleted 
>>>> BOs any more.
>>>>
>>>> But that other processes can grab memory during eviction is 
>>>> intentional. Otherwise greedy processes would completely dominate 
>>>> command submission.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 26.01.2018 um 14:50 schrieb Zhou, David(ChunMing):
>>>>> I don't want to prevent all, my new approach is to prevent the 
>>>>> later allocation is trying and ahead of front to get the memory 
>>>>> space that the front made from eviction.
>>>>>
>>>>>
>>>>> 发自坚果 Pro
>>>>>
>>>>> Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 
>>>>> 下午9:24写道：
>>>>>
>>>>> Yes, exactly that's the problem.
>>>>>
>>>>> See when you want to prevent a process B from allocating the 
>>>>> memory process A has evicted, you need to prevent all concurrent 
>>>>> allocation.
>>>>>
>>>>> And we don't do that because it causes a major performance drop.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>> Am 26.01.2018 um 14:21 schrieb Zhou, David(ChunMing):
>>>>>> You patch will prevent concurrent allocation, and will result in 
>>>>>> allocation performance drop much.
>>>>>>
>>>>>> 发自坚果 Pro
>>>>>>
>>>>>> Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 
>>>>>> 下午9:04写道：
>>>>>>
>>>>>> Attached is what you actually want to do cleanly implemented. But 
>>>>>> as I said this is a NO-GO.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>> Am 26.01.2018 um 13:43 schrieb Christian König:
>>>>>>>> After my investigation, this issue should be detect of TTM 
>>>>>>>> design self, which breaks scheduling balance.
>>>>>>> Yeah, but again. This is indented design we can't change easily.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>> Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
>>>>>>>> I am off work, so reply mail by phone, the format could not be 
>>>>>>>> text.
>>>>>>>>
>>>>>>>> back to topic itself:
>>>>>>>> the problem indeed happen on amdgpu driver, someone reports me 
>>>>>>>> that application runs with two instances, the performance are 
>>>>>>>> different.
>>>>>>>> I also reproduced the issue with unit test(bo_eviction_test). 
>>>>>>>> They always think our scheduler isn't working as expected.
>>>>>>>>
>>>>>>>> After my investigation, this issue should be detect of TTM 
>>>>>>>> design self, which breaks scheduling balance.
>>>>>>>>
>>>>>>>> Further, if we run containers for our gpu, container A could 
>>>>>>>> run high score, container B runs low score with same benchmark.
>>>>>>>>
>>>>>>>> So this is bug that we need fix.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> David Zhou
>>>>>>>>
>>>>>>>> 发自坚果 Pro
>>>>>>>>
>>>>>>>> Christian K鰊ig <ckoenig.leichtzumerken@gmail.com> 于 2018年1月26日 
>>>>>>>> 下午6:31写道：
>>>>>>>>
>>>>>>>> Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
>>>>>>>> > there is a scheduling balance issue about get node like:
>>>>>>>> > a. process A allocates full memory and use it for submission.
>>>>>>>> > b. process B tries to allocates memory, will wait for process 
>>>>>>>> A BO idle in eviction.
>>>>>>>> > c. process A completes the job, process B eviction will put 
>>>>>>>> process A BO node,
>>>>>>>> > but in the meantime, process C is comming to allocate BO, 
>>>>>>>> whill directly get node successfully, and do submission,
>>>>>>>> > process B will again wait for process C BO idle.
>>>>>>>> > d. repeat the above setps, process B could be delayed much more.
>>>>>>>> >
>>>>>>>> > later allocation must not be ahead of front in same place.
>>>>>>>>
>>>>>>>> Again NAK to the whole approach.
>>>>>>>>
>>>>>>>> At least with amdgpu the problem you described above never occurs
>>>>>>>> because evictions are pipelined operations. We could only block 
>>>>>>>> for
>>>>>>>> deleted regions to become free.
>>>>>>>>
>>>>>>>> But independent of that incoming memory requests while we make 
>>>>>>>> room for
>>>>>>>> eviction are intended to be served first.
>>>>>>>>
>>>>>>>> Changing that is certainly a no-go cause that would favor 
>>>>>>>> memory hungry
>>>>>>>> applications over small clients.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>> >
>>>>>>>> > Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
>>>>>>>> > Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>>>>>>>> > ---
>>>>>>>> >   drivers/gpu/drm/ttm/ttm_bo.c    | 69 
>>>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>>>> >   include/drm/ttm/ttm_bo_api.h    | 7 +++++
>>>>>>>> >   include/drm/ttm/ttm_bo_driver.h | 7 +++++
>>>>>>>> >   3 files changed, 80 insertions(+), 3 deletions(-)
>>>>>>>> >
>>>>>>>> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
>>>>>>>> b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>>>> > index d33a6bb742a1..558ec2cf465d 100644
>>>>>>>> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>>>> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>>>> > @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct 
>>>>>>>> ttm_buffer_object *bo,
>>>>>>>> >        return 0;
>>>>>>>> >   }
>>>>>>>> >
>>>>>>>> > +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
>>>>>>>> > +                             struct ttm_buffer_object *bo,
>>>>>>>> > +                             const struct ttm_place *place)
>>>>>>>> > +{
>>>>>>>> > +     waiter->tbo = bo;
>>>>>>>> > +     memcpy((void *)&waiter->place, (void *)place, 
>>>>>>>> sizeof(*place));
>>>>>>>> > + INIT_LIST_HEAD(&waiter->list);
>>>>>>>> > +}
>>>>>>>> > +
>>>>>>>> > +static void ttm_man_add_waiter(struct ttm_mem_type_manager *man,
>>>>>>>> > +                            struct ttm_bo_waiter *waiter)
>>>>>>>> > +{
>>>>>>>> > +     if (!waiter)
>>>>>>>> > +             return;
>>>>>>>> > + spin_lock(&man->wait_lock);
>>>>>>>> > + list_add_tail(&waiter->list, &man->waiter_list);
>>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>>> > +}
>>>>>>>> > +
>>>>>>>> > +static void ttm_man_del_waiter(struct ttm_mem_type_manager *man,
>>>>>>>> > +                            struct ttm_bo_waiter *waiter)
>>>>>>>> > +{
>>>>>>>> > +     if (!waiter)
>>>>>>>> > +             return;
>>>>>>>> > + spin_lock(&man->wait_lock);
>>>>>>>> > +     if (!list_empty(&waiter->list))
>>>>>>>> > + list_del(&waiter->list);
>>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>>> > +     kfree(waiter);
>>>>>>>> > +}
>>>>>>>> > +
>>>>>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>>>>>> > +                  struct ttm_buffer_object *bo,
>>>>>>>> > +                  const struct ttm_place *place)
>>>>>>>> > +{
>>>>>>>> > +     struct ttm_bo_waiter *waiter, *tmp;
>>>>>>>> > +
>>>>>>>> > + spin_lock(&man->wait_lock);
>>>>>>>> > + list_for_each_entry_safe(waiter, tmp, &man->waiter_list, 
>>>>>>>> list) {
>>>>>>>> > +             if ((bo != waiter->tbo) &&
>>>>>>>> > +                 ((place->fpfn >= waiter->place.fpfn &&
>>>>>>>> > +                   place->fpfn <= waiter->place.lpfn) ||
>>>>>>>> > +                  (place->lpfn <= waiter->place.lpfn && 
>>>>>>>> place->lpfn >=
>>>>>>>> > + waiter->place.fpfn)))
>>>>>>>> > +                 goto later_bo;
>>>>>>>> > +     }
>>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>>> > +     return true;
>>>>>>>> > +later_bo:
>>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>>> > +     return false;
>>>>>>>> > +}
>>>>>>>> >   /**
>>>>>>>> >    * Repeatedly evict memory from the LRU for @mem_type until 
>>>>>>>> we create enough
>>>>>>>> >    * space, or we've evicted everything and there isn't 
>>>>>>>> enough space.
>>>>>>>> > @@ -853,17 +905,26 @@ static int 
>>>>>>>> ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>>>>>>>> >   {
>>>>>>>> >        struct ttm_bo_device *bdev = bo->bdev;
>>>>>>>> >        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>>>>>>> > +     struct ttm_bo_waiter waiter;
>>>>>>>> >        int ret;
>>>>>>>> >
>>>>>>>> > + ttm_man_init_waiter(&waiter, bo, place);
>>>>>>>> > +     ttm_man_add_waiter(man, &waiter);
>>>>>>>> >        do {
>>>>>>>> >                ret = (*man->func->get_node)(man, bo, place, mem);
>>>>>>>> > -             if (unlikely(ret != 0))
>>>>>>>> > +             if (unlikely(ret != 0)) {
>>>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>>>> >                        return ret;
>>>>>>>> > -             if (mem->mm_node)
>>>>>>>> > +             }
>>>>>>>> > +             if (mem->mm_node) {
>>>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>>>> >                        break;
>>>>>>>> > +             }
>>>>>>>> >                ret = ttm_mem_evict_first(bdev, mem_type, 
>>>>>>>> place, ctx);
>>>>>>>> > -             if (unlikely(ret != 0))
>>>>>>>> > +             if (unlikely(ret != 0)) {
>>>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>>>> >                        return ret;
>>>>>>>> > +             }
>>>>>>>> >        } while (1);
>>>>>>>> >        mem->mem_type = mem_type;
>>>>>>>> >        return ttm_bo_add_move_fence(bo, man, mem);
>>>>>>>> > @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct ttm_bo_device 
>>>>>>>> *bdev, unsigned type,
>>>>>>>> >        man->use_io_reserve_lru = false;
>>>>>>>> > mutex_init(&man->io_reserve_mutex);
>>>>>>>> > spin_lock_init(&man->move_lock);
>>>>>>>> > + spin_lock_init(&man->wait_lock);
>>>>>>>> > + INIT_LIST_HEAD(&man->waiter_list);
>>>>>>>> > INIT_LIST_HEAD(&man->io_reserve_lru);
>>>>>>>> >
>>>>>>>> >        ret = bdev->driver->init_mem_type(bdev, type, man);
>>>>>>>> > diff --git a/include/drm/ttm/ttm_bo_api.h 
>>>>>>>> b/include/drm/ttm/ttm_bo_api.h
>>>>>>>> > index 2cd025c2abe7..0fce4dbd02e7 100644
>>>>>>>> > --- a/include/drm/ttm/ttm_bo_api.h
>>>>>>>> > +++ b/include/drm/ttm/ttm_bo_api.h
>>>>>>>> > @@ -40,6 +40,7 @@
>>>>>>>> >   #include <linux/mm.h>
>>>>>>>> >   #include <linux/bitmap.h>
>>>>>>>> >   #include <linux/reservation.h>
>>>>>>>> > +#include <drm/ttm/ttm_placement.h>
>>>>>>>> >
>>>>>>>> >   struct ttm_bo_device;
>>>>>>>> >
>>>>>>>> > @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>>>>>>>> >        struct mutex wu_mutex;
>>>>>>>> >   };
>>>>>>>> >
>>>>>>>> > +struct ttm_bo_waiter {
>>>>>>>> > +     struct ttm_buffer_object *tbo;
>>>>>>>> > +     struct ttm_place place;
>>>>>>>> > +     struct list_head list;
>>>>>>>> > +};
>>>>>>>> > +
>>>>>>>> >   /**
>>>>>>>> >    * struct ttm_bo_kmap_obj
>>>>>>>> >    *
>>>>>>>> > diff --git a/include/drm/ttm/ttm_bo_driver.h 
>>>>>>>> b/include/drm/ttm/ttm_bo_driver.h
>>>>>>>> > index 9b417eb2df20..dc6b8b4c9e06 100644
>>>>>>>> > --- a/include/drm/ttm/ttm_bo_driver.h
>>>>>>>> > +++ b/include/drm/ttm/ttm_bo_driver.h
>>>>>>>> > @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>>>>>>>> >        bool io_reserve_fastpath;
>>>>>>>> >        spinlock_t move_lock;
>>>>>>>> >
>>>>>>>> > +     /* waiters in list */
>>>>>>>> > +     spinlock_t wait_lock;
>>>>>>>> > +     struct list_head waiter_list;
>>>>>>>> > +
>>>>>>>> >        /*
>>>>>>>> >         * Protected by @io_reserve_mutex:
>>>>>>>> >         */
>>>>>>>> > @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct 
>>>>>>>> ttm_buffer_object *bo,
>>>>>>>> >                     struct ttm_mem_reg *mem,
>>>>>>>> >                     struct ttm_operation_ctx *ctx);
>>>>>>>> >
>>>>>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>>>>>> > +                  struct ttm_buffer_object *bo,
>>>>>>>> > +                  const struct ttm_place *place);
>>>>>>>> >   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct 
>>>>>>>> ttm_mem_reg *mem);
>>>>>>>> >   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>>>>>>>> >                           struct ttm_mem_reg *mem);
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> amd-gfx mailing list
>>>>>>>> amd-gfx@lists.freedesktop.org
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> dri-devel mailing list
>>>>>> dri-devel@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
>>>
>>
>


[-- Attachment #1.2: Type: text/html, Size: 36435 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 1/2] [WIP]drm/ttm: add waiter list to prevent allocation not in order

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 17699 bytes --]

Am 05.02.2018 um 09:22 schrieb Chunming Zhou:
> On 2018年01月31日 18:54, Christian König wrote:
>>> So I think preventing validation on same place is a simpler way:
>>> process B bo's place is fpfn~lpfn, it will only try to evict LRU BOs 
>>> in that range, while eviction, we just prevent those validation to 
>>> this range(fpfn~lpfn), if out of this range, the 
>>> allocation/validation still can be go on.
>>>
>>> Any negative?
>> That won't work either. The most common use of fpfn~lpfn range is to 
>> limit a BO to visible VRAM, the other use cases are to fullfil 
>> hardware limitations.
>>
>> So blocking this would result in blocking all normal GTT and VRAM 
>> allocations, adding a mutex to validate would have the same effect.
> No, different effect, mutex will make the every allocation serialized. 
> My approach only effects busy case, that is said, only when space is 
> used up, the allocation is serialized in that range, otherwise still 
> in parallel.

That would still not allow for concurrent evictions, not as worse as 
completely blocking concurrent validation but still not a good idea as 
far as I can see.

Going to give it a try today to use the ww_mutex owner to detect 
eviction of already reserved BOs.

Maybe that can be used instead,
Christian.

>
> Regards,
> David Zhou
>
>
>>
>> Regards,
>> Christian.
>>
>> Am 31.01.2018 um 11:30 schrieb Chunming Zhou:
>>>
>>>
>>>
>>> On 2018年01月26日 22:35, Christian König wrote:
>>>> I just realized that a change I'm thinking about for a while would 
>>>> solve your problem as well, but keep concurrent allocation possible.
>>>>
>>>> See ttm_mem_evict_first() unlocks the BO after evicting it:
>>>>>         ttm_bo_del_from_lru(bo);
>>>>>         spin_unlock(&glob->lru_lock);
>>>>>
>>>>>         ret = ttm_bo_evict(bo, ctx);
>>>>>         if (locked) {
>>>>>                 ttm_bo_unreserve(bo); <-------- here
>>>>>         } else {
>>>>>                 spin_lock(&glob->lru_lock);
>>>>>                 ttm_bo_add_to_lru(bo);
>>>>>                 spin_unlock(&glob->lru_lock);
>>>>>         }
>>>>>
>>>>>         kref_put(&bo->list_kref, ttm_bo_release_list);
>>>>
>>>> The effect is that in your example process C can not only beat 
>>>> process B once, but many many times because we run into a ping/pong 
>>>> situation where B evicts resources while C moves them back in.
>>> For ping/pong case, I want to disable busy placement for allocation 
>>> period, only enable it for cs bo validation.
>>>
>>>>
>>>> For a while now I'm thinking about dropping those reservations only 
>>>> after the original allocation succeeded.
>>>>
>>>> The effect would be that process C can still beat process B 
>>>> initially, but sooner or process B would evict some resources from 
>>>> process C as well and then it can succeed with its allocation.
>>> If it is from process C cs validation, process B still need evict 
>>> the resource only after process C command submission completion.
>>>
>>>>
>>>> The problem is for this approach to work we need to core change to 
>>>> the ww_mutexes to be able to handle this efficiently.
>>> Yes, ww_mutex doesn't support this net lock, which easily deadlock 
>>> without ticket and class.
>>>
>>> So I think preventing validation on same place is a simpler way:
>>> process B bo's place is fpfn~lpfn, it will only try to evict LRU BOs 
>>> in that range, while eviction, we just prevent those validation to 
>>> this range(fpfn~lpfn), if out of this range, the 
>>> allocation/validation still can be go on.
>>>
>>> Any negative?
>>>
>>> Regards,
>>> David Zhou
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 26.01.2018 um 14:59 schrieb Christian König:
>>>>> I know, but this has the same effect. You prevent concurrent 
>>>>> allocation from happening.
>>>>>
>>>>> What we could do is to pipeline reusing of deleted memory as well, 
>>>>> this makes it less likely to cause the problem you are seeing 
>>>>> because the evicting processes doesn't need to block for deleted 
>>>>> BOs any more.
>>>>>
>>>>> But that other processes can grab memory during eviction is 
>>>>> intentional. Otherwise greedy processes would completely dominate 
>>>>> command submission.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>> Am 26.01.2018 um 14:50 schrieb Zhou, David(ChunMing):
>>>>>> I don't want to prevent all, my new approach is to prevent the 
>>>>>> later allocation is trying and ahead of front to get the memory 
>>>>>> space that the front made from eviction.
>>>>>>
>>>>>>
>>>>>> 发自坚果 Pro
>>>>>>
>>>>>> Christian K鰊ig <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 于 2018年1月26日 
>>>>>> 下午9:24写道：
>>>>>>
>>>>>> Yes, exactly that's the problem.
>>>>>>
>>>>>> See when you want to prevent a process B from allocating the 
>>>>>> memory process A has evicted, you need to prevent all concurrent 
>>>>>> allocation.
>>>>>>
>>>>>> And we don't do that because it causes a major performance drop.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>> Am 26.01.2018 um 14:21 schrieb Zhou, David(ChunMing):
>>>>>>> You patch will prevent concurrent allocation, and will result in 
>>>>>>> allocation performance drop much.
>>>>>>>
>>>>>>> 发自坚果 Pro
>>>>>>>
>>>>>>> Christian K鰊ig <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 于 2018年1月26日 
>>>>>>> 下午9:04写道：
>>>>>>>
>>>>>>> Attached is what you actually want to do cleanly implemented. 
>>>>>>> But as I said this is a NO-GO.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>> Am 26.01.2018 um 13:43 schrieb Christian König:
>>>>>>>>> After my investigation, this issue should be detect of TTM 
>>>>>>>>> design self, which breaks scheduling balance.
>>>>>>>> Yeah, but again. This is indented design we can't change easily.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>> Am 26.01.2018 um 13:36 schrieb Zhou, David(ChunMing):
>>>>>>>>> I am off work, so reply mail by phone, the format could not be 
>>>>>>>>> text.
>>>>>>>>>
>>>>>>>>> back to topic itself:
>>>>>>>>> the problem indeed happen on amdgpu driver, someone reports me 
>>>>>>>>> that application runs with two instances, the performance are 
>>>>>>>>> different.
>>>>>>>>> I also reproduced the issue with unit test(bo_eviction_test). 
>>>>>>>>> They always think our scheduler isn't working as expected.
>>>>>>>>>
>>>>>>>>> After my investigation, this issue should be detect of TTM 
>>>>>>>>> design self, which breaks scheduling balance.
>>>>>>>>>
>>>>>>>>> Further, if we run containers for our gpu, container A could 
>>>>>>>>> run high score, container B runs low score with same benchmark.
>>>>>>>>>
>>>>>>>>> So this is bug that we need fix.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> David Zhou
>>>>>>>>>
>>>>>>>>> 发自坚果 Pro
>>>>>>>>>
>>>>>>>>> Christian K鰊ig <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 于 
>>>>>>>>> 2018年1月26日 下午6:31写道：
>>>>>>>>>
>>>>>>>>> Am 26.01.2018 um 11:22 schrieb Chunming Zhou:
>>>>>>>>> > there is a scheduling balance issue about get node like:
>>>>>>>>> > a. process A allocates full memory and use it for submission.
>>>>>>>>> > b. process B tries to allocates memory, will wait for 
>>>>>>>>> process A BO idle in eviction.
>>>>>>>>> > c. process A completes the job, process B eviction will put 
>>>>>>>>> process A BO node,
>>>>>>>>> > but in the meantime, process C is comming to allocate BO, 
>>>>>>>>> whill directly get node successfully, and do submission,
>>>>>>>>> > process B will again wait for process C BO idle.
>>>>>>>>> > d. repeat the above setps, process B could be delayed much more.
>>>>>>>>> >
>>>>>>>>> > later allocation must not be ahead of front in same place.
>>>>>>>>>
>>>>>>>>> Again NAK to the whole approach.
>>>>>>>>>
>>>>>>>>> At least with amdgpu the problem you described above never occurs
>>>>>>>>> because evictions are pipelined operations. We could only 
>>>>>>>>> block for
>>>>>>>>> deleted regions to become free.
>>>>>>>>>
>>>>>>>>> But independent of that incoming memory requests while we make 
>>>>>>>>> room for
>>>>>>>>> eviction are intended to be served first.
>>>>>>>>>
>>>>>>>>> Changing that is certainly a no-go cause that would favor 
>>>>>>>>> memory hungry
>>>>>>>>> applications over small clients.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Christian.
>>>>>>>>>
>>>>>>>>> >
>>>>>>>>> > Change-Id: I3daa892e50f82226c552cc008a29e55894a98f18
>>>>>>>>> > Signed-off-by: Chunming Zhou <david1.zhou-5C7GfCeVMHo@public.gmane.org>
>>>>>>>>> > ---
>>>>>>>>> >   drivers/gpu/drm/ttm/ttm_bo.c    | 69 
>>>>>>>>> +++++++++++++++++++++++++++++++++++++++--
>>>>>>>>> >   include/drm/ttm/ttm_bo_api.h |  7 +++++
>>>>>>>>> >   include/drm/ttm/ttm_bo_driver.h |  7 +++++
>>>>>>>>> >   3 files changed, 80 insertions(+), 3 deletions(-)
>>>>>>>>> >
>>>>>>>>> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
>>>>>>>>> b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>>>>> > index d33a6bb742a1..558ec2cf465d 100644
>>>>>>>>> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>>>>> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>>>>>> > @@ -841,6 +841,58 @@ static int ttm_bo_add_move_fence(struct 
>>>>>>>>> ttm_buffer_object *bo,
>>>>>>>>> >        return 0;
>>>>>>>>> >   }
>>>>>>>>> >
>>>>>>>>> > +static void ttm_man_init_waiter(struct ttm_bo_waiter *waiter,
>>>>>>>>> > + struct ttm_buffer_object *bo,
>>>>>>>>> > +                             const struct ttm_place *place)
>>>>>>>>> > +{
>>>>>>>>> > +     waiter->tbo = bo;
>>>>>>>>> > +     memcpy((void *)&waiter->place, (void *)place, 
>>>>>>>>> sizeof(*place));
>>>>>>>>> > + INIT_LIST_HEAD(&waiter->list);
>>>>>>>>> > +}
>>>>>>>>> > +
>>>>>>>>> > +static void ttm_man_add_waiter(struct ttm_mem_type_manager 
>>>>>>>>> *man,
>>>>>>>>> > +                            struct ttm_bo_waiter *waiter)
>>>>>>>>> > +{
>>>>>>>>> > +     if (!waiter)
>>>>>>>>> > +             return;
>>>>>>>>> > + spin_lock(&man->wait_lock);
>>>>>>>>> > + list_add_tail(&waiter->list, &man->waiter_list);
>>>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>>>> > +}
>>>>>>>>> > +
>>>>>>>>> > +static void ttm_man_del_waiter(struct ttm_mem_type_manager 
>>>>>>>>> *man,
>>>>>>>>> > +                            struct ttm_bo_waiter *waiter)
>>>>>>>>> > +{
>>>>>>>>> > +     if (!waiter)
>>>>>>>>> > +             return;
>>>>>>>>> > + spin_lock(&man->wait_lock);
>>>>>>>>> > +     if (!list_empty(&waiter->list))
>>>>>>>>> > + list_del(&waiter->list);
>>>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>>>> > +     kfree(waiter);
>>>>>>>>> > +}
>>>>>>>>> > +
>>>>>>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>>>>>>> > +                  struct ttm_buffer_object *bo,
>>>>>>>>> > +                  const struct ttm_place *place)
>>>>>>>>> > +{
>>>>>>>>> > +     struct ttm_bo_waiter *waiter, *tmp;
>>>>>>>>> > +
>>>>>>>>> > + spin_lock(&man->wait_lock);
>>>>>>>>> > + list_for_each_entry_safe(waiter, tmp, &man->waiter_list, 
>>>>>>>>> list) {
>>>>>>>>> > +             if ((bo != waiter->tbo) &&
>>>>>>>>> > +                 ((place->fpfn >= waiter->place.fpfn &&
>>>>>>>>> > +                   place->fpfn <= waiter->place.lpfn) ||
>>>>>>>>> > +                  (place->lpfn <= waiter->place.lpfn && 
>>>>>>>>> place->lpfn >=
>>>>>>>>> > + waiter->place.fpfn)))
>>>>>>>>> > +                 goto later_bo;
>>>>>>>>> > +     }
>>>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>>>> > +     return true;
>>>>>>>>> > +later_bo:
>>>>>>>>> > + spin_unlock(&man->wait_lock);
>>>>>>>>> > +     return false;
>>>>>>>>> > +}
>>>>>>>>> >   /**
>>>>>>>>> >    * Repeatedly evict memory from the LRU for @mem_type 
>>>>>>>>> until we create enough
>>>>>>>>> >    * space, or we've evicted everything and there isn't 
>>>>>>>>> enough space.
>>>>>>>>> > @@ -853,17 +905,26 @@ static int 
>>>>>>>>> ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>>>>>>>>> >   {
>>>>>>>>> >        struct ttm_bo_device *bdev = bo->bdev;
>>>>>>>>> >        struct ttm_mem_type_manager *man = &bdev->man[mem_type];
>>>>>>>>> > +     struct ttm_bo_waiter waiter;
>>>>>>>>> >        int ret;
>>>>>>>>> >
>>>>>>>>> > + ttm_man_init_waiter(&waiter, bo, place);
>>>>>>>>> > +     ttm_man_add_waiter(man, &waiter);
>>>>>>>>> >        do {
>>>>>>>>> >                ret = (*man->func->get_node)(man, bo, place, 
>>>>>>>>> mem);
>>>>>>>>> > -             if (unlikely(ret != 0))
>>>>>>>>> > +             if (unlikely(ret != 0)) {
>>>>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>>>>> >                        return ret;
>>>>>>>>> > -             if (mem->mm_node)
>>>>>>>>> > +             }
>>>>>>>>> > +             if (mem->mm_node) {
>>>>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>>>>> >                        break;
>>>>>>>>> > +             }
>>>>>>>>> >                ret = ttm_mem_evict_first(bdev, mem_type, 
>>>>>>>>> place, ctx);
>>>>>>>>> > -             if (unlikely(ret != 0))
>>>>>>>>> > +             if (unlikely(ret != 0)) {
>>>>>>>>> > + ttm_man_del_waiter(man, &waiter);
>>>>>>>>> >                        return ret;
>>>>>>>>> > +             }
>>>>>>>>> >        } while (1);
>>>>>>>>> >        mem->mem_type = mem_type;
>>>>>>>>> >        return ttm_bo_add_move_fence(bo, man, mem);
>>>>>>>>> > @@ -1450,6 +1511,8 @@ int ttm_bo_init_mm(struct 
>>>>>>>>> ttm_bo_device *bdev, unsigned type,
>>>>>>>>> >        man->use_io_reserve_lru = false;
>>>>>>>>> > mutex_init(&man->io_reserve_mutex);
>>>>>>>>> > spin_lock_init(&man->move_lock);
>>>>>>>>> > + spin_lock_init(&man->wait_lock);
>>>>>>>>> > + INIT_LIST_HEAD(&man->waiter_list);
>>>>>>>>> > INIT_LIST_HEAD(&man->io_reserve_lru);
>>>>>>>>> >
>>>>>>>>> >        ret = bdev->driver->init_mem_type(bdev, type, man);
>>>>>>>>> > diff --git a/include/drm/ttm/ttm_bo_api.h 
>>>>>>>>> b/include/drm/ttm/ttm_bo_api.h
>>>>>>>>> > index 2cd025c2abe7..0fce4dbd02e7 100644
>>>>>>>>> > --- a/include/drm/ttm/ttm_bo_api.h
>>>>>>>>> > +++ b/include/drm/ttm/ttm_bo_api.h
>>>>>>>>> > @@ -40,6 +40,7 @@
>>>>>>>>> >   #include <linux/mm.h>
>>>>>>>>> >   #include <linux/bitmap.h>
>>>>>>>>> >   #include <linux/reservation.h>
>>>>>>>>> > +#include <drm/ttm/ttm_placement.h>
>>>>>>>>> >
>>>>>>>>> >   struct ttm_bo_device;
>>>>>>>>> >
>>>>>>>>> > @@ -232,6 +233,12 @@ struct ttm_buffer_object {
>>>>>>>>> >        struct mutex wu_mutex;
>>>>>>>>> >   };
>>>>>>>>> >
>>>>>>>>> > +struct ttm_bo_waiter {
>>>>>>>>> > +     struct ttm_buffer_object *tbo;
>>>>>>>>> > +     struct ttm_place place;
>>>>>>>>> > +     struct list_head list;
>>>>>>>>> > +};
>>>>>>>>> > +
>>>>>>>>> >   /**
>>>>>>>>> >    * struct ttm_bo_kmap_obj
>>>>>>>>> >    *
>>>>>>>>> > diff --git a/include/drm/ttm/ttm_bo_driver.h 
>>>>>>>>> b/include/drm/ttm/ttm_bo_driver.h
>>>>>>>>> > index 9b417eb2df20..dc6b8b4c9e06 100644
>>>>>>>>> > --- a/include/drm/ttm/ttm_bo_driver.h
>>>>>>>>> > +++ b/include/drm/ttm/ttm_bo_driver.h
>>>>>>>>> > @@ -293,6 +293,10 @@ struct ttm_mem_type_manager {
>>>>>>>>> >        bool io_reserve_fastpath;
>>>>>>>>> >        spinlock_t move_lock;
>>>>>>>>> >
>>>>>>>>> > +     /* waiters in list */
>>>>>>>>> > +     spinlock_t wait_lock;
>>>>>>>>> > +     struct list_head waiter_list;
>>>>>>>>> > +
>>>>>>>>> >        /*
>>>>>>>>> >         * Protected by @io_reserve_mutex:
>>>>>>>>> >         */
>>>>>>>>> > @@ -748,6 +752,9 @@ int ttm_bo_mem_space(struct 
>>>>>>>>> ttm_buffer_object *bo,
>>>>>>>>> >                     struct ttm_mem_reg *mem,
>>>>>>>>> >                     struct ttm_operation_ctx *ctx);
>>>>>>>>> >
>>>>>>>>> > +int ttm_man_check_bo(struct ttm_mem_type_manager *man,
>>>>>>>>> > +                  struct ttm_buffer_object *bo,
>>>>>>>>> > +                  const struct ttm_place *place);
>>>>>>>>> >   void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct 
>>>>>>>>> ttm_mem_reg *mem);
>>>>>>>>> >   void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
>>>>>>>>> >                           struct ttm_mem_reg *mem);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> amd-gfx mailing list
>>>>>>>>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> dri-devel mailing list
>>>>>>> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>
>>>
>>
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 39976 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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