Re: [PATCH 6/7] drm/amdkfd: use tlb_seq from the VM subsystem for SVM as well

[Felix Kuehling <felix.kuehling@xxxxxxx>]

Am 2022-03-17 um 09:50 schrieb Christian König:
> Instead of hand rolling the table_freed parameter.
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  5 ++---
>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 18 ++++++++----------
>   drivers/gpu/drm/amd/amdkfd/kfd_chardev.c      | 19 ++++++++-----------
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c      |  2 +-
>   4 files changed, 19 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> index 4cb14c2fe53f..9fcfb5e1b55c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h
> @@ -273,9 +273,8 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
>   int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>   		struct amdgpu_device *adev, struct kgd_mem *mem, void *drm_priv,
>   		uint64_t *size);
> -int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
> -		struct amdgpu_device *adev, struct kgd_mem *mem, void *drm_priv,
> -		bool *table_freed);
> +int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(struct amdgpu_device *adev,
> +					  struct kgd_mem *mem, void *drm_priv);
>   int amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu(
>   		struct amdgpu_device *adev, struct kgd_mem *mem, void *drm_priv);
>   int amdgpu_amdkfd_gpuvm_sync_memory(
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index cd89d2e46852..57b521bb1eec 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1093,8 +1093,7 @@ static void unmap_bo_from_gpuvm(struct kgd_mem *mem,
>   
>   static int update_gpuvm_pte(struct kgd_mem *mem,
>   			    struct kfd_mem_attachment *entry,
> -			    struct amdgpu_sync *sync,
> -			    bool *table_freed)
> +			    struct amdgpu_sync *sync)
>   {
>   	struct amdgpu_bo_va *bo_va = entry->bo_va;
>   	struct amdgpu_device *adev = entry->adev;
> @@ -1105,7 +1104,7 @@ static int update_gpuvm_pte(struct kgd_mem *mem,
>   		return ret;
>   
>   	/* Update the page tables  */
> -	ret = amdgpu_vm_bo_update(adev, bo_va, false, table_freed);
> +	ret = amdgpu_vm_bo_update(adev, bo_va, false, NULL);
>   	if (ret) {
>   		pr_err("amdgpu_vm_bo_update failed\n");
>   		return ret;
> @@ -1117,8 +1116,7 @@ static int update_gpuvm_pte(struct kgd_mem *mem,
>   static int map_bo_to_gpuvm(struct kgd_mem *mem,
>   			   struct kfd_mem_attachment *entry,
>   			   struct amdgpu_sync *sync,
> -			   bool no_update_pte,
> -			   bool *table_freed)
> +			   bool no_update_pte)
>   {
>   	int ret;
>   
> @@ -1135,7 +1133,7 @@ static int map_bo_to_gpuvm(struct kgd_mem *mem,
>   	if (no_update_pte)
>   		return 0;
>   
> -	ret = update_gpuvm_pte(mem, entry, sync, table_freed);
> +	ret = update_gpuvm_pte(mem, entry, sync);
>   	if (ret) {
>   		pr_err("update_gpuvm_pte() failed\n");
>   		goto update_gpuvm_pte_failed;
> @@ -1745,7 +1743,7 @@ int amdgpu_amdkfd_gpuvm_free_memory_of_gpu(
>   
>   int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
>   		struct amdgpu_device *adev, struct kgd_mem *mem,
> -		void *drm_priv, bool *table_freed)
> +		void *drm_priv)
>   {
>   	struct amdgpu_vm *avm = drm_priv_to_vm(drm_priv);
>   	int ret;
> @@ -1832,7 +1830,7 @@ int amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
>   			 entry->va, entry->va + bo_size, entry);
>   
>   		ret = map_bo_to_gpuvm(mem, entry, ctx.sync,
> -				      is_invalid_userptr, table_freed);
> +				      is_invalid_userptr);
>   		if (ret) {
>   			pr_err("Failed to map bo to gpuvm\n");
>   			goto out_unreserve;
> @@ -2300,7 +2298,7 @@ static int validate_invalid_user_pages(struct amdkfd_process_info *process_info)
>   				continue;
>   
>   			kfd_mem_dmaunmap_attachment(mem, attachment);
> -			ret = update_gpuvm_pte(mem, attachment, &sync, NULL);
> +			ret = update_gpuvm_pte(mem, attachment, &sync);
>   			if (ret) {
>   				pr_err("%s: update PTE failed\n", __func__);
>   				/* make sure this gets validated again */
> @@ -2506,7 +2504,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence **ef)
>   				continue;
>   
>   			kfd_mem_dmaunmap_attachment(mem, attachment);
> -			ret = update_gpuvm_pte(mem, attachment, &sync_obj, NULL);
> +			ret = update_gpuvm_pte(mem, attachment, &sync_obj);
>   			if (ret) {
>   				pr_debug("Memory eviction: update PTE failed. Try again\n");
>   				goto validate_map_fail;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index e1e2362841f8..91f82a9ccdaf 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -1146,7 +1146,6 @@ static int kfd_ioctl_map_memory_to_gpu(struct file *filep,
>   	long err = 0;
>   	int i;
>   	uint32_t *devices_arr = NULL;
> -	bool table_freed = false;
>   
>   	if (!args->n_devices) {
>   		pr_debug("Device IDs array empty\n");
> @@ -1208,7 +1207,7 @@ static int kfd_ioctl_map_memory_to_gpu(struct file *filep,
>   
>   		err = amdgpu_amdkfd_gpuvm_map_memory_to_gpu(
>   			peer_pdd->dev->adev, (struct kgd_mem *)mem,
> -			peer_pdd->drm_priv, &table_freed);
> +			peer_pdd->drm_priv);
>   		if (err) {
>   			struct pci_dev *pdev = peer_pdd->dev->adev->pdev;
>   
> @@ -1233,13 +1232,11 @@ static int kfd_ioctl_map_memory_to_gpu(struct file *filep,
>   	}
>   
>   	/* Flush TLBs after waiting for the page table updates to complete */
> -	if (table_freed || !kfd_flush_tlb_after_unmap(dev)) {
> -		for (i = 0; i < args->n_devices; i++) {
> -			peer_pdd = kfd_process_device_data_by_id(p, devices_arr[i]);
> -			if (WARN_ON_ONCE(!peer_pdd))
> -				continue;
> -			kfd_flush_tlb(peer_pdd, TLB_FLUSH_LEGACY);
> -		}
> +	for (i = 0; i < args->n_devices; i++) {
> +		peer_pdd = kfd_process_device_data_by_id(p, devices_arr[i]);
> +		if (WARN_ON_ONCE(!peer_pdd))
> +			continue;
> +		kfd_flush_tlb(peer_pdd, TLB_FLUSH_LEGACY);

My knee-jerk reaction was that you need to check 
!kfd_flush_tlb_after_unmap(dev) somewhere. But I guess in cases where we 
flush after unmap, the sequence number would tell kfd_flush_tlb that no 
flush is needed.

Patches 5-7 are

Reviewed-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>

Patches 1-4 are

Acked-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>

Regards,
  Felix


>   	}
>   	kfree(devices_arr);
>   
> @@ -2203,8 +2200,8 @@ static int criu_restore_bo(struct kfd_process *p,
>   		if (IS_ERR(peer_pdd))
>   			return PTR_ERR(peer_pdd);
>   
> -		ret = amdgpu_amdkfd_gpuvm_map_memory_to_gpu(peer->adev, kgd_mem, peer_pdd->drm_priv,
> -							    NULL);
> +		ret = amdgpu_amdkfd_gpuvm_map_memory_to_gpu(peer->adev, kgd_mem,
> +							    peer_pdd->drm_priv);
>   		if (ret) {
>   			pr_err("Failed to map to gpu %d/%d\n", j, p->n_pdds);
>   			return ret;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 4a8a047b7593..ac8123c1ee8f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -722,7 +722,7 @@ static int kfd_process_alloc_gpuvm(struct kfd_process_device *pdd,
>   		goto err_alloc_mem;
>   
>   	err = amdgpu_amdkfd_gpuvm_map_memory_to_gpu(kdev->adev, *mem,
> -			pdd->drm_priv, NULL);
> +			pdd->drm_priv);
>   	if (err)
>   		goto err_map_mem;
>