Re: [PATCH v2 4/5] drm/amdgpu: use bulk moves for efficient VM LRU handling (v2)

[Huang Rui <ray.huang-5C7GfCeVMHo@public.gmane.org>]

On Fri, Aug 10, 2018 at 10:18:24PM +0800, Koenig, Christian wrote:
> Am 10.08.2018 um 13:55 schrieb Huang Rui:
> > I continue to work for bulk moving that based on the proposal by Christian.
> >
> > Background:
> > amdgpu driver will move all PD/PT and PerVM BOs into idle list. Then move all of
> > them on the end of LRU list one by one. Thus, that cause so many BOs moved to
> > the end of the LRU, and impact performance seriously.
> >
> > Then Christian provided a workaround to not move PD/PT BOs on LRU with below
> > patch:
> > "drm/amdgpu: band aid validating VM PTs"
> > Commit 0bbf32026cf5ba41e9922b30e26e1bed1ecd38ae
> >
> > However, the final solution should bulk move all PD/PT and PerVM BOs on the LRU
> > instead of one by one.
> >
> > Whenever amdgpu_vm_validate_pt_bos() is called and we have BOs which need to be
> > validated we move all BOs together to the end of the LRU without dropping the
> > lock for the LRU.
> >
> > While doing so we note the beginning and end of this block in the LRU list.
> >
> > Now when amdgpu_vm_validate_pt_bos() is called and we don't have anything to do,
> > we don't move every BO one by one, but instead cut the LRU list into pieces so
> > that we bulk move everything to the end in just one operation.
> >
> > Test data:
> > +--------------+-----------------+-----------+---------------------------------------+
> > |              |The Talos        |Clpeak(OCL)|BusSpeedReadback(OCL)                  |
> > |              |Principle(Vulkan)|           |                                       |
> > +------------------------------------------------------------------------------------+
> > |              |                 |           |0.319 ms(1k) 0.314 ms(2K) 0.308 ms(4K) |
> > | Original     |  147.7 FPS      |  76.86 us |0.307 ms(8K) 0.310 ms(16K)             |
> > +------------------------------------------------------------------------------------+
> > | Orignial + WA|                 |           |0.254 ms(1K) 0.241 ms(2K)              |
> > |(don't move   |  162.1 FPS      |  42.15 us |0.230 ms(4K) 0.223 ms(8K) 0.204 ms(16K)|
> > |PT BOs on LRU)|                 |           |                                       |
> > +------------------------------------------------------------------------------------+
> > | Bulk move    |  163.1 FPS      |  40.52 us |0.244 ms(1K) 0.252 ms(2K) 0.213 ms(4K) |
> > |              |                 |           |0.214 ms(8K) 0.225 ms(16K)             |
> > +--------------+-----------------+-----------+---------------------------------------+
> >
> > After test them with above three benchmarks include vulkan and opencl. We can
> > see the visible improvement than original, and even better than original with
> > workaround.
> >
> > v2: move all BOs include idle, relocated, and moved list to the end of LRU and
> > put them together.
> >
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Huang Rui <ray.huang@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 75 ++++++++++++++++++++++++++--------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  4 ++
> >   2 files changed, 63 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index 9c84770..81fb1ac 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -268,6 +268,55 @@ void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
> >   }
> >   
> >   /**
> > + * amdgpu_vm_move_to_lru_tail - move one list of BOs to end of LRU
> > + *
> > + * @adev: amdgpu device pointer
> > + * @vm: vm providing the BOs
> > + * @list: the list that stored BOs
> > + *
> > + * Move one list of BOs to the end of LRU and update the positions.
> > + */
> > +static void
> > +amdgpu_vm_move_to_lru_tail_by_list(struct amdgpu_device *adev,
> > +				   struct amdgpu_vm *vm, struct list_head *list)
> 
> Am I blind or is adev not actually used in the function?
> 

No, what you see is definitely real. ;-)
That's my fault to add an unused adev int the function. Will remove it in
next version.

> > +{
> > +	struct amdgpu_vm_bo_base *bo_base, *tmp;
> > +
> > +	list_for_each_entry_safe(bo_base, tmp, list, vm_status) {
> 
> No need for the _safe variant as far as I can see.
> 

Yes, we won't move vm->vm_status to another list in the loop.

Thanks,
Ray

> Apart from that this looks good to me.
> 
> Christian.
> 
> > +		struct amdgpu_bo *bo = bo_base->bo;
> > +
> > +		if (!bo->parent)
> > +			continue;
> > +
> > +		ttm_bo_move_to_lru_tail(&bo->tbo, &vm->lru_bulk_move);
> > +		if (bo->shadow)
> > +			ttm_bo_move_to_lru_tail(&bo->shadow->tbo,
> > +						&vm->lru_bulk_move);
> > +	}
> > +}
> > +
> > +/**
> > + * amdgpu_vm_move_to_lru_tail - move all BOs to the end of LRU
> > + *
> > + * @adev: amdgpu device pointer
> > + * @vm: vm providing the BOs
> > + *
> > + * Move all BOs to the end of LRU and remember their positions to put them
> > + * together.
> > + */
> > +static void
> > +amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> > +{
> > +	struct ttm_bo_global *glob = adev->mman.bdev.glob;
> > +
> > +	spin_lock(&glob->lru_lock);
> > +	amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->idle);
> > +	amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->relocated);
> > +	amdgpu_vm_move_to_lru_tail_by_list(adev, vm, &vm->moved);
> > +	spin_unlock(&glob->lru_lock);
> > +}
> > +
> > +/**
> >    * amdgpu_vm_validate_pt_bos - validate the page table BOs
> >    *
> >    * @adev: amdgpu device pointer
> > @@ -286,6 +335,7 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> >   {
> >   	struct ttm_bo_global *glob = adev->mman.bdev.glob;
> >   	struct amdgpu_vm_bo_base *bo_base, *tmp;
> > +	bool validated = false;
> >   	int r = 0;
> >   
> >   	list_for_each_entry_safe(bo_base, tmp, &vm->evicted, vm_status) {
> > @@ -295,14 +345,9 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> >   			r = validate(param, bo);
> >   			if (r)
> >   				break;
> > -
> > -			spin_lock(&glob->lru_lock);
> > -			ttm_bo_move_to_lru_tail(&bo->tbo, NULL);
> > -			if (bo->shadow)
> > -				ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);
> > -			spin_unlock(&glob->lru_lock);
> >   		}
> >   
> > +		validated = true;
> >   		if (bo->tbo.type != ttm_bo_type_kernel) {
> >   			spin_lock(&vm->moved_lock);
> >   			list_move(&bo_base->vm_status, &vm->moved);
> > @@ -312,18 +357,16 @@ int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> >   		}
> >   	}
> >   
> > -	spin_lock(&glob->lru_lock);
> > -	list_for_each_entry(bo_base, &vm->idle, vm_status) {
> > -		struct amdgpu_bo *bo = bo_base->bo;
> > +	if (!validated) {
> > +		spin_lock(&glob->lru_lock);
> > +		ttm_bo_bulk_move_lru_tail(&vm->lru_bulk_move);
> > +		spin_unlock(&glob->lru_lock);
> > +		return 0;
> > +	}
> >   
> > -		if (!bo->parent)
> > -			continue;
> > +	memset(&vm->lru_bulk_move, 0, sizeof(vm->lru_bulk_move));
> >   
> > -		ttm_bo_move_to_lru_tail(&bo->tbo, NULL);
> > -		if (bo->shadow)
> > -			ttm_bo_move_to_lru_tail(&bo->shadow->tbo, NULL);
> > -	}
> > -	spin_unlock(&glob->lru_lock);
> > +	amdgpu_vm_move_to_lru_tail(adev, vm);
> >   
> >   	return r;
> >   }
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > index 67a15d4..92725ac 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > @@ -29,6 +29,7 @@
> >   #include <linux/rbtree.h>
> >   #include <drm/gpu_scheduler.h>
> >   #include <drm/drm_file.h>
> > +#include <drm/ttm/ttm_bo_driver.h>
> >   
> >   #include "amdgpu_sync.h"
> >   #include "amdgpu_ring.h"
> > @@ -226,6 +227,9 @@ struct amdgpu_vm {
> >   
> >   	/* Some basic info about the task */
> >   	struct amdgpu_task_info task_info;
> > +
> > +	/* Store positions of group of BOs */
> > +	struct ttm_lru_bulk_move lru_bulk_move;
> >   };
> >   
> >   struct amdgpu_vm_manager {
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx