Re: [PATCH 3/3] drm/amdkfd: Don't split unchanged SVM ranges

[philip yang <yangp@xxxxxxx>]

On 2021-12-08 3:24 a.m., Felix Kuehling wrote:

> If an existing SVM range overlaps an svm_range_set_attr call, we would
> normally split it in order to update only the overlapping part.
> However, if the attributes of the existing range would not be changed
> splitting it is unnecessary.
>
> Signed-off-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
>     
    Reviewed-by: Philip Yang <Philip.Yang@xxxxxxx>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 9ea3981545e5..c93a26e6318b 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -1853,18 +1853,24 @@ svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
>  	node = interval_tree_iter_first(&svms->objects, start, last);
>  	while (node) {
>  		struct interval_tree_node *next;
> -		struct svm_range *old;
>  		unsigned long next_start;
>  
>  		pr_debug("found overlap node [0x%lx 0x%lx]\n", node->start,
>  			 node->last);
>  
> -		old = container_of(node, struct svm_range, it_node);
> +		prange = container_of(node, struct svm_range, it_node);
>  		next = interval_tree_iter_next(node, start, last);
>  		next_start = min(node->last, last) + 1;
>  
> -		if (node->start < start || node->last > last) {
> -			/* node intersects the updated range, clone+split it */
> +		if (svm_range_is_same_attrs(p, prange, nattr, attrs)) {
> +			/* nothing to do */
> +		} else if (node->start < start || node->last > last) {
> +			/* node intersects the update range and its attributes
> +			 * will change. Clone and split it, apply updates only
> +			 * to the overlapping part
> +			 */
> +			struct svm_range *old = prange;
> +
>  			prange = svm_range_clone(old);
>  			if (!prange) {
>  				r = -ENOMEM;
> @@ -1873,6 +1879,7 @@ svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
>  
>  			list_add(&old->remove_list, remove_list);
>  			list_add(&prange->insert_list, insert_list);
> +			list_add(&prange->update_list, update_list);
>  
>  			if (node->start < start) {
>  				pr_debug("change old range start\n");
> @@ -1892,16 +1899,12 @@ svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
>  			/* The node is contained within start..last,
>  			 * just update it
>  			 */
> -			prange = old;
> -		}
> -
> -		if (!svm_range_is_same_attrs(p, prange, nattr, attrs))
>  			list_add(&prange->update_list, update_list);
> +		}
>  
>  		/* insert a new node if needed */
>  		if (node->start > start) {
> -			prange = svm_range_new(prange->svms, start,
> -					       node->start - 1);
> +			prange = svm_range_new(svms, start, node->start - 1);
>  			if (!prange) {
>  				r = -ENOMEM;
>  				goto out;
>