Improve swiotlb performance by using physical addresses

In the case of swiotlb we already have the start of the region and the number of slabs that give us the region size. Instead of having to call virt_to_phys on two pointers we can just take advantage of the fact that the region is linear and just compute the end based on the start plus the size.

Signed-off-by Alexander Duyck <[email protected]>

---

 lib/swiotlb.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

lib/swiotlb.c +12 -14

index f114bf6..5cc4d4e 100644
@@ -57,11 +57,11 @@ int swiotlb_force;
  * swiotlb_tbl_sync_single_*, to see if the memory was in fact allocated by this
  * API.
  */
-static char *io_tlb_start, *io_tlb_end;
+static char *io_tlb_start;
 
 /*
- * The number of IO TLB blocks (in groups of 64) between io_tlb_start and
- * io_tlb_end.  This is command line adjustable via setup_io_tlb_npages.
+ * The number of IO TLB blocks (in groups of 64).
+ * This is command line adjustable via setup_io_tlb_npages.
  */
 static unsigned long io_tlb_nslabs;
 

@@ -128,11 +128,11 @@ void swiotlb_print_info(void)
 	phys_addr_t pstart, pend;
 
 	pstart = virt_to_phys(io_tlb_start);
-	pend = virt_to_phys(io_tlb_end);
+	pend = pstart + bytes;
 
 	printk(KERN_INFO "software IO TLB [mem %#010llx-%#010llx] (%luMB) mapped at [%p-%p]\n",
 	       (unsigned long long)pstart, (unsigned long long)pend - 1,
-	       bytes >> 20, io_tlb_start, io_tlb_end - 1);
+	       bytes >> 20, io_tlb_start, io_tlb_start + bytes - 1);
 }
 
 void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)

@@ -143,12 +143,10 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 
 	io_tlb_nslabs = nslabs;
 	io_tlb_start = tlb;
-	io_tlb_end = io_tlb_start + bytes;
 
 	/*
 	 * Allocate and initialize the free list array.  This array is used
 	 * to find contiguous free memory regions of size up to IO_TLB_SEGSIZE
-	 * between io_tlb_start and io_tlb_end.
 	 */
 	io_tlb_list = alloc_bootmem_pages(PAGE_ALIGN(io_tlb_nslabs * sizeof(int)));
 	for (i = 0; i < io_tlb_nslabs; i++)

@@ -254,14 +252,12 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
 
 	io_tlb_nslabs = nslabs;
 	io_tlb_start = tlb;
-	io_tlb_end = io_tlb_start + bytes;
 
 	memset(io_tlb_start, 0, bytes);
 
 	/*
 	 * Allocate and initialize the free list array.  This array is used
 	 * to find contiguous free memory regions of size up to IO_TLB_SEGSIZE
-	 * between io_tlb_start and io_tlb_end.
 	 */
 	io_tlb_list = (unsigned int *)__get_free_pages(GFP_KERNEL,
 	                              get_order(io_tlb_nslabs * sizeof(int)));

@@ -304,7 +300,6 @@ cleanup3:
 	                                                 sizeof(int)));
 	io_tlb_list = NULL;
 cleanup2:
-	io_tlb_end = NULL;
 	io_tlb_start = NULL;
 	io_tlb_nslabs = 0;
 	return -ENOMEM;

@@ -339,8 +334,10 @@ void __init swiotlb_free(void)
 
 static int is_swiotlb_buffer(phys_addr_t paddr)
 {
-	return paddr >= virt_to_phys(io_tlb_start) &&
-		paddr < virt_to_phys(io_tlb_end);
+	phys_addr_t swiotlb_start = virt_to_phys(io_tlb_start);
+
+	return paddr >= swiotlb_start &&
+		paddr < (swiotlb_start + (io_tlb_nslabs << IO_TLB_SHIFT));
 }
 
 /*

@@ -938,6 +935,8 @@ EXPORT_SYMBOL(swiotlb_dma_mapping_error);
 int
 swiotlb_dma_supported(struct device *hwdev, u64 mask)
 {
-	return swiotlb_virt_to_bus(hwdev, io_tlb_end - 1) <= mask;
+	unsigned long bytes = io_tlb_nslabs << IO_TLB_SHIFT;
+
+	return swiotlb_virt_to_bus(hwdev, io_tlb_start + bytes - 1) <= mask;
 }
 EXPORT_SYMBOL(swiotlb_dma_supported);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 4 lines · Alexander Duyck

In the case of swiotlb we already have the start of the region and the number of slabs that give us the region size. Instead of having to call virt_to_phys on two pointers we can just take advantage of the fact that the region is linear and just compute the end based on the start plus the size.

Why not take advantage of 'the fact that the region is linear' and just pre-compute the end in swiotlb_init_with_tbl?

That way the logic in is_swiotlb_buffer is even simpler?

Quoted 10 lines · Konrad Rzeszutek Wilk

Quoted 4 lines · Alexander Duyck

In the case of swiotlb we already have the start of the region and the number of slabs that give us the region size. Instead of having to call virt_to_phys on two pointers we can just take advantage of the fact that the region is linear and just compute the end based on the start plus the size.

Why not take advantage of 'the fact that the region is linear' and just pre-compute the end in swiotlb_init_with_tbl?

That way the logic in is_swiotlb_buffer is even simpler?

Using a pre-computed end point based on a virtual address is more expensive in the x86_64 case. The calls to __phys_addr require a separate function call. By just using the physical address of the start and adding the offset I can avoid the second call and the compiler will take advantage of the smaller function size. The result is that is_swiotlb_buffer will be inlined.

Thanks,

Alex

This change makes it so that io_tlb_start contains a physical address instead of a virtual address. The advantage to this is that we can avoid costly translations between virtual and physical addresses when comparing the io_tlb_start against DMA addresses.

Signed-off-by Alexander Duyck <[email protected]>

---

 lib/swiotlb.c |   61 +++++++++++++++++++++++++++++----------------------------
 1 files changed, 31 insertions(+), 30 deletions(-)

lib/swiotlb.c +7 -6

index 5cc4d4e..02abb72 100644
@@ -57,7 +57,7 @@ int swiotlb_force;
  * swiotlb_tbl_sync_single_*, to see if the memory was in fact allocated by this
  * API.
  */
-static char *io_tlb_start;
+phys_addr_t io_tlb_start;
 
 /*
  * The number of IO TLB blocks (in groups of 64).

@@ -125,14 +125,15 @@ static dma_addr_t swiotlb_virt_to_bus(struct device *hwdev,
 void swiotlb_print_info(void)
 {
 	unsigned long bytes = io_tlb_nslabs << IO_TLB_SHIFT;
-	phys_addr_t pstart, pend;
+	unsigned char *vstart, *vend;
 
-	pstart = virt_to_phys(io_tlb_start);
-	pend = pstart + bytes;
+	vstart = phys_to_virt(io_tlb_start);
+	vend = vstart + bytes;
 
 	printk(KERN_INFO "software IO TLB [mem %#010llx-%#010llx] (%luMB) mapped at [%p-%p]\n",
-	       (unsigned long long)pstart, (unsigned long long)pend - 1,
-	       bytes >> 20, io_tlb_start, io_tlb_start + bytes - 1);
+	       (unsigned long long)io_tlb_start,
+	       (unsigned long long)io_tlb_start + bytes - 1,
+	       bytes >> 20, vstart, vend - 1);
 }
 
 void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)

lib/swiotlb.c +18 -18

 	/*
 	 * Allocate and initialize the free list array.  This array is used

@@ -171,6 +172,7 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 static void __init
 swiotlb_init_with_default_size(size_t default_size, int verbose)
 {
+	unsigned char *vstart;
 	unsigned long bytes;
 
 	if (!io_tlb_nslabs) {

@@ -183,11 +185,11 @@ swiotlb_init_with_default_size(size_t default_size, int verbose)
 	/*
 	 * Get IO TLB memory from the low pages
 	 */
-	io_tlb_start = alloc_bootmem_low_pages(PAGE_ALIGN(bytes));
-	if (!io_tlb_start)
+	vstart = alloc_bootmem_low_pages(PAGE_ALIGN(bytes));
+	if (!vstart)
 		panic("Cannot allocate SWIOTLB buffer");
 
-	swiotlb_init_with_tbl(io_tlb_start, io_tlb_nslabs, verbose);
+	swiotlb_init_with_tbl(vstart, io_tlb_nslabs, verbose);
 }
 
 void __init

@@ -205,6 +207,7 @@ int
 swiotlb_late_init_with_default_size(size_t default_size)
 {
 	unsigned long bytes, req_nslabs = io_tlb_nslabs;
+	unsigned char *vstart = NULL;
 	unsigned int order;
 	int rc = 0;
 

@@ -221,14 +224,14 @@ swiotlb_late_init_with_default_size(size_t default_size)
 	bytes = io_tlb_nslabs << IO_TLB_SHIFT;
 
 	while ((SLABS_PER_PAGE << order) > IO_TLB_MIN_SLABS) {
-		io_tlb_start = (void *)__get_free_pages(GFP_DMA | __GFP_NOWARN,
-							order);
-		if (io_tlb_start)
+		vstart = (void *)__get_free_pages(GFP_DMA | __GFP_NOWARN,
+						  order);
+		if (vstart)
 			break;
 		order--;
 	}
 
-	if (!io_tlb_start) {
+	if (!vstart) {
 		io_tlb_nslabs = req_nslabs;
 		return -ENOMEM;
 	}

@@ -237,9 +240,9 @@ swiotlb_late_init_with_default_size(size_t default_size)
 		       "for software IO TLB\n", (PAGE_SIZE << order) >> 20);
 		io_tlb_nslabs = SLABS_PER_PAGE << order;
 	}
-	rc = swiotlb_late_init_with_tbl(io_tlb_start, io_tlb_nslabs);
+	rc = swiotlb_late_init_with_tbl(vstart, io_tlb_nslabs);
 	if (rc)
-		free_pages((unsigned long)io_tlb_start, order);
+		free_pages((unsigned long)vstart, order);
 	return rc;
 }
 

@@ -251,9 +254,9 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
 	bytes = nslabs << IO_TLB_SHIFT;
 
 	io_tlb_nslabs = nslabs;
-	io_tlb_start = tlb;
+	io_tlb_start = virt_to_phys(tlb);
 
-	memset(io_tlb_start, 0, bytes);
+	memset(tlb, 0, bytes);
 
 	/*
 	 * Allocate and initialize the free list array.  This array is used

@@ -300,7 +303,7 @@ cleanup3:
 	                                                 sizeof(int)));
 	io_tlb_list = NULL;
 cleanup2:
-	io_tlb_start = NULL;
+	io_tlb_start = 0;
 	io_tlb_nslabs = 0;
 	return -ENOMEM;
 }

@@ -317,7 +320,7 @@ void __init swiotlb_free(void)
 			   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *
 								 sizeof(int)));
-		free_pages((unsigned long)io_tlb_start,
+		free_pages((unsigned long)phys_to_virt(io_tlb_start),
 			   get_order(io_tlb_nslabs << IO_TLB_SHIFT));
 	} else {
 		free_bootmem_late(__pa(io_tlb_overflow_buffer),

@@ -326,7 +329,7 @@ void __init swiotlb_free(void)
 				  PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_bootmem_late(__pa(io_tlb_list),
 				  PAGE_ALIGN(io_tlb_nslabs * sizeof(int)));
-		free_bootmem_late(__pa(io_tlb_start),
+		free_bootmem_late(io_tlb_start,
 				  PAGE_ALIGN(io_tlb_nslabs << IO_TLB_SHIFT));
 	}
 	io_tlb_nslabs = 0;

@@ -334,10 +337,8 @@ void __init swiotlb_free(void)
 
 static int is_swiotlb_buffer(phys_addr_t paddr)
 {
-	phys_addr_t swiotlb_start = virt_to_phys(io_tlb_start);
-
-	return paddr >= swiotlb_start &&
-		paddr < (swiotlb_start + (io_tlb_nslabs << IO_TLB_SHIFT));
+	return paddr >= io_tlb_start &&
+		paddr < (io_tlb_start + (io_tlb_nslabs << IO_TLB_SHIFT));
 }
 
 /*

lib/swiotlb.c +1 -1

 			/*
 			 * Update the indices to avoid searching in the next

@@ -494,7 +495,7 @@ static void *
 map_single(struct device *hwdev, phys_addr_t phys, size_t size,
 	   enum dma_data_direction dir)
 {
-	dma_addr_t start_dma_addr = swiotlb_virt_to_bus(hwdev, io_tlb_start);
+	dma_addr_t start_dma_addr = phys_to_dma(hwdev, io_tlb_start);
 
 	return swiotlb_tbl_map_single(hwdev, start_dma_addr, phys, size, dir);
 }

lib/swiotlb.c +2 -3

 	phys_addr_t phys = io_tlb_orig_addr[index];
 
 	/*

@@ -549,7 +550,7 @@ swiotlb_tbl_sync_single(struct device *hwdev, char *dma_addr, size_t size,
 			enum dma_data_direction dir,
 			enum dma_sync_target target)
 {
-	int index = (dma_addr - io_tlb_start) >> IO_TLB_SHIFT;
+	int index = (dma_addr - (char *)phys_to_virt(io_tlb_start)) >> IO_TLB_SHIFT;
 	phys_addr_t phys = io_tlb_orig_addr[index];
 
 	phys += ((unsigned long)dma_addr & ((1 << IO_TLB_SHIFT) - 1));

@@ -937,6 +938,6 @@ swiotlb_dma_supported(struct device *hwdev, u64 mask)
 {
 	unsigned long bytes = io_tlb_nslabs << IO_TLB_SHIFT;
 
-	return swiotlb_virt_to_bus(hwdev, io_tlb_start + bytes - 1) <= mask;
+	return phys_to_dma(hwdev, io_tlb_start + bytes - 1) <= mask;
 }
 EXPORT_SYMBOL(swiotlb_dma_supported);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 44 lines · Alexander Duyck

This change makes it so that io_tlb_start contains a physical address instead of a virtual address. The advantage to this is that we can avoid costly translations between virtual and physical addresses when comparing the io_tlb_start against DMA addresses.

Signed-off-by Alexander Duyck <[email protected]>

---

 lib/swiotlb.c |   61 +++++++++++++++++++++++++++++----------------------------
 1 files changed, 31 insertions(+), 30 deletions(-)

lib/swiotlb.c +7 -6

index 5cc4d4e..02abb72 100644
@@ -57,7 +57,7 @@ int swiotlb_force;
  * swiotlb_tbl_sync_single_*, to see if the memory was in fact allocated by this
  * API.
  */
-static char *io_tlb_start;
+phys_addr_t io_tlb_start;
 
 /*
  * The number of IO TLB blocks (in groups of 64).

@@ -125,14 +125,15 @@ static dma_addr_t swiotlb_virt_to_bus(struct device *hwdev,
 void swiotlb_print_info(void)
 {
 	unsigned long bytes = io_tlb_nslabs << IO_TLB_SHIFT;
-	phys_addr_t pstart, pend;
+	unsigned char *vstart, *vend;
 
-	pstart = virt_to_phys(io_tlb_start);
-	pend = pstart + bytes;
+	vstart = phys_to_virt(io_tlb_start);
+	vend = vstart + bytes;
 
 	printk(KERN_INFO "software IO TLB [mem %#010llx-%#010llx] (%luMB) mapped at [%p-%p]\n",
-	       (unsigned long long)pstart, (unsigned long long)pend - 1,
-	       bytes >> 20, io_tlb_start, io_tlb_start + bytes - 1);
+	       (unsigned long long)io_tlb_start,
+	       (unsigned long long)io_tlb_start + bytes - 1,
+	       bytes >> 20, vstart, vend - 1);
 }
 

lib/swiotlb.c void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose) · line 146

Quoted 7 lines

lib/swiotlb.c +1 -1

 void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)

@@ -142,7 +143,7 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 	bytes = nslabs << IO_TLB_SHIFT;
 
 	io_tlb_nslabs = nslabs;
-	io_tlb_start = tlb;
+	io_tlb_start = __pa(tlb);

Why not 'virt_to_phys' ?

Quoted 118 lines

lib/swiotlb.c +18 -18

 	/*
 	 * Allocate and initialize the free list array.  This array is used

@@ -171,6 +172,7 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 static void __init
 swiotlb_init_with_default_size(size_t default_size, int verbose)
 {
+	unsigned char *vstart;
 	unsigned long bytes;
 
 	if (!io_tlb_nslabs) {

@@ -183,11 +185,11 @@ swiotlb_init_with_default_size(size_t default_size, int verbose)
 	/*
 	 * Get IO TLB memory from the low pages
 	 */
-	io_tlb_start = alloc_bootmem_low_pages(PAGE_ALIGN(bytes));
-	if (!io_tlb_start)
+	vstart = alloc_bootmem_low_pages(PAGE_ALIGN(bytes));
+	if (!vstart)
 		panic("Cannot allocate SWIOTLB buffer");
 
-	swiotlb_init_with_tbl(io_tlb_start, io_tlb_nslabs, verbose);
+	swiotlb_init_with_tbl(vstart, io_tlb_nslabs, verbose);
 }
 
 void __init

@@ -205,6 +207,7 @@ int
 swiotlb_late_init_with_default_size(size_t default_size)
 {
 	unsigned long bytes, req_nslabs = io_tlb_nslabs;
+	unsigned char *vstart = NULL;
 	unsigned int order;
 	int rc = 0;
 

@@ -221,14 +224,14 @@ swiotlb_late_init_with_default_size(size_t default_size)
 	bytes = io_tlb_nslabs << IO_TLB_SHIFT;
 
 	while ((SLABS_PER_PAGE << order) > IO_TLB_MIN_SLABS) {
-		io_tlb_start = (void *)__get_free_pages(GFP_DMA | __GFP_NOWARN,
-							order);
-		if (io_tlb_start)
+		vstart = (void *)__get_free_pages(GFP_DMA | __GFP_NOWARN,
+						  order);
+		if (vstart)
 			break;
 		order--;
 	}
 
-	if (!io_tlb_start) {
+	if (!vstart) {
 		io_tlb_nslabs = req_nslabs;
 		return -ENOMEM;
 	}

@@ -237,9 +240,9 @@ swiotlb_late_init_with_default_size(size_t default_size)
 		       "for software IO TLB\n", (PAGE_SIZE << order) >> 20);
 		io_tlb_nslabs = SLABS_PER_PAGE << order;
 	}
-	rc = swiotlb_late_init_with_tbl(io_tlb_start, io_tlb_nslabs);
+	rc = swiotlb_late_init_with_tbl(vstart, io_tlb_nslabs);
 	if (rc)
-		free_pages((unsigned long)io_tlb_start, order);
+		free_pages((unsigned long)vstart, order);
 	return rc;
 }
 

@@ -251,9 +254,9 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
 	bytes = nslabs << IO_TLB_SHIFT;
 
 	io_tlb_nslabs = nslabs;
-	io_tlb_start = tlb;
+	io_tlb_start = virt_to_phys(tlb);
 
-	memset(io_tlb_start, 0, bytes);
+	memset(tlb, 0, bytes);
 
 	/*
 	 * Allocate and initialize the free list array.  This array is used

@@ -300,7 +303,7 @@ cleanup3:
 	                                                 sizeof(int)));
 	io_tlb_list = NULL;
 cleanup2:
-	io_tlb_start = NULL;
+	io_tlb_start = 0;
 	io_tlb_nslabs = 0;
 	return -ENOMEM;
 }

@@ -317,7 +320,7 @@ void __init swiotlb_free(void)
 			   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *
 								 sizeof(int)));
-		free_pages((unsigned long)io_tlb_start,
+		free_pages((unsigned long)phys_to_virt(io_tlb_start),
 			   get_order(io_tlb_nslabs << IO_TLB_SHIFT));
 	} else {
 		free_bootmem_late(__pa(io_tlb_overflow_buffer),

@@ -326,7 +329,7 @@ void __init swiotlb_free(void)
 				  PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_bootmem_late(__pa(io_tlb_list),
 				  PAGE_ALIGN(io_tlb_nslabs * sizeof(int)));
-		free_bootmem_late(__pa(io_tlb_start),
+		free_bootmem_late(io_tlb_start,
 				  PAGE_ALIGN(io_tlb_nslabs << IO_TLB_SHIFT));
 	}
 	io_tlb_nslabs = 0;

@@ -334,10 +337,8 @@ void __init swiotlb_free(void)
 
 static int is_swiotlb_buffer(phys_addr_t paddr)
 {
-	phys_addr_t swiotlb_start = virt_to_phys(io_tlb_start);
-
-	return paddr >= swiotlb_start &&
-		paddr < (swiotlb_start + (io_tlb_nslabs << IO_TLB_SHIFT));
+	return paddr >= io_tlb_start &&
+		paddr < (io_tlb_start + (io_tlb_nslabs << IO_TLB_SHIFT));
 }
 
 /*

lib/swiotlb.c void *swiotlb_tbl_map_single(struct device *hwdev, dma_addr_t tbl_dma_addr, · line 454

Quoted 5 lines

lib/swiotlb.c +1 -1

@@ -450,7 +451,7 @@ void *swiotlb_tbl_map_single(struct device *hwdev, dma_addr_t tbl_dma_addr,
 				io_tlb_list[i] = 0;
 			for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE - 1) && io_tlb_list[i]; i--)
 				io_tlb_list[i] = ++count;
-			dma_addr = io_tlb_start + (index << IO_TLB_SHIFT);
+			dma_addr = (char *)phys_to_virt(io_tlb_start) + (index << IO_TLB_SHIFT);

I think this is going to fall flat with the other user of swiotlb_tbl_map_single - Xen SWIOTLB. When it allocates the io_tlb_start and does it magic to make sure its under 4GB - the io_tlb_start swath of memory, ends up consisting of 2MB chunks of contingous spaces. But each chunk is not linearly in the DMA space (thought it is in the CPU space).

Meaning the io_tlb_start region 0-2MB can fall within the DMA address space of 2048MB->2032MB, and io_tlb_start offset 2MB->4MB, can fall within 1024MB->1026MB, and so on (depending on the availability of memory under 4GB).

There is a clear virt_to_phys(x) != virt_to_dma(x).

Quoted 12 lines

lib/swiotlb.c void *swiotlb_tbl_map_single(struct device *hwdev, dma_addr_t tbl_dma_addr, · line 454

Quoted 6 lines

lib/swiotlb.c +1 -1

@@ -450,7 +451,7 @@ void *swiotlb_tbl_map_single(struct device *hwdev, dma_addr_t tbl_dma_addr,
 				io_tlb_list[i] = 0;
 			for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE - 1) && io_tlb_list[i]; i--)
 				io_tlb_list[i] = ++count;
-			dma_addr = io_tlb_start + (index << IO_TLB_SHIFT);
+			dma_addr = (char *)phys_to_virt(io_tlb_start) + (index << IO_TLB_SHIFT);

I think this is going to fall flat with the other user of swiotlb_tbl_map_single - Xen SWIOTLB. When it allocates the io_tlb_start and does it magic to make sure its under 4GB - the io_tlb_start swath of memory, ends up consisting of 2MB chunks of contingous spaces. But each chunk is not linearly in the DMA space (thought it is in the CPU space).

Quoted 5 lines

Meaning the io_tlb_start region 0-2MB can fall within the DMA address space of 2048MB->2032MB, and io_tlb_start offset 2MB->4MB, can fall within 1024MB->1026MB, and so on (depending on the availability of memory under 4GB).

There is a clear virt_to_phys(x) != virt_to_dma(x).

Just to be sure I understand you are talking about DMA address space, not physical address space correct? I am fully aware that DMA address space can be all over the place. When I was writing the patch set the big reason why I decided to stop at physical address space was because DMA address spaces are device specific.

I understand that virt_to_phys(x) != virt_to_dma(x) for many platforms, however that is not my assertion. My assertion is (virt_to_phys(x) + y) == virt_to_phys(x + y). This should be true for any large block of contiguous memory that is DMA accessible since the CPU and the device should be able to view the memory in the same layout. If that wasn't

true I don't think is_swiotlb_buffer would be working correctly since it is essentially operating on the same assumption prior to my patches.

If you take a look at patches 4 and 5 I do address changes that end up needing to be made to Xen SWIOTLB since it makes use of swiotlb_tbl_map_single. All that I effectively end up changing is that instead of messing with a void pointer we instead are dealing with a physical address, and instead of calling xen_virt_to_bus we end up calling xen_phys_to_bus and thereby drop one extra virt_to_phys call in the process.

Thanks,

Alex

Quoted 23 lines

Quoted 12 lines

lib/swiotlb.c void *swiotlb_tbl_map_single(struct device *hwdev, dma_addr_t tbl_dma_addr, · line 454

Quoted 6 lines

lib/swiotlb.c +1 -1

@@ -450,7 +451,7 @@ void *swiotlb_tbl_map_single(struct device *hwdev, dma_addr_t tbl_dma_addr,
 				io_tlb_list[i] = 0;
 			for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE - 1) && io_tlb_list[i]; i--)
 				io_tlb_list[i] = ++count;
-			dma_addr = io_tlb_start + (index << IO_TLB_SHIFT);
+			dma_addr = (char *)phys_to_virt(io_tlb_start) + (index << IO_TLB_SHIFT);

I think this is going to fall flat with the other user of swiotlb_tbl_map_single - Xen SWIOTLB. When it allocates the io_tlb_start and does it magic to make sure its under 4GB - the io_tlb_start swath of memory, ends up consisting of 2MB chunks of contingous spaces. But each chunk is not linearly in the DMA space (thought it is in the CPU space).

Quoted 5 lines

Meaning the io_tlb_start region 0-2MB can fall within the DMA address space of 2048MB->2032MB, and io_tlb_start offset 2MB->4MB, can fall within 1024MB->1026MB, and so on (depending on the availability of memory under 4GB).

There is a clear virt_to_phys(x) != virt_to_dma(x).

Just to be sure I understand you are talking about DMA address space, not physical address space correct? I am fully aware that DMA address space can be all over the place. When I was writing the patch set the big reason why I decided to stop at physical address space was because DMA address spaces are device specific.

Quoted 5 lines

I understand that virt_to_phys(x) != virt_to_dma(x) for many platforms, however that is not my assertion. My assertion is (virt_to_phys(x) + y) == virt_to_phys(x + y). This should be true for any large block of contiguous memory that is DMA accessible since the CPU and the device should be able to view the memory in the same layout. If that wasn't

That is true mostly for x86 but not all platforms do this.

Quoted 2 lines

true I don't think is_swiotlb_buffer would be working correctly since it is essentially operating on the same assumption prior to my patches.

There are two pieces here - the is_swiotlb_buffer and the swiotlb_tbl_[map|unmap] functions.

The is_swiotlb_buffer is operating on that principle (and your change to reflect that is OK). The swiotlb_tbl_[*] is not.

Quoted 7 lines

If you take a look at patches 4 and 5 I do address changes that end up needing to be made to Xen SWIOTLB since it makes use of swiotlb_tbl_map_single. All that I effectively end up changing is that instead of messing with a void pointer we instead are dealing with a physical address, and instead of calling xen_virt_to_bus we end up calling xen_phys_to_bus and thereby drop one extra virt_to_phys call in the process.

Sure that is OK. All of those changes when we bypass the bounce buffer look OK (thought I should double-check again the patch to make sure and also just take it for a little test spin).

The issue is when we do _use_ the bounce buffer. At that point we run into the allocation from the bounce buffer where the patches assume that the 64MB swath of bounce buffer memory is bus (or DMA) memory contingous. And that is not the case sadly.

Quoted 44 lines · Konrad Rzeszutek Wilk

Quoted 23 lines

Quoted 12 lines

lib/swiotlb.c void *swiotlb_tbl_map_single(struct device *hwdev, dma_addr_t tbl_dma_addr, · line 454

Quoted 6 lines

lib/swiotlb.c +1 -1

@@ -450,7 +451,7 @@ void *swiotlb_tbl_map_single(struct device *hwdev, dma_addr_t tbl_dma_addr,
 				io_tlb_list[i] = 0;
 			for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE - 1) && io_tlb_list[i]; i--)
 				io_tlb_list[i] = ++count;
-			dma_addr = io_tlb_start + (index << IO_TLB_SHIFT);
+			dma_addr = (char *)phys_to_virt(io_tlb_start) + (index << IO_TLB_SHIFT);

I think this is going to fall flat with the other user of swiotlb_tbl_map_single - Xen SWIOTLB. When it allocates the io_tlb_start and does it magic to make sure its under 4GB - the io_tlb_start swath of memory, ends up consisting of 2MB chunks of contingous spaces. But each chunk is not linearly in the DMA space (thought it is in the CPU space).

Quoted 5 lines

Meaning the io_tlb_start region 0-2MB can fall within the DMA address space of 2048MB->2032MB, and io_tlb_start offset 2MB->4MB, can fall within 1024MB->1026MB, and so on (depending on the availability of memory under 4GB).

There is a clear virt_to_phys(x) != virt_to_dma(x).

Just to be sure I understand you are talking about DMA address space, not physical address space correct? I am fully aware that DMA address space can be all over the place. When I was writing the patch set the big reason why I decided to stop at physical address space was because DMA address spaces are device specific.

Quoted 5 lines

I understand that virt_to_phys(x) != virt_to_dma(x) for many platforms, however that is not my assertion. My assertion is (virt_to_phys(x) + y) == virt_to_phys(x + y). This should be true for any large block of contiguous memory that is DMA accessible since the CPU and the device should be able to view the memory in the same layout. If that wasn't

That is true mostly for x86 but not all platforms do this.

Quoted 2 lines

true I don't think is_swiotlb_buffer would be working correctly since it is essentially operating on the same assumption prior to my patches.

There are two pieces here - the is_swiotlb_buffer and the swiotlb_tbl_[map|unmap] functions.

The is_swiotlb_buffer is operating on that principle (and your change to reflect that is OK). The swiotlb_tbl_[*] is not.

Quoted 7 lines

If you take a look at patches 4 and 5 I do address changes that end up needing to be made to Xen SWIOTLB since it makes use of swiotlb_tbl_map_single. All that I effectively end up changing is that instead of messing with a void pointer we instead are dealing with a physical address, and instead of calling xen_virt_to_bus we end up calling xen_phys_to_bus and thereby drop one extra virt_to_phys call in the process.

Quoted 3 lines

Sure that is OK. All of those changes when we bypass the bounce buffer look OK (thought I should double-check again the patch to make sure and also just take it for a little test spin).

I'm interesting in finding out what the results of your test spin are.

Quoted 5 lines

The issue is when we do _use_ the bounce buffer. At that point we run into the allocation from the bounce buffer where the patches assume that the 64MB swath of bounce buffer memory is bus (or DMA) memory contingous. And that is not the case sadly.

I think I understand what you are saying now. However, I don't think the issue applies to my patches.

If I am not mistaken what you are talking about is the pseudo-physical memory versus machine memory. I understand the 64MB block is not machine-memory contiguous, but it should be pseudo-physical contiguous memory. As such using the pseudo-physical addresses instead of virtual addresses should function the same way as using true physical addresses to replace virtual addresses. All of the physical memory translation to machine memory translation is happening in xen_phys_to_bus and all of the changes I have made take place before that so the bounce buffers should still be working correctly. In addition none of the changes I

have made change the bounce buffer boundary assumptions so we should have no bounce buffers mapped across the 2MB boundaries.

Thanks,

Alex

Quoted 45 lines · Alexander Duyck

Quoted 44 lines · Konrad Rzeszutek Wilk

Quoted 23 lines

Quoted 12 lines

lib/swiotlb.c void *swiotlb_tbl_map_single(struct device *hwdev, dma_addr_t tbl_dma_addr, · line 454

Quoted 6 lines

lib/swiotlb.c +1 -1

@@ -450,7 +451,7 @@ void *swiotlb_tbl_map_single(struct device *hwdev, dma_addr_t tbl_dma_addr,
 				io_tlb_list[i] = 0;
 			for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE - 1) && io_tlb_list[i]; i--)
 				io_tlb_list[i] = ++count;
-			dma_addr = io_tlb_start + (index << IO_TLB_SHIFT);
+			dma_addr = (char *)phys_to_virt(io_tlb_start) + (index << IO_TLB_SHIFT);

I think this is going to fall flat with the other user of swiotlb_tbl_map_single - Xen SWIOTLB. When it allocates the io_tlb_start and does it magic to make sure its under 4GB - the io_tlb_start swath of memory, ends up consisting of 2MB chunks of contingous spaces. But each chunk is not linearly in the DMA space (thought it is in the CPU space).

Quoted 5 lines

Meaning the io_tlb_start region 0-2MB can fall within the DMA address space of 2048MB->2032MB, and io_tlb_start offset 2MB->4MB, can fall within 1024MB->1026MB, and so on (depending on the availability of memory under 4GB).

There is a clear virt_to_phys(x) != virt_to_dma(x).

Just to be sure I understand you are talking about DMA address space, not physical address space correct? I am fully aware that DMA address space can be all over the place. When I was writing the patch set the big reason why I decided to stop at physical address space was because DMA address spaces are device specific.

Quoted 5 lines

I understand that virt_to_phys(x) != virt_to_dma(x) for many platforms, however that is not my assertion. My assertion is (virt_to_phys(x) + y) == virt_to_phys(x + y). This should be true for any large block of contiguous memory that is DMA accessible since the CPU and the device should be able to view the memory in the same layout. If that wasn't

That is true mostly for x86 but not all platforms do this.

Quoted 2 lines

true I don't think is_swiotlb_buffer would be working correctly since it is essentially operating on the same assumption prior to my patches.

There are two pieces here - the is_swiotlb_buffer and the swiotlb_tbl_[map|unmap] functions.

The is_swiotlb_buffer is operating on that principle (and your change to reflect that is OK). The swiotlb_tbl_[*] is not.

Quoted 7 lines

If you take a look at patches 4 and 5 I do address changes that end up needing to be made to Xen SWIOTLB since it makes use of swiotlb_tbl_map_single. All that I effectively end up changing is that instead of messing with a void pointer we instead are dealing with a physical address, and instead of calling xen_virt_to_bus we end up calling xen_phys_to_bus and thereby drop one extra virt_to_phys call in the process.

Quoted 4 lines

Quoted 3 lines

Sure that is OK. All of those changes when we bypass the bounce buffer look OK (thought I should double-check again the patch to make sure and also just take it for a little test spin).

I'm interesting in finding out what the results of your test spin are.

Haven't gotten to that yet :-(

Quoted 6 lines

Quoted 4 lines

The issue is when we do _use_ the bounce buffer. At that point we run into the allocation from the bounce buffer where the patches assume that the 64MB swath of bounce buffer memory is bus (or DMA) memory contingous. And that is not the case sadly.

I think I understand what you are saying now. However, I don't think the issue applies to my patches.

Great.

Quoted 9 lines

If I am not mistaken what you are talking about is the pseudo-physical memory versus machine memory. I understand the 64MB block is not machine-memory contiguous, but it should be pseudo-physical contiguous memory. As such using the pseudo-physical addresses instead of virtual addresses should function the same way as using true physical addresses to replace virtual addresses. All of the physical memory translation to machine memory translation is happening in xen_phys_to_bus and all of the changes I have made take place before that so the bounce buffers should still be working correctly. In addition none of the changes I

OK.

While working on 10Gb/s routing performance I found a significant amount of time was being spent in the swiotlb DMA handler. Further digging found that a significant amount of this was due to the fact that virtual to physical address translation and calling the function that did it. It accounted for nearly 60% of the total overhead.

This patch set works to resolve that by changing the io_tlb_start address and io_tlb_overflow_buffer address from virtual addresses to physical addresses. By doing this, devices that are not making use of bounce buffers can significantly reduce their overhead. In addition I followed through with the

cleanup to the point that the only functions that really require the virtual address for the dma buffer are the init, free, and bounce functions.

When running a routing throughput test using small packets I saw roughly a 5% increase in packets rates after applying these patches. This appears to match up with the CPU overhead reduction I was tracking via perf.

Before:
Results 10.29Mps
# Overhead Symbol

# ........ ...........................................................................................................
#
     1.97%  [k] __phys_addr                                                                                   
            |              
            |--24.97%-- swiotlb_sync_single
            |          
            |--16.55%-- is_swiotlb_buffer
            |          
            |--11.25%-- unmap_single
            |          
             --2.71%-- swiotlb_dma_mapping_error
     1.66%  [k] swiotlb_sync_single                                                                                 
     1.45%  [k] is_swiotlb_buffer                                     
     0.53%  [k] unmap_single                                                                                     
     0.52%  [k] swiotlb_map_page                                      
     0.47%  [k] swiotlb_sync_single_for_device                                                                 
     0.43%  [k] swiotlb_sync_single_for_cpu                           
     0.42%  [k] swiotlb_dma_mapping_error                                                               
     0.34%  [k] swiotlb_unmap_page                

After:
Results 10.99Mps
# Overhead Symbol

# ........ ...........................................................................................................
#
     0.50%  [k] swiotlb_map_page                                                                                        
     0.50%  [k] swiotlb_sync_single                                                                                     
     0.36%  [k] swiotlb_sync_single_for_cpu                                                                             
     0.35%  [k] swiotlb_sync_single_for_device                                                                          
     0.25%  [k] swiotlb_unmap_page                                                                                      
     0.17%  [k] swiotlb_dma_mapping_error  

---

Alexander Duyck (7):
swiotlb: Do not export swiotlb_bounce since there are no external consumers
swiotlb: Use physical addresses instead of virtual in swiotlb_tbl_sync_single
swiotlb: Use physical addresses for swiotlb_tbl_unmap_single
swiotlb: Return physical addresses when calling swiotlb_tbl_map_single
swiotlb: Make io_tlb_overflow_buffer a physical address
swiotlb: Make io_tlb_start a physical address instead of a virtual address
swiotlb: Instead of tracking the end of the swiotlb region just calculate it

 drivers/xen/swiotlb-xen.c |   25 ++---
 include/linux/swiotlb.h   |   20 ++--
 lib/swiotlb.c             |  247 +++++++++++++++++++++++----------------------
 3 files changed, 150 insertions(+), 142 deletions(-)

Signature · 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 6 lines · Alexander Duyck

While working on 10Gb/s routing performance I found a significant amount of time was being spent in the swiotlb DMA handler. Further digging found that a significant amount of this was due to the fact that virtual to physical address translation and calling the function that did it. It accounted for nearly 60% of the total overhead.

Quoted 4 lines

This patch set works to resolve that by changing the io_tlb_start address and io_tlb_overflow_buffer address from virtual addresses to physical addresses. By doing this, devices that are not making use of bounce buffers can significantly reduce their overhead. In addition I followed through with the

.. but are still using SWIOTLB for their DMA operations, right?

While working on 10Gb/s routing performance I found a significant amount of time was being spent in the swiotlb DMA handler. Further digging found that a significant amount of this was due to the fact that virtual to physical address translation and calling the function that did it. It accounted for nearly 60% of the total overhead.

This patch set works to resolve that by changing the io_tlb_start address and io_tlb_overflow_buffer address from virtual addresses to physical addresses.

Quoted 6 lines · Alexander Duyck

While working on 10Gb/s routing performance I found a significant amount of time was being spent in the swiotlb DMA handler. Further digging found that a significant amount of this was due to the fact that virtual to physical address translation and calling the function that did it. It accounted for nearly 60% of the total overhead.

Quoted 2 lines

This patch set works to resolve that by changing the io_tlb_start address and io_tlb_overflow_buffer address from virtual addresses to physical addresses.

The assertion in your patches is that the DMA addresses (bus address) are linear is not applicable (unfortunatly). Meaning virt_to_phys() != virt_to_dma().

Now, on x86 and ia64 it is true - but one of the users of swiotlb library is the Xen swiotlb - which cannot guarantee that the physical address are 1-1 with the bus addresses. Hence the bulk of dealing with figuring out the right physical to bus address is done in Xen-SWIOTLB and the basics of finding an entry in the bounce buffer (if needed), mapping it, unmapping ,etc is carried out by the generic library.

This is sad - b/c your patches are a good move forward.

Perhaps another way to do this is by having your patches split the lookups in "chunks". Wherein we validate in swiotlb_init_*tbl that the 'tbl' (so the bounce buffer) is linear - if not, we split it up in chunks. Perhaps the various backends can be responsible for this since they would know which of their memory regions are linear or not. But that sounds complicated and we don't want to complicate this library.

Or another way would be to provide 'phys_to_virt' and 'virt_to_phys' functions for the swiotlb_tbl_{map|unmap}_single and the main library (lib/swiotlb.c) can decide to use them. If they are NULL, then it would do what your patches suggested. If they are defined, then carry out those lookups on the 'empty-to-be-used' bounce buffer address. Hm, that sounds like a better way of doing it.

While working on 10Gb/s routing performance I found a significant amount of time was being spent in the swiotlb DMA handler. Further digging found that a significant amount of this was due to the fact that virtual to physical address translation and calling the function that did it. It accounted for nearly 60% of the total overhead.

Quoted 5 lines · Alexander Duyck

While working on 10Gb/s routing performance I found a significant amount of time was being spent in the swiotlb DMA handler. Further digging found that a significant amount of this was due to the fact that virtual to physical address translation and calling the function that did it. It accounted for nearly 60% of the total overhead.

Can you find out why that is? Traditionally virt_to_phys was just a subtraction. Then later on it was a if and a subtraction.

It cannot really be that expensive. Do you have some debugging enabled?

Really virt_to_phys should be fixed. Such fundamental operations shouldn't slow. I don't think hacking up all the users to work around this is the r ight way.

Looking at the code a bit someone (crazy) made it out of line. But that cannot explain that much overhead.

-Andi

I was thinking the issue was all of the calls to relatively small functions occurring in quick succession. The way most of this code is setup it seems like it is one small function call in turn calling another, and then another, and I would imagine the code fragmentation can have a significant negative impact.

Quoted 5 lines

I was thinking the issue was all of the calls to relatively small functions occurring in quick succession. The way most of this code is setup it seems like it is one small function call in turn calling another, and then another, and I would imagine the code fragmentation can have a significant negative impact.

Maybe. Can you just inline everything and see if it it's faster then?

This was out of line when the "text cost at all costs" drive was still envogue, but luckily we're not doing that anymore.

-Andiu

Inlining everything did speed things up a bit, but I still didn't reach the same speed I achieved using the patch set. However I did notice the resulting swiotlb code was considerably larger.

assembly, is replaced with 8 lines of assembly and becomes inline. In addition we drop the number of calls to __phys_addr from 9 to 2 by dropping them all from swiotlb. By my math I am probably saving about 120 instructions per packet. I suspect all of that would probably be cutting the number of instructions per packet enough to probably account for a 5% difference when you consider I am running at about 1.5Mpps per core on a 2.7Ghz processor.

Maybe it's just me, but that's somehow sad for one if() and a su btraction

BTW __pa used to be a simple subtraction, the if () was just added to handle the few call sites for x86-64 that do __pa(&text_symbol). Maybe we should just go back to the old __pa_symbol() for those cases, then __pa could be the simple subtraction it used to was again and it could be inlined and everyone would be happy.

Quoted 3 lines

Inlining everything did speed things up a bit, but I still didn't reach the same speed I achieved using the patch set. However I did notice the resulting swiotlb code was considerably larger.

Thanks. So your patch makes sense, but imho should pursue the inlining in parallel for other call sites.

Quoted 7 lines

assembly, is replaced with 8 lines of assembly and becomes inline. In addition we drop the number of calls to __phys_addr from 9 to 2 by dropping them all from swiotlb. By my math I am probably saving about 120 instructions per packet. I suspect all of that would probably be cutting the number of instructions per packet enough to probably account for a 5% difference when you consider I am running at about 1.5Mpps per core on a 2.7Ghz processor.

Maybe it's just me, but that's somehow sad for one if() and a subtraction

BTW __pa used to be a simple subtraction, the if () was just added to handle the few call sites for x86-64 that do __pa(&text_symbol). Maybe we should just go back to the old __pa_symbol() for those cases, then __pa could be the simple subtraction it used to was again and it could be inlined and everyone would be happy.

-Andi

Quoted 7 lines · Andi Kleen

BTW __pa used to be a simple subtraction, the if () was just added to handle the few call sites for x86-64 that do __pa(&text_symbol). Maybe we should just go back to the old __pa_symbol() for those cases, then __pa could be the simple subtraction it used to was again and it could be inlined and everyone would be happy.

-Andi

What I am probably looking at doing is splitting the function in two as you suggest where we have a separate function for the text symbol case. I will probably also take the 32 bit approach and add a debug version that is still a separate function for uses such as determining if we have any callers who should be using __pa_symbol instead of __pa.

Thanks,

Alex