Convert backwards goto into a while loop

[PATCH] Convert backwards goto into a while loop

Posted by Shachar Raindel 2 years, 8 months ago

The function vaddr_get_pfns used a goto retry structure to implement
retrying.  This is discouraged by the coding style guide (which is
only recommending goto for handling function exits). Convert the code
to a while loop, making it explicit that the follow block only runs
when the pin attempt failed.

Signed-off-by: Shachar Raindel <shacharr@google.com>
---
 drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 23c24fe98c00..7f38d7fc3f62 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -570,27 +570,28 @@ static int vaddr_get_pfns(struct mm_struct *mm, unsigned long vaddr,
 		}
 
 		*pfn = page_to_pfn(pages[0]);
-		goto done;
-	}
+	} else
+		do {
+
+			/* This is not a normal page, lookup PFN for P2P DMA */
+			vaddr = untagged_addr(vaddr);
 
-	vaddr = untagged_addr(vaddr);
+			vma = vma_lookup(mm, vaddr);
 
-retry:
-	vma = vma_lookup(mm, vaddr);
+			if (!vma || !(vma->vm_flags & VM_PFNMAP))
+				break;
 
-	if (vma && vma->vm_flags & VM_PFNMAP) {
-		ret = follow_fault_pfn(vma, mm, vaddr, pfn, prot & IOMMU_WRITE);
-		if (ret == -EAGAIN)
-			goto retry;
+			ret = follow_fault_pfn(vma, mm, vaddr, pfn,
+					       prot & IOMMU_WRITE);
+			if (ret)
+				continue; /* Retry for EAGAIN, otherwise bail */
 
-		if (!ret) {
 			if (is_invalid_reserved_pfn(*pfn))
 				ret = 1;
 			else
 				ret = -EFAULT;
-		}
-	}
-done:
+		} while (ret == -EAGAIN);
+
 	mmap_read_unlock(mm);
 	return ret;
 }
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH] Convert backwards goto into a while loop

Posted by Alex Williamson 2 years, 8 months ago

On Wed, 28 Dec 2022 21:32:12 +0000
Shachar Raindel <shacharr@google.com> wrote:

> The function vaddr_get_pfns used a goto retry structure to implement
> retrying.  This is discouraged by the coding style guide (which is
> only recommending goto for handling function exits). Convert the code
> to a while loop, making it explicit that the follow block only runs
> when the pin attempt failed.

What coding style guide are you referring to?  In
Documentation/process/coding-style.rst I only see goto mentioned in 7)
Centralized exiting of functions, which suggests it's a useful
mechanism for creating centralized cleanup code, while noting "[a]lbeit
deprecated by *some people*", emphasis added.  This doesn't suggest to
me such a strong statement as implied in this commit log.
 
> Signed-off-by: Shachar Raindel <shacharr@google.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c index 23c24fe98c00..7f38d7fc3f62
> 100644 --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -570,27 +570,28 @@ static int vaddr_get_pfns(struct mm_struct *mm,
> unsigned long vaddr, }
>  
>  		*pfn = page_to_pfn(pages[0]);
> -		goto done;
> -	}
> +	} else

Coding style would however discourage skipping the braces around this
half of the branch, as done here, as a) they're used around the former
half and b) the below is not a single simple statement.  Thanks,

Alex

> +		do {
> +
> +			/* This is not a normal page, lookup PFN for P2P DMA */
> +			vaddr = untagged_addr(vaddr);
>  
> -	vaddr = untagged_addr(vaddr);
> +			vma = vma_lookup(mm, vaddr);
>  
> -retry:
> -	vma = vma_lookup(mm, vaddr);
> +			if (!vma || !(vma->vm_flags & VM_PFNMAP))
> +				break;
>  
> -	if (vma && vma->vm_flags & VM_PFNMAP) {
> -		ret = follow_fault_pfn(vma, mm, vaddr, pfn, prot & IOMMU_WRITE);
> -		if (ret == -EAGAIN)
> -			goto retry;
> +			ret = follow_fault_pfn(vma, mm, vaddr, pfn,
> +					       prot & IOMMU_WRITE);
> +			if (ret)
> +				continue; /* Retry for EAGAIN, otherwise bail */
>  
> -		if (!ret) {
>  			if (is_invalid_reserved_pfn(*pfn))
>  				ret = 1;
>  			else
>  				ret = -EFAULT;
> -		}
> -	}
> -done:
> +		} while (ret == -EAGAIN);
> +
>  	mmap_read_unlock(mm);
>  	return ret;
>  }

[PATCH v2] Convert backwards goto with a while loop

Posted by Shachar Raindel 2 years, 8 months ago

The function vaddr_get_pfns used a goto retry structure to implement
retrying.  This is a gray area in the coding style guide (which is
only explicitly recommending goto for handling function exits).
Convert the code to a while loop, making it explicit that the
following block only runs when the pin attempt failed.

Signed-off-by: Shachar Raindel <shacharr@google.com>
---

Changelog:

v1 -> v2: Refine commit message, fix minor code style issue

 drivers/vfio/vfio_iommu_type1.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 23c24fe98c00..6335eabe1b7c 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -570,27 +570,28 @@ static int vaddr_get_pfns(struct mm_struct *mm, unsigned long vaddr,
 		}
 
 		*pfn = page_to_pfn(pages[0]);
-		goto done;
-	}
+	} else {
+		do {
+
+			/* This is not a normal page, lookup PFN for P2P DMA */
+			vaddr = untagged_addr(vaddr);
 
-	vaddr = untagged_addr(vaddr);
+			vma = vma_lookup(mm, vaddr);
 
-retry:
-	vma = vma_lookup(mm, vaddr);
+			if (!vma || !(vma->vm_flags & VM_PFNMAP))
+				break;
 
-	if (vma && vma->vm_flags & VM_PFNMAP) {
-		ret = follow_fault_pfn(vma, mm, vaddr, pfn, prot & IOMMU_WRITE);
-		if (ret == -EAGAIN)
-			goto retry;
+			ret = follow_fault_pfn(vma, mm, vaddr, pfn,
+					       prot & IOMMU_WRITE);
+			if (ret)
+				continue; /* Retry for EAGAIN, otherwise bail */
 
-		if (!ret) {
 			if (is_invalid_reserved_pfn(*pfn))
 				ret = 1;
 			else
 				ret = -EFAULT;
-		}
-	}
-done:
+		} while (ret == -EAGAIN);
+	}
 	mmap_read_unlock(mm);
 	return ret;
 }
-- 
2.39.0.314.g84b9a713c41-goog