iommu-iotlb-flushing

The following patch is for batching up the flushing of the IOTLB for the DMAR implementation found in the Intel VT-d hardware. It works by building a list of to be flushed IOTLB entries and a bitmap list of which DMAR engine they are from.

After either a high water mark (250 accessible via debugfs) or 10ms the list of iova's will be reclaimed and the DMAR engines associated are IOTLB-flushed.

This approach recovers 15 to 20% of the performance lost when using the IOMMU for my netperf udp stream benchmark with small packets. It can be disabled with a kernel boot parameter
"intel_iommu=strict".

Its use does weaken the IOMMU protections a bit.

I would like to see this go into MM for a while and then onto mainline.

...

drivers/pci/intel-iommu.c +16 -0

+static struct intel_iommu *g_iommus;
+/* bitmap for indexing intel_iommus */
+static unsigned long 	*g_iommus_to_flush;
+static int g_num_of_iommus;
+
+static DEFINE_SPINLOCK(async_umap_flush_lock);
+static LIST_HEAD(unmaps_to_do);
+
+static int timer_on;
+static long list_size;
+static int high_watermark;
+
+static struct dentry *intel_iommu_debug, *debug;
+
+
 static void domain_remove_dev_info(struct dmar_domain *domain);
 
 static int dmar_disabled;
 static int __initdata dmar_map_gfx = 1;
 static int dmar_forcedac;
+static int intel_iommu_strict;
 
 #define DUMMY_DEVICE_DOMAIN_INFO ((struct device_domain_info *)(-1))
 static DEFINE_SPINLOCK(device_domain_lock);

drivers/pci/intel-iommu.c +7 -8

+			printk(KERN_INFO
+				"Intel-IOMMU: disable batched IOTLB flush\n");
+			intel_iommu_strict = 1;
 		}
 
 		str += strcspn(str, ",");

@@ -965,17 +999,13 @@
 		set_bit(0, iommu->domain_ids);
 	return 0;
 }
-
-static struct intel_iommu *alloc_iommu(struct dmar_drhd_unit *drhd)
+static struct intel_iommu *alloc_iommu(struct intel_iommu *iommu,
+					struct dmar_drhd_unit *drhd)
 {
-	struct intel_iommu *iommu;
 	int ret;
 	int map_size;
 	u32 ver;
 
-	iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
-	if (!iommu)
-		return NULL;
 	iommu->reg = ioremap(drhd->reg_base_addr, PAGE_SIZE_4K);
 	if (!iommu->reg) {
 		printk(KERN_ERR "IOMMU: can't map the region\n");

@@ -1396,7 +1426,7 @@
 	int index;
 
 	while (dev) {
-		for (index = 0; index < cnt; index ++)
+		for (index = 0; index < cnt; index++)
 			if (dev == devices[index])
 				return 1;
 

@@ -1661,7 +1691,7 @@
 	struct dmar_rmrr_unit *rmrr;
 	struct pci_dev *pdev;
 	struct intel_iommu *iommu;
-	int ret, unit = 0;
+	int nlongs, i, ret, unit = 0;
 
 	/*
 	 * for each drhd

drivers/pci/intel-iommu.c +15 -1

+	}
+
+	g_iommus = kzalloc(g_num_of_iommus * sizeof(*iommu), GFP_KERNEL);
+	if (!g_iommus) {
+		kfree(g_iommus_to_flush);
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	i = 0;
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+		iommu = alloc_iommu(&g_iommus[i], drhd);
+		i++;
 		if (!iommu) {
 			ret = -ENOMEM;
 			goto error;

@@ -1705,7 +1758,6 @@
 	 * endfor
 	 */
 	for_each_rmrr_units(rmrr) {
-		int i;
 		for (i = 0; i < rmrr->devices_cnt; i++) {
 			pdev = rmrr->devices[i];
 			/* some BIOS lists non-exist devices in DMAR table */

@@ -1909,6 +1961,54 @@
 	return 0;
 }
 

drivers/pci/intel-iommu.c +15 -0

+	for (i = 0; i < g_num_of_iommus; i++) {
+		if (test_and_clear_bit(i, g_iommus_to_flush))
+			iommu_flush_iotlb_global(&g_iommus[i], 0);
+	}
+
+	list_for_each_entry_safe(node, n, &unmaps_to_do, list) {
+		/* free iova */
+		list_del(&node->list);
+		__free_iova(&((struct dmar_domain *)node->dmar)->iovad, node);
+
+	}
+	list_size = 0;
+	spin_unlock_irqrestore(&async_umap_flush_lock, flags);
+}
+

drivers/pci/intel-iommu.c +25 -7

+		timer_on = 1;
+	}
+	list_size++;
+	spin_unlock_irqrestore(&async_umap_flush_lock, flags);
+}
+
 static void intel_unmap_single(struct device *dev, dma_addr_t dev_addr,
 	size_t size, int dir)
 {

@@ -1936,13 +2036,21 @@
 	dma_pte_clear_range(domain, start_addr, start_addr + size);
 	/* free page tables */
 	dma_pte_free_pagetable(domain, start_addr, start_addr + size);
-
-	if (iommu_flush_iotlb_psi(domain->iommu, domain->id, start_addr,
-			size >> PAGE_SHIFT_4K, 0))
-		iommu_flush_write_buffer(domain->iommu);
-
-	/* free iova */
-	__free_iova(&domain->iovad, iova);
+	if (intel_iommu_strict) {
+		if (iommu_flush_iotlb_psi(domain->iommu,
+			domain->id, start_addr, size >> PAGE_SHIFT_4K, 0))
+			iommu_flush_write_buffer(domain->iommu);
+		/* free iova */
+		__free_iova(&domain->iovad, iova);
+	} else {
+		add_unmap(domain, iova);
+		/*
+		 * queue up the release of the unmap to save the 1/6th of the
+		 * cpu used up by the iotlb flush operation...
+		 */
+		if (list_size > high_watermark)
+			flush_unmaps();
+	}
 }
 
 static void * intel_alloc_coherent(struct device *hwdev, size_t size,

@@ -2266,6 +2374,10 @@
 	if (dmar_table_init())
 		return 	-ENODEV;
 
+	high_watermark = 250;
+	intel_iommu_debug = debugfs_create_dir("intel_iommu", NULL);
+	debug = debugfs_create_u32("high_watermark", S_IWUGO | S_IRUGO,
+					intel_iommu_debug, &high_watermark);
 	iommu_init_mempool();
 	dmar_init_reserved_ranges();
 

drivers/pci/intel-iommu.c +0 -0

 	force_iommu = 1;
 	dma_ops = &intel_dma_ops;
 	return 0;

Index: linux-2.6.24-mm1/drivers/pci/iova.h
===================================================================

drivers/pci/iova.h +2 -0

@@ -23,6 +23,8 @@
 	struct rb_node	node;
 	unsigned long	pfn_hi; /* IOMMU dish out addr hi */
 	unsigned long	pfn_lo; /* IOMMU dish out addr lo */
+	struct list_head list;
+	void *dmar;
 };
 
 /* holds all the iova translations for a domain */

Index: linux-2.6.24-mm1/Documentation/kernel-parameters.txt
===================================================================

Quoted 16 lines · On Wed, 20 Feb 2008 16:06:23 -0800 mark gross <[email protected]> wrote:

The following patch is for batching up the flushing of the IOTLB for the DMAR implementation found in the Intel VT-d hardware. It works by building a list of to be flushed IOTLB entries and a bitmap list of which DMAR engine they are from.

After either a high water mark (250 accessible via debugfs) or 10ms the list of iova's will be reclaimed and the DMAR engines associated are IOTLB-flushed.

This approach recovers 15 to 20% of the performance lost when using the IOMMU for my netperf udp stream benchmark with small packets. It can be disabled with a kernel boot parameter
"intel_iommu=strict".

Its use does weaken the IOMMU protections a bit.

drivers/pci/intel-iommu.c line 59

Quoted 6 lines

I would like to see this go into MM for a while and then onto mainline.

...

drivers/pci/intel-iommu.c +2 -0

+static struct timer_list unmap_timer =
+	TIMER_INITIALIZER(flush_unmaps_timeout, 0, 0);

Could use DEFINE_TIMER here.

drivers/pci/intel-iommu.c line 65

Quoted 6 lines

drivers/pci/intel-iommu.c +5 -0

+struct unmap_list {
+	struct list_head list;
+	struct dmar_domain *domain;
+	struct iova *iova;
+};

unmap_list doens't seem to be used anywhere?

Quoted 23 lines

drivers/pci/intel-iommu.c +16 -0

+static struct intel_iommu *g_iommus;
+/* bitmap for indexing intel_iommus */
+static unsigned long 	*g_iommus_to_flush;
+static int g_num_of_iommus;
+
+static DEFINE_SPINLOCK(async_umap_flush_lock);
+static LIST_HEAD(unmaps_to_do);
+
+static int timer_on;
+static long list_size;
+static int high_watermark;
+
+static struct dentry *intel_iommu_debug, *debug;
+
+
 static void domain_remove_dev_info(struct dmar_domain *domain);
 
 static int dmar_disabled;
 static int __initdata dmar_map_gfx = 1;
 static int dmar_forcedac;
+static int intel_iommu_strict;

drivers/pci/intel-iommu.c line 109

Quoted 11 lines

#define DUMMY_DEVICE_DOMAIN_INFO ((struct device_domain_info *)(-1)) static DEFINE_SPINLOCK(device_domain_lock);

drivers/pci/intel-iommu.c +2 -1

@@ -73,9 +103,13 @@
 			printk(KERN_INFO
 				"Intel-IOMMU: disable GFX device mapping\n");
 		} else if (!strncmp(str, "forcedac", 8)) {
-			printk (KERN_INFO
+			printk(KERN_INFO
 				"Intel-IOMMU: Forcing DAC for PCI devices\n");
 			dmar_forcedac = 1;
+		} else if (!strncmp(str, "strict", 8)) {

s/8/6/

Quoted 43 lines

drivers/pci/intel-iommu.c +7 -8

+			printk(KERN_INFO
+				"Intel-IOMMU: disable batched IOTLB flush\n");
+			intel_iommu_strict = 1;
 		}
 
 		str += strcspn(str, ",");

@@ -965,17 +999,13 @@
 		set_bit(0, iommu->domain_ids);
 	return 0;
 }
-
-static struct intel_iommu *alloc_iommu(struct dmar_drhd_unit *drhd)
+static struct intel_iommu *alloc_iommu(struct intel_iommu *iommu,
+					struct dmar_drhd_unit *drhd)
 {
-	struct intel_iommu *iommu;
 	int ret;
 	int map_size;
 	u32 ver;
 
-	iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
-	if (!iommu)
-		return NULL;
 	iommu->reg = ioremap(drhd->reg_base_addr, PAGE_SIZE_4K);
 	if (!iommu->reg) {
 		printk(KERN_ERR "IOMMU: can't map the region\n");

@@ -1396,7 +1426,7 @@
 	int index;
 
 	while (dev) {
-		for (index = 0; index < cnt; index ++)
+		for (index = 0; index < cnt; index++)
 			if (dev == devices[index])
 				return 1;
 

@@ -1661,7 +1691,7 @@
 	struct dmar_rmrr_unit *rmrr;
 	struct pci_dev *pdev;
 	struct intel_iommu *iommu;
-	int ret, unit = 0;
+	int nlongs, i, ret, unit = 0;
 

drivers/pci/intel-iommu.c line 1705

Quoted 8 lines

drivers/pci/intel-iommu.c +1 -1

 	/*
 	 * for each drhd

@@ -1672,7 +1702,30 @@
 	for_each_drhd_unit(drhd) {
 		if (drhd->ignored)
 			continue;
-		iommu = alloc_iommu(drhd);
+		g_num_of_iommus++;

No locking needed for g_num_of_iommus?

drivers/pci/intel-iommu.c line 1708

Quoted 3 lines

drivers/pci/intel-iommu.c +3 -0

+	}
+
+	nlongs = BITS_TO_LONGS(g_num_of_iommus);

Would this code be neater if it used the <linux/bitmap.h> stuff?

drivers/pci/intel-iommu.c line 1713

Quoted 6 lines

drivers/pci/intel-iommu.c +5 -0

+	g_iommus_to_flush = kzalloc(nlongs * sizeof(unsigned long), GFP_KERNEL);
+	if (!g_iommus_to_flush) {
+		printk(KERN_ERR "Intel-IOMMU: "
+			"Allocating bitmap array failed\n");
+		return -ENOMEM;

Are you sure we aren't leaking anything here? Like the alloc_iommu() above?

Quoted 31 lines

drivers/pci/intel-iommu.c +15 -1

+	}
+
+	g_iommus = kzalloc(g_num_of_iommus * sizeof(*iommu), GFP_KERNEL);
+	if (!g_iommus) {
+		kfree(g_iommus_to_flush);
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	i = 0;
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+		iommu = alloc_iommu(&g_iommus[i], drhd);
+		i++;
 		if (!iommu) {
 			ret = -ENOMEM;
 			goto error;

@@ -1705,7 +1758,6 @@
 	 * endfor
 	 */
 	for_each_rmrr_units(rmrr) {
-		int i;
 		for (i = 0; i < rmrr->devices_cnt; i++) {
 			pdev = rmrr->devices[i];
 			/* some BIOS lists non-exist devices in DMAR table */

@@ -1909,6 +1961,54 @@
 	return 0;
 }
 

drivers/pci/intel-iommu.c line 1973

Quoted 10 lines

drivers/pci/intel-iommu.c +10 -0

+static void flush_unmaps(void)
+{
+	struct iova *node, *n;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&async_umap_flush_lock, flags);
+	timer_on = 0;
+
+	/*just flush them all*/

I'm surprised that checkpatch didn't grump about the odd commenting style.

Quoted 20 lines

drivers/pci/intel-iommu.c +19 -0

+	for (i = 0; i < g_num_of_iommus; i++) {
+		if (test_and_clear_bit(i, g_iommus_to_flush))
+			iommu_flush_iotlb_global(&g_iommus[i], 0);
+	}
+
+	list_for_each_entry_safe(node, n, &unmaps_to_do, list) {
+		/* free iova */
+		list_del(&node->list);
+		__free_iova(&((struct dmar_domain *)node->dmar)->iovad, node);
+
+	}
+	list_size = 0;
+	spin_unlock_irqrestore(&async_umap_flush_lock, flags);
+}
+
+static void flush_unmaps_timeout(unsigned long data)
+{
+	flush_unmaps();
+}

drivers/pci/intel-iommu.c line 1998

Quoted 6 lines

drivers/pci/intel-iommu.c +6 -0

+
+static void add_unmap(struct dmar_domain *dom, struct iova *iova)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&async_umap_flush_lock, flags);

How scalable is this?

drivers/pci/intel-iommu.c line 2005

Quoted 8 lines

drivers/pci/intel-iommu.c +7 -0

+	iova->dmar = dom;
+	list_add(&iova->list, &unmaps_to_do);
+	set_bit((dom->iommu - g_iommus), g_iommus_to_flush);
+
+	if (!timer_on) {
+		unmap_timer.expires = jiffies + msecs_to_jiffies(10);
+		mod_timer(&unmap_timer, unmap_timer.expires);

No, this modifies unmap_timer.expires twice. Might be racy too. You want

	mod_timer(&unmap_timer, jiffies + msecs_to_jiffies(10));

Quoted 50 lines

drivers/pci/intel-iommu.c +25 -7

+		timer_on = 1;
+	}
+	list_size++;
+	spin_unlock_irqrestore(&async_umap_flush_lock, flags);
+}
+
 static void intel_unmap_single(struct device *dev, dma_addr_t dev_addr,
 	size_t size, int dir)
 {

@@ -1936,13 +2036,21 @@
 	dma_pte_clear_range(domain, start_addr, start_addr + size);
 	/* free page tables */
 	dma_pte_free_pagetable(domain, start_addr, start_addr + size);
-
-	if (iommu_flush_iotlb_psi(domain->iommu, domain->id, start_addr,
-			size >> PAGE_SHIFT_4K, 0))
-		iommu_flush_write_buffer(domain->iommu);
-
-	/* free iova */
-	__free_iova(&domain->iovad, iova);
+	if (intel_iommu_strict) {
+		if (iommu_flush_iotlb_psi(domain->iommu,
+			domain->id, start_addr, size >> PAGE_SHIFT_4K, 0))
+			iommu_flush_write_buffer(domain->iommu);
+		/* free iova */
+		__free_iova(&domain->iovad, iova);
+	} else {
+		add_unmap(domain, iova);
+		/*
+		 * queue up the release of the unmap to save the 1/6th of the
+		 * cpu used up by the iotlb flush operation...
+		 */
+		if (list_size > high_watermark)
+			flush_unmaps();
+	}
 }
 
 static void * intel_alloc_coherent(struct device *hwdev, size_t size,

@@ -2266,6 +2374,10 @@
 	if (dmar_table_init())
 		return 	-ENODEV;
 
+	high_watermark = 250;
+	intel_iommu_debug = debugfs_create_dir("intel_iommu", NULL);
+	debug = debugfs_create_u32("high_watermark", S_IWUGO | S_IRUGO,
+					intel_iommu_debug, &high_watermark);
 	iommu_init_mempool();
 	dmar_init_reserved_ranges();
 

drivers/pci/intel-iommu.c line 2396

Quoted 5 lines

drivers/pci/intel-iommu.c +1 -0

@@ -2281,6 +2393,7 @@
 	printk(KERN_INFO
 	"PCI-DMA: Intel(R) Virtualization Technology for Directed I/O\n");
 
+	init_timer(&unmap_timer);

I see timers being added, but I see no del_timer_sync()s added on cleanup paths. Are you sure that we don't have races on various teardown paths?

Quoted 15 lines

drivers/pci/intel-iommu.c +0 -0

 	force_iommu = 1;
 	dma_ops = &intel_dma_ops;
 	return 0;

Index: linux-2.6.24-mm1/drivers/pci/iova.h
===================================================================

drivers/pci/iova.h +2 -0

@@ -23,6 +23,8 @@
 	struct rb_node	node;
 	unsigned long	pfn_hi; /* IOMMU dish out addr hi */
 	unsigned long	pfn_lo; /* IOMMU dish out addr lo */
+	struct list_head list;
+	void *dmar;
 };
 

Documentation/kernel-parameters.txt line 828

Quoted 13 lines

drivers/pci/iova.h +0 -0

 /* holds all the iova translations for a domain */

Index: linux-2.6.24-mm1/Documentation/kernel-parameters.txt
===================================================================

Documentation/kernel-parameters.txt +4 -0

@@ -822,6 +822,10 @@
 			than 32 bit addressing. The default is to look
 			for translation below 32 bit and if not available
 			then look in the higher range.
+		strict [Default Off]
+			With this option on every unmap_single operation will
+			result in a hardware IOTLB flush operation as opposed
+			to batching them for performance.

boot-time options suck. Is it not possible to tweak this at runtime?

Quoted 3 lines

drivers/pci/intel-iommu.c line 1973

Quoted 1 line

drivers/pci/intel-iommu.c +1 -0

+	/*just flush them all*/

I'm surprised that checkpatch didn't grump about the odd commenting style.

It didn't. What's odd about the comment style here?

Quoted 16 lines · On Wed, 20 Feb 2008 16:06:23 -0800 mark gross <[email protected]> wrote:

The following patch is for batching up the flushing of the IOTLB for the DMAR implementation found in the Intel VT-d hardware. It works by building a list of to be flushed IOTLB entries and a bitmap list of which DMAR engine they are from.

After either a high water mark (250 accessible via debugfs) or 10ms the list of iova's will be reclaimed and the DMAR engines associated are IOTLB-flushed.

This approach recovers 15 to 20% of the performance lost when using the IOMMU for my netperf udp stream benchmark with small packets. It can be disabled with a kernel boot parameter
"intel_iommu=strict".

Its use does weaken the IOMMU protections a bit.

Review

Quoted 6 lines

I would like to see this go into MM for a while and then onto mainline.

...

(quoted diff) +2 -0

+static struct timer_list unmap_timer =
+	TIMER_INITIALIZER(flush_unmaps_timeout, 0, 0);

Could use DEFINE_TIMER here.

Review

Quoted 6 lines

(quoted diff) +5 -0

+struct unmap_list {
+	struct list_head list;
+	struct dmar_domain *domain;
+	struct iova *iova;
+};

unmap_list doens't seem to be used anywhere?

Quoted 23 lines

(quoted diff) +16 -0

+static struct intel_iommu *g_iommus;
+/* bitmap for indexing intel_iommus */
+static unsigned long 	*g_iommus_to_flush;
+static int g_num_of_iommus;
+
+static DEFINE_SPINLOCK(async_umap_flush_lock);
+static LIST_HEAD(unmaps_to_do);
+
+static int timer_on;
+static long list_size;
+static int high_watermark;
+
+static struct dentry *intel_iommu_debug, *debug;
+
+
 static void domain_remove_dev_info(struct dmar_domain *domain);
 
 static int dmar_disabled;
 static int __initdata dmar_map_gfx = 1;
 static int dmar_forcedac;
+static int intel_iommu_strict;

Review line 109

Quoted 11 lines

#define DUMMY_DEVICE_DOMAIN_INFO ((struct device_domain_info *)(-1)) static DEFINE_SPINLOCK(device_domain_lock);

(quoted diff) +2 -1

@@ -73,9 +103,13 @@
 			printk(KERN_INFO
 				"Intel-IOMMU: disable GFX device mapping\n");
 		} else if (!strncmp(str, "forcedac", 8)) {
-			printk (KERN_INFO
+			printk(KERN_INFO
 				"Intel-IOMMU: Forcing DAC for PCI devices\n");
 			dmar_forcedac = 1;
+		} else if (!strncmp(str, "strict", 8)) {

s/8/6/

Quoted 43 lines

(quoted diff) +7 -8

+			printk(KERN_INFO
+				"Intel-IOMMU: disable batched IOTLB flush\n");
+			intel_iommu_strict = 1;
 		}
 
 		str += strcspn(str, ",");

@@ -965,17 +999,13 @@
 		set_bit(0, iommu->domain_ids);
 	return 0;
 }
-
-static struct intel_iommu *alloc_iommu(struct dmar_drhd_unit *drhd)
+static struct intel_iommu *alloc_iommu(struct intel_iommu *iommu,
+					struct dmar_drhd_unit *drhd)
 {
-	struct intel_iommu *iommu;
 	int ret;
 	int map_size;
 	u32 ver;
 
-	iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
-	if (!iommu)
-		return NULL;
 	iommu->reg = ioremap(drhd->reg_base_addr, PAGE_SIZE_4K);
 	if (!iommu->reg) {
 		printk(KERN_ERR "IOMMU: can't map the region\n");

@@ -1396,7 +1426,7 @@
 	int index;
 
 	while (dev) {
-		for (index = 0; index < cnt; index ++)
+		for (index = 0; index < cnt; index++)
 			if (dev == devices[index])
 				return 1;
 

@@ -1661,7 +1691,7 @@
 	struct dmar_rmrr_unit *rmrr;
 	struct pci_dev *pdev;
 	struct intel_iommu *iommu;
-	int ret, unit = 0;
+	int nlongs, i, ret, unit = 0;
 

Review line 1705

Quoted 8 lines

(quoted diff) +1 -1

 	/*
 	 * for each drhd

@@ -1672,7 +1702,30 @@
 	for_each_drhd_unit(drhd) {
 		if (drhd->ignored)
 			continue;
-		iommu = alloc_iommu(drhd);
+		g_num_of_iommus++;

No locking needed for g_num_of_iommus?

Review line 1708

Quoted 4 lines

(quoted diff) +3 -0

+	}
+
+	nlongs = BITS_TO_LONGS(g_num_of_iommus);

Would this code be neater if it used the <linux/bitmap.h> stuff?

Review line 1713

Quoted 5 lines

(quoted diff) +5 -0

+	g_iommus_to_flush = kzalloc(nlongs * sizeof(unsigned long), GFP_KERNEL);
+	if (!g_iommus_to_flush) {
+		printk(KERN_ERR "Intel-IOMMU: "
+			"Allocating bitmap array failed\n");
+		return -ENOMEM;

Are you sure we aren't leaking anything here? Like the alloc_iommu() above?

Quoted 30 lines

(quoted diff) +15 -1

+	}
+
+	g_iommus = kzalloc(g_num_of_iommus * sizeof(*iommu), GFP_KERNEL);
+	if (!g_iommus) {
+		kfree(g_iommus_to_flush);
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	i = 0;
+	for_each_drhd_unit(drhd) {
+		if (drhd->ignored)
+			continue;
+		iommu = alloc_iommu(&g_iommus[i], drhd);
+		i++;
 		if (!iommu) {
 			ret = -ENOMEM;
 			goto error;

@@ -1705,7 +1758,6 @@
 	 * endfor
 	 */
 	for_each_rmrr_units(rmrr) {
-		int i;
 		for (i = 0; i < rmrr->devices_cnt; i++) {
 			pdev = rmrr->devices[i];
 			/* some BIOS lists non-exist devices in DMAR table */

@@ -1909,6 +1961,54 @@
 	return 0;
 }
 

Review line 1973

Quoted 10 lines

(quoted diff) +10 -0

+static void flush_unmaps(void)
+{
+	struct iova *node, *n;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&async_umap_flush_lock, flags);
+	timer_on = 0;
+
+	/*just flush them all*/

I'm surprised that checkpatch didn't grump about the odd commenting style.

Quoted 19 lines

(quoted diff) +19 -0

+	for (i = 0; i < g_num_of_iommus; i++) {
+		if (test_and_clear_bit(i, g_iommus_to_flush))
+			iommu_flush_iotlb_global(&g_iommus[i], 0);
+	}
+
+	list_for_each_entry_safe(node, n, &unmaps_to_do, list) {
+		/* free iova */
+		list_del(&node->list);
+		__free_iova(&((struct dmar_domain *)node->dmar)->iovad, node);
+
+	}
+	list_size = 0;
+	spin_unlock_irqrestore(&async_umap_flush_lock, flags);
+}
+
+static void flush_unmaps_timeout(unsigned long data)
+{
+	flush_unmaps();
+}

Review line 1998

Quoted 6 lines

(quoted diff) +6 -0

+
+static void add_unmap(struct dmar_domain *dom, struct iova *iova)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&async_umap_flush_lock, flags);

How scalable is this?

Review line 2005

Quoted 8 lines

(quoted diff) +7 -0

+	iova->dmar = dom;
+	list_add(&iova->list, &unmaps_to_do);
+	set_bit((dom->iommu - g_iommus), g_iommus_to_flush);
+
+	if (!timer_on) {
+		unmap_timer.expires = jiffies + msecs_to_jiffies(10);
+		mod_timer(&unmap_timer, unmap_timer.expires);

No, this modifies unmap_timer.expires twice. Might be racy too. You want

	mod_timer(&unmap_timer, jiffies + msecs_to_jiffies(10));

Quoted 49 lines

(quoted diff) +25 -7

+		timer_on = 1;
+	}
+	list_size++;
+	spin_unlock_irqrestore(&async_umap_flush_lock, flags);
+}
+
 static void intel_unmap_single(struct device *dev, dma_addr_t dev_addr,
 	size_t size, int dir)
 {

@@ -1936,13 +2036,21 @@
 	dma_pte_clear_range(domain, start_addr, start_addr + size);
 	/* free page tables */
 	dma_pte_free_pagetable(domain, start_addr, start_addr + size);
-
-	if (iommu_flush_iotlb_psi(domain->iommu, domain->id, start_addr,
-			size >> PAGE_SHIFT_4K, 0))
-		iommu_flush_write_buffer(domain->iommu);
-
-	/* free iova */
-	__free_iova(&domain->iovad, iova);
+	if (intel_iommu_strict) {
+		if (iommu_flush_iotlb_psi(domain->iommu,
+			domain->id, start_addr, size >> PAGE_SHIFT_4K, 0))
+			iommu_flush_write_buffer(domain->iommu);
+		/* free iova */
+		__free_iova(&domain->iovad, iova);
+	} else {
+		add_unmap(domain, iova);
+		/*
+		 * queue up the release of the unmap to save the 1/6th of the
+		 * cpu used up by the iotlb flush operation...
+		 */
+		if (list_size > high_watermark)
+			flush_unmaps();
+	}
 }
 
 static void * intel_alloc_coherent(struct device *hwdev, size_t size,

@@ -2266,6 +2374,10 @@
 	if (dmar_table_init())
 		return 	-ENODEV;
 
+	high_watermark = 250;
+	intel_iommu_debug = debugfs_create_dir("intel_iommu", NULL);
+	debug = debugfs_create_u32("high_watermark", S_IWUGO | S_IRUGO,
+					intel_iommu_debug, &high_watermark);
 	iommu_init_mempool();
 	dmar_init_reserved_ranges();
 

Review line 2396

Quoted 5 lines

(quoted diff) +1 -0

@@ -2281,6 +2393,7 @@
 	printk(KERN_INFO
 	"PCI-DMA: Intel(R) Virtualization Technology for Directed I/O\n");
 
+	init_timer(&unmap_timer);

I see timers being added, but I see no del_timer_sync()s added on cleanup paths. Are you sure that we don't have races on various teardown paths?

Quoted 15 lines

(quoted diff) +0 -0

 	force_iommu = 1;
 	dma_ops = &intel_dma_ops;
 	return 0;

Index: linux-2.6.24-mm1/drivers/pci/iova.h
===================================================================

drivers/pci/iova.h +2 -0

@@ -23,6 +23,8 @@
 	struct rb_node	node;
 	unsigned long	pfn_hi; /* IOMMU dish out addr hi */
 	unsigned long	pfn_lo; /* IOMMU dish out addr lo */
+	struct list_head list;
+	void *dmar;
 };
 

Documentation/kernel-parameters.txt line 828

Quoted 13 lines

drivers/pci/iova.h +0 -0

 /* holds all the iova translations for a domain */

Index: linux-2.6.24-mm1/Documentation/kernel-parameters.txt
===================================================================

Documentation/kernel-parameters.txt +4 -0

@@ -822,6 +822,10 @@
 			than 32 bit addressing. The default is to look
 			for translation below 32 bit and if not available
 			then look in the higher range.
+		strict [Default Off]
+			With this option on every unmap_single operation will
+			result in a hardware IOTLB flush operation as opposed
+			to batching them for performance.

boot-time options suck. Is it not possible to tweak this at runtime?

Quoted 4 lines

Quoted 4 lines

Documentation/kernel-parameters.txt +0 -0

@@ -822,6 +822,10 @@
 			than 32 bit addressing. The default is to look

Quoted 8 lines

Documentation/kernel-parameters.txt line 836

Quoted 6 lines

Documentation/kernel-parameters.txt +4 -0

 			for translation below 32 bit and if not available
 			then look in the higher range.
+		strict [Default Off]
+			With this option on every unmap_single operation will
+			result in a hardware IOTLB flush operation as opposed
+			to batching them for performance.

boot-time options suck. Is it not possible to tweak this at runtime?

Yes, I could easily create a sysfs or debugfs mechanism for turning it on / off at run time. I would like input on the preferred way to do this. sysfs or debugfs?

Quoted 6 lines · Andrew Morton

Quoted 4 lines · On Wed, 20 Feb 2008 16:06:23 -0800 mark gross <[email protected]> wrote:

The following patch is for batching up the flushing of the IOTLB for the DMAR implementation found in the Intel VT-d hardware. It works by building a list of to be flushed IOTLB entries and a bitmap list of which DMAR engine they are from.

Quoted 6 lines

Quoted 6 lines

This approach recovers 15 to 20% of the performance lost when using the IOMMU for my netperf udp stream benchmark with small packets. It can be disabled with a kernel boot parameter
"intel_iommu=strict".

Its use does weaken the IOMMU protections a bit.

Quoted 1 line

drivers/pci/intel-iommu.c line 2037

Quoted 1 line

drivers/pci/intel-iommu.c +1 -0

+	if (intel_iommu_strict) {

Quoted 1 line

boot-time options suck. Is it not possible to tweak this at runtime?

Yes, I could easily create a sysfs or debugfs mechanism for turning it on / off at run time. I would like input on the preferred way to do this. sysfs or debugfs?

Signed-off-by <[email protected]>

Index: linux-2.6.25-rc2-mm1/drivers/pci/intel-iommu.c
===================================================================

drivers/pci/intel-iommu.c +0 -0

drivers/pci/intel-iommu.c +4 -0

+static void flush_unmaps(void)
+{
+	struct iova *node, *n;
+	unsigned long flags;

drivers/pci/intel-iommu.c +6 -0

+		__free_iova(&((struct dmar_domain *)node->dmar)->iovad, node);
+
+	}
+	list_size = 0;
+	spin_unlock_irqrestore(&async_umap_flush_lock, flags);
+}

drivers/pci/intel-iommu.c +6 -0

+static void add_unmap(struct dmar_domain *dom, struct iova *iova)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&async_umap_flush_lock, flags);
+	iova->dmar = dom;

Quoted 7 lines · mark gross

Quoted 6 lines · Andrew Morton

Quoted 4 lines · On Wed, 20 Feb 2008 16:06:23 -0800 mark gross <[email protected]> wrote:

The following patch is for batching up the flushing of the IOTLB for the DMAR implementation found in the Intel VT-d hardware. It works by building a list of to be flushed IOTLB entries and a bitmap list of which DMAR engine they are from.

I didn't see the original patch - thanks for cc'ing linux-pci.

PA-RISC and IA64 have been doing this for several years now. See DELAYED_RESOURCE_CNT usage in drivers/parisc/sba_iommu.c and in arch/ia64/hp/common/sba_iommu.c.

I prototyped the same concept for x86_64 GART support but it didn't seem to matter on benchmarks since most of the devices I use are 64-bit and don't need to use the IOMMU. IDE/SATA controllers are 32-bit but there is LOTS of overhead in so many other places, this change made less than 0.3 difference for them.

(And the decision to use uncached memory for the IO Pdir made this moot). I'd be happy to post the patch if someone wants to see it though.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

This approach recovers 15 to 20% of the performance lost when using the IOMMU for my netperf udp stream benchmark with small packets. It can be disabled with a kernel boot parameter
"intel_iommu=strict".

Quoted 1 line

Quoted 1 line

Quoted 1 line

Its use does weaken the IOMMU protections a bit.

One can do a few things to limit how much the protections are weakened:
1) Invalidate the IO TLB entry and IO Pdir entries (just don't force syncronization). At least this was possible on the IOMMU's I'm familiar with.

2) release the IO Pdir entry for re-use once syncronization has been forced. Syncronization/flushing of the IO TLB shoot-down is the most expensive ops AFAICT on the IOMMU's I've worked with.

I don't ever recall finding a bug where the device was DMA'ing to a buffer again shortly after unmap call. Most DMA errors are device driver not programming the DMA engines correctly.

Quoted 1 line

Quoted 1 line

drivers/pci/intel-iommu.c line 2037

Quoted 1 line

drivers/pci/intel-iommu.c +1 -0

+	if (intel_iommu_strict) {

I suggest adding an "unlikely()" around this test so the compiler can generate better code for this and it's clear what your intent is.

I just looked at the implementation of flush_unmaps(). I strongly reccomend comparing this implementation with the DELAYED_RESOURCE_CNT since this looks like it will need 2X or more of the CPU cycles to execute for each entry. Use of "list_del()" is substantially more expensive than a simple linear arrary. (Fewer entries per cacheline by 4X maybe?)

Another problem is every now and then some IO is going to burn a bunch of CPU cycles and introduce inconsistent performance for particular unmap operations. One has to tradeoff amortizing the cost of the IOMMU flushing with the number of "unusable" IO Pdir entries and more consistent CPU utilization for each unmap() call. My gut feeling is 250 is rather high for high_watermark unless flushing the IO TLB is extremely expensive.

...

Quoted 2 lines

Quoted 1 line

boot-time options suck. Is it not possible to tweak this at runtime?

Quoted 3 lines

Yes, I could easily create a sysfs or debugfs mechanism for turning it on / off at run time. I would like input on the preferred way to do this. sysfs or debugfs?

I prefer a compile time constant since we are talking about fixed costs for this implementation. The compiler can do a better job with a constant. I know this sounds nit picky but if I can't sufficiently emphasized how perf critical DMA map/unmap code is.

If it has to be a variable, I prefer sysfs but don't have a good reason for that.

Quoted 1 line

Signed-off-by <[email protected]>

Ah! Maybe you can get together with Matthew Wilcox (also) and consider how the IOMMU code might be included in the scsi_ram driver he wrote. Or maybe just directly use the ram disk (rd.c) instead.

At LSF, I suggested using the IOAT DMA engine (if available) do real DMA. Running the IO requests through the IOMMU would expose the added CPU cost of the IOMMU in it's worst case. This doesn't need to go to kernel.org but might help you isolate perf bottlenecks in this iommu code.

Quoted 6 lines

Index: linux-2.6.25-rc2-mm1/drivers/pci/intel-iommu.c
===================================================================

drivers/pci/intel-iommu.c +0 -0

...

drivers/pci/intel-iommu.c line 58

Quoted 3 lines

drivers/pci/intel-iommu.c +3 -0

+static void flush_unmaps_timeout(unsigned long data);
+
+DEFINE_TIMER(unmap_timer,  flush_unmaps_timeout, 0, 0);

Why bother with the timer?

This just adds more overhead and won't help much to improve protection. If someone needs tighter protection, the "strict" unmapping/flushing parameter should be sufficient to track down the issues they might be seeing. And it's perfectly OK to be sitting on a few "unusable" IOMMU entries.

...

Quoted 9 lines

drivers/pci/intel-iommu.c +9 -0

+static void flush_unmaps(void)
+{
+	struct iova *node, *n;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&async_umap_flush_lock, flags);
+	timer_on = 0;
+

drivers/pci/intel-iommu.c line 1976

Quoted 5 lines

drivers/pci/intel-iommu.c +5 -0

+	/* just flush them all */
+	for (i = 0; i < g_num_of_iommus; i++) {
+		if (test_and_clear_bit(i, g_iommus_to_flush))
+			iommu_flush_iotlb_global(&g_iommus[i], 0);
+	}

When we hit a highwater mark, would be make sense to only flush the iommu associated with the device in question?

I'm trying to limit the amount of time spent in any single call to flush_unmaps(). If iommu_flush_iotlb_global() is really fast (ie not 1000s of cycles), then this might be still ok.

But it would certainly make more sense to only flush the iommu associated with the IO device calling unmap.

drivers/pci/intel-iommu.c line 1980

Quoted 3 lines

drivers/pci/intel-iommu.c +3 -0

+	list_for_each_entry_safe(node, n, &unmaps_to_do, list) {
+		/* free iova */
+		list_del(&node->list);

Using a linear array would be alot more efficient than list_del(). One could increment a local index (we are holding the spinlock) and not touch the data again. See code snippet from sba_iommu.c below.

Quoted 6 lines

drivers/pci/intel-iommu.c +6 -0

+		__free_iova(&((struct dmar_domain *)node->dmar)->iovad, node);
+
+	}
+	list_size = 0;
+	spin_unlock_irqrestore(&async_umap_flush_lock, flags);
+}

...

Quoted 6 lines

drivers/pci/intel-iommu.c +6 -0

+static void add_unmap(struct dmar_domain *dom, struct iova *iova)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&async_umap_flush_lock, flags);
+	iova->dmar = dom;

drivers/pci/intel-iommu.c line 2008

Quoted 10 lines

drivers/pci/intel-iommu.c +10 -0

+	list_add(&iova->list, &unmaps_to_do);
+	set_bit((dom->iommu - g_iommus), g_iommus_to_flush);
+
+	if (!timer_on) {
+		mod_timer(&unmap_timer, jiffies + msecs_to_jiffies(10));
+		timer_on = 1;
+	}
+	list_size++;
+	spin_unlock_irqrestore(&async_umap_flush_lock, flags);
+}

I'd like to compare the above with code from parisc sba_iommu.c which does the same thing:
...

        d = &(ioc->saved[ioc->saved_cnt]);
        d->iova = iova;
        d->size = size;
        if (++(ioc->saved_cnt) >= DELAYED_RESOURCE_CNT) {

...

(plus spinlock acquire/release)

map/unmap_single is called _alot_. It's probably called more often than the low level CPU interrupt handler on a busy NIC.

Removing stats gathering from the SBA code yielded an easily measurable difference in perf. I don't recall exactly what it was. Apologies for not quantifying the diference when I made the change:
http://lists.parisc-linux.org/pipermail/parisc-linux-cvs/2004-January/033739.html

drivers/pci/intel-iommu.c line 26

Quoted 5 lines

drivers/pci/intel-iommu.c +1 -0

@@ -23,6 +23,8 @@
 	struct rb_node	node;
 	unsigned long	pfn_hi; /* IOMMU dish out addr hi */
 	unsigned long	pfn_lo; /* IOMMU dish out addr lo */
+	struct list_head list;

Can we have a more descriptive name than "list"?

drivers/pci/intel-iommu.c line 27

Quoted 1 line

drivers/pci/intel-iommu.c +1 -0

+	void *dmar;

Why isn't this declared "struct dmar_domain *"?

I'm looking at this usage in the patch:
+ __free_iova(&((struct dmar_domain *)node->dmar)->iovad, node);

Given this is all for intel IOMMUs, I expect a struct could be visible. But I might be overlooking some higher design here.

thanks and I hope the above helps,

grant

The intel DMAR engines with the TLB's do have a measurable overhead flushing TLB entries on every unmapsingle. This hardware forces a bit poll on TLB flush completion that adds up on high rates of unmap operations. (about 15% on my netperf small packet 32 byte UDP stream over a 1GigE adapter)

FWIW: I think IOMMU use for avoiding bounce buffers is a thing of the past.

(at least I have a "show me where it helps" point of view for modern hardware. what new hardware needs bounce buffers anymore?)

Also, to be fair to the Intel hardware the performance overhead is really only noticeable on small packed and 10Gig benchmarking. Most users will not be able to notice the overhead.

Quoted 5 lines

One can do a few things to limit how much the protections are weakened:
1) Invalidate the IO TLB entry and IO Pdir entries (just don't force syncronization). At least this was possible on the IOMMU's I'm familiar with.

The default is the TLB invalidation. It has a nasty polling loop that hurts on the small packet netperf benchmark.

Quoted 1 line

Quoted 3 lines

2) release the IO Pdir entry for re-use once syncronization has been forced. Syncronization/flushing of the IO TLB shoot-down is the most expensive ops AFAICT on the IOMMU's I've worked with.

this is what the patch does.

Quoted 7 lines

I just looked at the implementation of flush_unmaps(). I strongly reccomend comparing this implementation with the DELAYED_RESOURCE_CNT since this looks like it will need 2X or more of the CPU cycles to execute for each entry. Use of "list_del()" is substantially more expensive than a simple linear arrary. (Fewer entries per cacheline by 4X maybe?)

/me looks, an array of pointers to iova's to clean up makes good sense. I don't know why I didn't think of this approach first. Working with lists is messy and if I can avoid it I usually try.

Quoted 9 lines

Another problem is every now and then some IO is going to burn a bunch of CPU cycles and introduce inconsistent performance for particular unmap operations. One has to tradeoff amortizing the cost of the IOMMU flushing with the number of "unusable" IO Pdir entries and more consistent CPU utilization for each unmap() call. My gut feeling is 250 is rather high for high_watermark unless flushing the IO TLB is extremely expensive.

the 250 high water mark came from 1gig small packet netperf udp streaming testing. The 1000 I initially used didn't perform quite as well. Nothing scientific went into this number just some tests cases.

Quoted 7 lines

...

Quoted 5 lines

Quoted 1 line

boot-time options suck. Is it not possible to tweak this at runtime?

Yes, I could easily create a sysfs or debugfs mechanism for turning it on / off at run time. I would like input on the preferred way to do this. sysfs or debugfs?

Quoted 4 lines

I prefer a compile time constant since we are talking about fixed costs for this implementation. The compiler can do a better job with a constant. I know this sounds nit picky but if I can't sufficiently emphasized how perf critical DMA map/unmap code is.

Compile time options lock out OSV's from using the feature.

These IOMMU's are in desktop systems now. My job is to make it not suck so OSV's will leave DMAR enabled in production kernels. (well at least try to anyway.)

Quoted 6 lines

If it has to be a variable, I prefer sysfs but don't have a good reason for that.

Quoted 1 line

Signed-off-by <[email protected]>

Quoted 3 lines

Ah! Maybe you can get together with Matthew Wilcox (also) and consider how the IOMMU code might be included in the scsi_ram driver he wrote. Or maybe just directly use the ram disk (rd.c) instead.

Yup, Willi has just started to look over my shoulder on some of this. He pointed me at some OLS papers and whatnot to look at. I botched my chance to work face to face with him last week on his last visit to Oregon but we'll keep talking.

Quoted 4 lines

At LSF, I suggested using the IOAT DMA engine (if available) do real DMA. Running the IO requests through the IOMMU would expose the added CPU cost of the IOMMU in it's worst case. This doesn't need to go to kernel.org but might help you isolate perf bottlenecks in this iommu code.

Oh the worst cases are easy. Just run a 10gig NIC and see what you get.

With a 1Gig NIC you need to drop the packet size down to 32 bytes or something tiny like that to really notice it. 16K packets have only a smallish overhead that are iffy as benchmarks because the CPU utilization is too low (<25%) to be a good measure.

FWIW : throughput isn't effected so much as the CPU overhead. iommu=strict: 16K UDP UNIDIRECTIONAL SEND TEST 826Mbps at 25% cpu with this patch: 16K UDP UNIDIRECTIONAL SEND TEST 826Mbps at 18% cpu IOMMU=OFF: 16K UDP UNIDIRECTIONAL SEND TEST 826Mbps at 11% cpu

Quoted 5 lines · mark gross

The intel DMAR engines with the TLB's do have a measurable overhead flushing TLB entries on every unmapsingle. This hardware forces a bit poll on TLB flush completion that adds up on high rates of unmap operations. (about 15% on my netperf small packet 32 byte UDP stream over a 1GigE adapter)

So if we flushed the IOTLB every 16 unmap calls, it should be < 1% penalty?

Using that logic is how I decided the current calue of DELAYED_RESOURCE_CNT on parisc.

Quoted 2 lines

FWIW: I think IOMMU use for avoiding bounce buffers is a thing of the past.

Agreed. PA-RISC is a legacy platform at this point.

Quoted 2 lines

(at least I have a "show me where it helps" point of view for modern hardware. what new hardware needs bounce buffers anymore?)

Otherwise, Anything running in a legacy IDE mode is using 32-bit DMA. So linux still has plenty of SATA drivers using 32-bit DMA.

All the NICs, FC, Infiniband, et al are PCI-e and thus by definition 64-bit DMA capable.

Quoted 3 lines

Also, to be fair to the Intel hardware the performance overhead is really only noticeable on small packed and 10Gig benchmarking. Most users will not be able to notice the overhead.

*nod* - I know. That why I use pktgen to measure the dma map/unmap overhead. Note that with solid state storage (should be more visible in the next year or so), the transaction rate is going to look alot more like a NIC than the traditional HBA storage controller. So map/unmap performance will matter for those configurations too.

Quoted 16 lines

Quoted 7 lines

One can do a few things to limit how much the protections are weakened:
1) Invalidate the IO TLB entry and IO Pdir entries (just don't force syncronization). At least this was possible on the IOMMU's I'm familiar with.

The default is the TLB invalidation. It has a nasty polling loop that hurts on the small packet netperf benchmark.

Quoted 4 lines

2) release the IO Pdir entry for re-use once syncronization has been forced. Syncronization/flushing of the IO TLB shoot-down is the most expensive ops AFAICT on the IOMMU's I've worked with.

Quoted 1 line

this is what the patch does.

Ok - I wasn't sure which step was the "syncronize step".

BTW, I hope you (and others from Intel - go willy! ;) can give feedback to the Intel/AMD chipset designers to ditch this design ASAP. It clearly sucks.

Quoted 11 lines

Quoted 7 lines

I just looked at the implementation of flush_unmaps(). I strongly reccomend comparing this implementation with the DELAYED_RESOURCE_CNT since this looks like it will need 2X or more of the CPU cycles to execute for each entry. Use of "list_del()" is substantially more expensive than a simple linear arrary. (Fewer entries per cacheline by 4X maybe?)

/me looks, an array of pointers to iova's to clean up makes good sense. I don't know why I didn't think of this approach first. Working with lists is messy and if I can avoid it I usually try.

*shrug* I didn't think too much about it on the original implementation and it happened to work out nicely. Think cacheline "life cycle" when picking array sizes (or when comparing to linked lists) and you'll usually end up with the best performing design.

Quoted 10 lines

Quoted 9 lines

Another problem is every now and then some IO is going to burn a bunch of CPU cycles and introduce inconsistent performance for particular unmap operations. One has to tradeoff amortizing the cost of the IOMMU flushing with the number of "unusable" IO Pdir entries and more consistent CPU utilization for each unmap() call. My gut feeling is 250 is rather high for high_watermark unless flushing the IO TLB is extremely expensive.

Quoted 3 lines

the 250 high water mark came from 1gig small packet netperf udp streaming testing. The 1000 I initially used didn't perform quite as well. Nothing scientific went into this number just some tests cases.

Well, it can be more scientific. Determine the overhead (with vs without). Then divide by how many times one can afford to ignore the sync op. That will be (roughly) be the new overhead.

Quoted 7 lines

Quoted 7 lines

...

Quoted 5 lines

Quoted 1 line

boot-time options suck. Is it not possible to tweak this at runtime?

Yes, I could easily create a sysfs or debugfs mechanism for turning it on / off at run time. I would like input on the preferred way to do this. sysfs or debugfs?

Quoted 6 lines

Quoted 4 lines

I prefer a compile time constant since we are talking about fixed costs for this implementation. The compiler can do a better job with a constant. I know this sounds nit picky but if I can't sufficiently emphasized how perf critical DMA map/unmap code is.

Compile time options lock out OSV's from using the feature.

Huh? I think you misunderstood. Decide how _many_ you want to defer at compile time and leave the general feature enabled as a runtime option. (e.g. use_vtd=1 boot param or when booting with Xen or KVM enabled).

Quoted 3 lines

These IOMMU's are in desktop systems now. My job is to make it not suck so OSV's will leave DMAR enabled in production kernels. (well at least try to anyway.)

Ok - my suggestion still applies. "compile time constant" only refers to the number of unmaps the code will defer. e.g. "borrow" DELAY_RESOURCE_CNT. I understand the DMAR needs to be enabled at runtime for production. (I've spent ~5 years dealing with RH/SuSE/Debian IA64-linux kernels...)

If you can reduce the overhead to < 1% for pktgen, TPC-C won't notice it and I doubt specweb will either.

Quoted 11 lines

Quoted 10 lines

If it has to be a variable, I prefer sysfs but don't have a good reason for that.

Quoted 1 line

Signed-off-by <[email protected]>

Ah! Maybe you can get together with Matthew Wilcox (also) and consider how the IOMMU code might be included in the scsi_ram driver he wrote. Or maybe just directly use the ram disk (rd.c) instead.

Quoted 4 lines

Yup, Willi has just started to look over my shoulder on some of this. He pointed me at some OLS papers and whatnot to look at. I botched my chance to work face to face with him last week on his last visit to Oregon but we'll keep talking.

Excellent. I'm also happy to answer questions on linux-pci about this. And willy will be back in oregon I'm sure. :)
You can also visit him in Ottawa if you register for OLS this year.

Quoted 6 lines

Quoted 4 lines

At LSF, I suggested using the IOAT DMA engine (if available) do real DMA. Running the IO requests through the IOMMU would expose the added CPU cost of the IOMMU in it's worst case. This doesn't need to go to kernel.org but might help you isolate perf bottlenecks in this iommu code.

Oh the worst cases are easy. Just run a 10gig NIC and see what you get.

Well, you still need a consistent benchmark/workload (e.g. pktgen) and a precise tool to measure the overhead (e.g. oprofile or perfmon2).

Using the IOAT would allow people without 10G NICs to mess around with this.

Quoted 4 lines

With a 1Gig NIC you need to drop the packet size down to 32 bytes or something tiny like that to really notice it. 16K packets have only a smallish overhead that are iffy as benchmarks because the CPU utilization is too low (<25%) to be a good measure.

*nod*

Quoted 4 lines

FWIW : throughput isn't effected so much as the CPU overhead. iommu=strict: 16K UDP UNIDIRECTIONAL SEND TEST 826Mbps at 25% cpu with this patch: 16K UDP UNIDIRECTIONAL SEND TEST 826Mbps at 18% cpu IOMMU=OFF: 16K UDP UNIDIRECTIONAL SEND TEST 826Mbps at 11% cpu

Understood. That's why netperf (see netperf.org) measures "service demand". Taking CPU away from user space generally results in lower benchmark/app perf.

Quoted 5 lines

*nod* - I know. That why I use pktgen to measure the dma map/unmap overhead. Note that with solid state storage (should be more visible in the next year or so), the transaction rate is going to look alot more like a NIC than the traditional HBA storage controller. So map/unmap performance will matter for those configurations too.

Sweet! now I have an excuse to get one of those spiffy SD-Disks!

Quoted 6 lines

Ok - I wasn't sure which step was the "syncronize step".

BTW, I hope you (and others from Intel - go willy! ;) can give feedback to the Intel/AMD chipset designers to ditch this design ASAP. It clearly sucks.

The HW implementation IS evolving. Especially as the MCH and more of the chipsets are moved into the CPU package. It will get better over time, but the protection will never be "free".

Quoted 2 lines

If you can reduce the overhead to < 1% for pktgen, TPC-C won't notice it and I doubt specweb will either.

Sadly only a fraction of the overhead is due to the IOTLB flushes. I can't wave away the IOVA management overhead with batched flushes of the IOTLB.

I've been using oprofile, and some TSC counters I have in an out-of tree patch for instrumenting the code and dumping the cycles per code-path-of-interest. Its been pretty effective, but it affects the throughput of the run.

Quoted 8 lines

Quoted 4 lines

FWIW : throughput isn't effected so much as the CPU overhead. iommu=strict: 16K UDP UNIDIRECTIONAL SEND TEST 826Mbps at 25% cpu with this patch: 16K UDP UNIDIRECTIONAL SEND TEST 826Mbps at 18% cpu IOMMU=OFF: 16K UDP UNIDIRECTIONAL SEND TEST 826Mbps at 11% cpu

Understood. That's why netperf (see netperf.org) measures "service demand". Taking CPU away from user space generally results in lower benchmark/app perf.

The following patch is an update to use an array instead of a list of IOVA's in the implementation of defered iotlb flushes. It takes inspiration from sba_iommu.c

I like this implementation better as it encapsulates the batch process within intel-iommu.c, and no longer touches iova.h (which is shared)

Performance data: Netperf 32byte UDP streaming 2.6.25-rc3-mm1:
IOMMU-strict : 58Mps @ 62% cpu
NO-IOMMU : 71Mbs @ 41% cpu
List-based IOMMU-default-batched-IOTLB flush: 66Mbps @ 57% cpu

with this patch:
IOMMU-strict : 73Mps @ 75% cpu
NO-IOMMU : 74Mbs @ 42% cpu
Array-based IOMMU-default-batched-IOTLB flush: 72Mbps @ 62% cpu