Re: [PATCH 8/12] x86-64: update iommu/dma mapping functions to sg helpers

[Benny Halevy <bhalevy@panasas.com>]

Jens Axboe wrote:
<snip>
> @@ -323,13 +324,17 @@ static int __dma_map_cont(struct scatterlist *sg, int start, int stopat,
>  {
>  	unsigned long iommu_start = alloc_iommu(pages);
>  	unsigned long iommu_page = iommu_start; 
> -	int i;
> +	struct scatterlist *s;
> +	int i, nelems;
>  
>  	if (iommu_start == -1)
>  		return -1;
> +
> +	nelems = stopat - start;
> +	while (start--)
> +		sg = sg_next(sg);

Ouch.  This will suck if we merge many times in a long list.
How about keeping and passing start as a struct scatterlist * rather
than an index? (see attached example, (compiles, bu untested though)


>  	
> -	for (i = start; i < stopat; i++) {
> -		struct scatterlist *s = &sg[i];
> +	for_each_sg(sg, s, nelems, i) {
>  		unsigned long pages, addr;
>  		unsigned long phys_addr = s->dma_address;


There seems to be a bug hiding here as now i is in [0, nelems) now,
not [start, stopat) so "if (i == start)" below should turn into "if (i == 0)"

this is fixed by comparing pointers instead way in the attached example.

>  		
> @@ -360,12 +365,14 @@ static inline int dma_map_cont(struct scatterlist *sg, int start, int stopat,
>  		      struct scatterlist *sout,
>  		      unsigned long pages, int need)
>  {
> -	if (!need) { 
> +	if (!need) {
>  		BUG_ON(stopat - start != 1);
> -		*sout = sg[start]; 
> -		sout->dma_length = sg[start].length; 
> +		while (--start)
> +			sg = sg_next(sg);

same efficiency issue as above.

> +		*sout = *sg;
> +		sout->dma_length = sg->length;
>  		return 0;
> -	} 
> +	}
>  	return __dma_map_cont(sg, start, stopat, sout, pages);
>  }
>  		
<snip>

diff --git a/arch/x86_64/kernel/pci-gart.c b/arch/x86_64/kernel/pci-gart.c
index 8bc2ed7..48ce635 100644
--- a/arch/x86_64/kernel/pci-gart.c
+++ b/arch/x86_64/kernel/pci-gart.c
@@ -319,27 +319,23 @@ static int dma_map_sg_nonforce(struct device *dev, struct scatterlist *sg,
 }
 
 /* Map multiple scatterlist entries continuous into the first. */
-static int __dma_map_cont(struct scatterlist *sg, int start, int stopat,
+static int __dma_map_cont(struct scatterlist *start, int nelems,
 		      struct scatterlist *sout, unsigned long pages)
 {
 	unsigned long iommu_start = alloc_iommu(pages);
 	unsigned long iommu_page = iommu_start; 
 	struct scatterlist *s;
-	int i, nelems;
+	int i;
 
 	if (iommu_start == -1)
 		return -1;
 
-	nelems = stopat - start;
-	while (start--)
-		sg = sg_next(sg);
-	
-	for_each_sg(sg, s, nelems, i) {
+	for_each_sg(start, s, nelems, i) {
 		unsigned long pages, addr;
 		unsigned long phys_addr = s->dma_address;
 		
-		BUG_ON(i > start && s->offset);
-		if (i == start) {
+		BUG_ON(s != start && s->offset);
+		if (s == start) {
 			*sout = *s; 
 			sout->dma_address = iommu_bus_base;
 			sout->dma_address += iommu_page*PAGE_SIZE + s->offset;
@@ -361,19 +357,17 @@ static int __dma_map_cont(struct scatterlist *sg, int start, int stopat,
 	return 0;
 }
 
-static inline int dma_map_cont(struct scatterlist *sg, int start, int stopat,
+static inline int dma_map_cont(struct scatterlist *start, int nelems,
 		      struct scatterlist *sout,
 		      unsigned long pages, int need)
 {
 	if (!need) {
-		BUG_ON(stopat - start != 1);
-		while (--start)
-			sg = sg_next(sg);
-		*sout = *sg;
-		sout->dma_length = sg->length;
+		BUG_ON(nelems != 1);
+		*sout = *start;
+		sout->dma_length = start->length;
 		return 0;
 	}
-	return __dma_map_cont(sg, start, stopat, sout, pages);
+	return __dma_map_cont(start, nelems, sout, pages);
 }
 		
 /*
@@ -387,7 +381,7 @@ int gart_map_sg(struct device *dev, struct scatterlist *sg, int nents, int dir)
 	int start;
 	unsigned long pages = 0;
 	int need = 0, nextneed;
-	struct scatterlist *s, *ps;
+	struct scatterlist *s, *ps, *start_sg;
 
 	if (nents == 0) 
 		return 0;
@@ -397,6 +391,7 @@ int gart_map_sg(struct device *dev, struct scatterlist *sg, int nents, int dir)
 
 	out = 0;
 	start = 0;
+	start_sg = sg;
 	ps = NULL; /* shut up gcc */
 	for_each_sg(sg, s, nents, i) {
 		dma_addr_t addr = page_to_phys(s->page) + s->offset;
@@ -411,12 +406,13 @@ int gart_map_sg(struct device *dev, struct scatterlist *sg, int nents, int dir)
 			   boundary and the new one doesn't have an offset. */
 			if (!iommu_merge || !nextneed || !need || s->offset ||
 			    (ps->offset + ps->length) % PAGE_SIZE) { 
-				if (dma_map_cont(sg, start, i, sg+out, pages,
-						 need) < 0)
+				if (dma_map_cont(start_sg, i - start, sg+out,
+						  pages, need) < 0)
 					goto error;
 				out++;
 				pages = 0;
-				start = i;	
+				start = i;
+				start_sg = s;
 			}
 		}
 
@@ -424,7 +420,7 @@ int gart_map_sg(struct device *dev, struct scatterlist *sg, int nents, int dir)
 		pages += to_pages(s->offset, s->length);
 		ps = s;
 	}
-	if (dma_map_cont(sg, start, i, sg+out, pages, need) < 0)
+	if (dma_map_cont(start_sg, i - start, sg+out, pages, need) < 0)
 		goto error;
 	out++;
 	flush_gart();