Revert "iommu/tegra: smmu: Fix unsleepable memory allocation"

PATCH In review Hiroshi DOYU 4 participants 7 messages Jul 2, 2012 – Jul 18, 2012 2 messages not in the archive

Subsystems (3)

Tegra architecture support IOMMU Tegra IOMMU drivers

lore

Share & tools

lore

Hiroshi DOYU

author

2012-07-02 11:26 UTC #0 lore raw

PATCH 1/3 Revert "iommu/tegra: smmu: Fix unsleepable memory allocation"

This reverts commit 64b3aeed242ae55611e0f6286159957873cf968d.

For the succeeding commits.

Signed-off-by Hiroshi DOYU <[email protected]>

---
 drivers/iommu/tegra-smmu.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/iommu/tegra-smmu.c +2 -2

index 3f3d09d..ecd6790 100644
@@ -550,13 +550,13 @@ static int alloc_pdir(struct smmu_as *as)
 		return 0;
 
 	as->pte_count = devm_kzalloc(smmu->dev,
-		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_ATOMIC);
+		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
 	if (!as->pte_count) {
 		dev_err(smmu->dev,
 			"failed to allocate smmu_device PTE cunters\n");
 		return -ENOMEM;
 	}
-	as->pdir_page = alloc_page(GFP_ATOMIC | __GFP_DMA);
+	as->pdir_page = alloc_page(GFP_KERNEL | __GFP_DMA);
 	if (!as->pdir_page) {
 		dev_err(smmu->dev,
 			"failed to allocate smmu_device page directory\n");

Signature · 1 line

1.7.5.4

A message that is not in the archive (sent off-list or not yet received): 1341228398-6878-1-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org

Hiroshi DOYU

author

2012-07-02 11:26 UTC #2 lore raw

PATCH 2/3 iommu/tegra: smmu: Remove unnecessary sanity check at alloc_pdir()

alloc_pdir() is called with smmu->as[?].pdir_page == NULL. No need to check pdir_page again inside alloc_pdir().

Signed-off-by Hiroshi DOYU <[email protected]>

Acked-by Stephen Warren <[email protected]>

---
 drivers/iommu/tegra-smmu.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

drivers/iommu/tegra-smmu.c +0 -3

index ecd6790..532c8a4 100644
@@ -546,9 +546,6 @@ static int alloc_pdir(struct smmu_as *as)
 	u32 val;
 	struct smmu_device *smmu = as->smmu;
 
-	if (as->pdir_page)
-		return 0;
-
 	as->pte_count = devm_kzalloc(smmu->dev,
 		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
 	if (!as->pte_count) {

Signature · 1 line

1.7.5.4

Hiroshi DOYU

author

2012-07-02 11:26 UTC #3 lore raw

PATCH 3/3 iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

In reply to a message not in the archive

alloc_pdir() is called from smmu_iommu_domain_init() with spin_lock held. memory allocations in alloc_pdir() had to be atomic. Instead of converting into atomic allocation, this patch once releases a lock, does the allocation, holds the lock again and then sees if it's raced or not in order to avoid introducing mutex and preallocation.

Signed-off-by Hiroshi DOYU <[email protected]>

Reported-by Chris Wright <[email protected]>

Cc Chris Wright <[email protected]>

Acked-by Stephen Warren <[email protected]>

---
 drivers/iommu/tegra-smmu.c |   77 +++++++++++++++++++++++++------------------
 1 files changed, 45 insertions(+), 32 deletions(-)

drivers/iommu/tegra-smmu.c +45 ~~-32~~

index 532c8a4..dbba94c 100644
@@ -539,28 +539,39 @@ static inline void put_signature(struct smmu_as *as,
 /*
  * Caller must lock/unlock as
  */
-static int alloc_pdir(struct smmu_as *as)
+static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
 {
 	unsigned long *pdir;
-	int pdn;
+	int pdn, err = 0;
 	u32 val;
 	struct smmu_device *smmu = as->smmu;
+	struct page *page;
+	unsigned int *cnt;
 
-	as->pte_count = devm_kzalloc(smmu->dev,
-		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
-	if (!as->pte_count) {
-		dev_err(smmu->dev,
-			"failed to allocate smmu_device PTE cunters\n");
-		return -ENOMEM;
+	/*
+	 * do the allocation outside the as lock
+	 */
+	spin_unlock_irqrestore(&as->lock, *flags);
+	cnt = devm_kzalloc(smmu->dev,
+			   sizeof(cnt[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
+	page = alloc_page(GFP_KERNEL | __GFP_DMA);
+	spin_lock_irqsave(&as->lock, *flags);
+
+	if (as->pdir_page) {
+		/* We raced, free the redundant */
+		err = -EAGAIN;
+		goto err_out;
 	}
-	as->pdir_page = alloc_page(GFP_KERNEL | __GFP_DMA);
-	if (!as->pdir_page) {
-		dev_err(smmu->dev,
-			"failed to allocate smmu_device page directory\n");
-		devm_kfree(smmu->dev, as->pte_count);
-		as->pte_count = NULL;
-		return -ENOMEM;
+
+	if (!page || !cnt) {
+		dev_err(smmu->dev, "failed to allocate at %s\n", __func__);
+		err = -ENOMEM;
+		goto err_out;
 	}
+
+	as->pdir_page = page;
+	as->pte_count = cnt;
+
 	SetPageReserved(as->pdir_page);
 	pdir = page_address(as->pdir_page);

@@ -577,6 +588,12 @@ static int alloc_pdir(struct smmu_as *as)
 	FLUSH_SMMU_REGS(as->smmu);
 
 	return 0;
+
+err_out:
+	devm_kfree(smmu->dev, cnt);
+	if (page)
+		__free_page(page);
+	return err;
 }
 
 static void __smmu_iommu_unmap(struct smmu_as *as, dma_addr_t iova)

@@ -768,29 +785,29 @@ out:
 
 static int smmu_iommu_domain_init(struct iommu_domain *domain)
 {
-	int i;
+	int i, err = -ENODEV;
 	unsigned long flags;
 	struct smmu_as *as;
 	struct smmu_device *smmu = smmu_handle;
 
 	/* Look for a free AS with lock held */
 	for  (i = 0; i < smmu->num_as; i++) {
-		struct smmu_as *tmp = &smmu->as[i];
-
-		spin_lock_irqsave(&tmp->lock, flags);
-		if (!tmp->pdir_page) {
-			as = tmp;
-			goto found;
+		as = &smmu->as[i];
+		spin_lock_irqsave(&as->lock, flags);
+		if (!as->pdir_page) {
+			err = alloc_pdir(as, &flags);
+			if (!err)
+				goto found;
 		}
-		spin_unlock_irqrestore(&tmp->lock, flags);
+		spin_unlock_irqrestore(&as->lock, flags);
+		if (err != -EAGAIN)
+			break;
 	}
-	dev_err(smmu->dev, "no free AS\n");
-	return -ENODEV;
+	if (i == smmu->num_as)
+		dev_err(smmu->dev,  "no free AS\n");
+	return err;
 
 found:
-	if (alloc_pdir(as) < 0)
-		goto err_alloc_pdir;
-
 	spin_lock(&smmu->lock);
 
 	/* Update PDIR register */

@@ -806,10 +823,6 @@ found:
 
 	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
 	return 0;
-
-err_alloc_pdir:
-	spin_unlock_irqrestore(&as->lock, flags);
-	return -ENODEV;
 }
 
 static void smmu_iommu_domain_destroy(struct iommu_domain *domain)

Signature · 1 line

1.7.5.4

A message that is not in the archive (sent off-list or not yet received): 1341228398-6878-3-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org

Joerg Roedel

2012-07-17 10:09 UTC #5 lore raw

Re: [v3.6 3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

Quoted 4 lines · Hiroshi DOYU

Signed-off-by Hiroshi DOYU <[email protected]>
Reported-by Chris Wright <[email protected]>
Cc Chris Wright <[email protected]>
Acked-by Stephen Warren <[email protected]>

Applied patch 2 and 3 but not patch 1. The resulting conflicts are solved while merging the next branch. Also I am not happy with the way the as->lock is taken and released multiple times in patch 3. So I added another commit on-top. Please have a look at it as I can only compile-test that change:

Quoted 1 line

From f9a4f063a88297e361fd6676986cf3e39b22de72 Mon Sep 17 00:00:00 2001

From: Joerg Roedel <[email protected]>
Date: Tue, 17 Jul 2012 11:47:14 +0200
Subject: [PATCH] iommu/tegra: Don't call alloc_pdir with as->lock

Instead of taking as->lock before calling alloc_pdir() and releasing it in that function to allocate memory, just take the lock only in the alloc_pdir function and run the loop without any lock held. This simplifies the complicated lock->unlock->alloc->lock->unlock sequence into alloc->lock->unlock.

Signed-off-by Joerg Roedel <[email protected]>

---
 drivers/iommu/tegra-smmu.c |   29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

drivers/iommu/tegra-smmu.c +16 ~~-13~~

index 68a15a0..541d210 100644
@@ -553,11 +553,11 @@ static inline void put_signature(struct smmu_as *as,
 #endif
 
 /*
- * Caller must lock/unlock as
+ * Caller must not hold as->lock
  */
-static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
+static int alloc_pdir(struct smmu_as *as)
 {
-	unsigned long *pdir;
+	unsigned long *pdir, flags;
 	int pdn, err = 0;
 	u32 val;
 	struct smmu_device *smmu = as->smmu;

@@ -565,13 +565,14 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
 	unsigned int *cnt;
 
 	/*
-	 * do the allocation outside the as->lock
+	 * do the allocation, then grab as->lock
 	 */
-	spin_unlock_irqrestore(&as->lock, *flags);
 	cnt = devm_kzalloc(smmu->dev,
-			   sizeof(cnt[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
+			   sizeof(cnt[0]) * SMMU_PDIR_COUNT,
+			   GFP_KERNEL);
 	page = alloc_page(GFP_KERNEL | __GFP_DMA);
-	spin_lock_irqsave(&as->lock, *flags);
+
+	spin_lock_irqsave(&as->lock, flags);
 
 	if (as->pdir_page) {
 		/* We raced, free the redundant */

@@ -603,9 +604,13 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
 	smmu_write(smmu, val, SMMU_TLB_FLUSH);
 	FLUSH_SMMU_REGS(as->smmu);
 
+	spin_unlock_irqrestore(&as->lock, flags);
+
 	return 0;
 
 err_out:
+	spin_unlock_irqrestore(&as->lock, flags);
+
 	devm_kfree(smmu->dev, cnt);
 	if (page)
 		__free_page(page);

@@ -809,13 +814,11 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
 	/* Look for a free AS with lock held */
 	for  (i = 0; i < smmu->num_as; i++) {
 		as = &smmu->as[i];
-		spin_lock_irqsave(&as->lock, flags);
 		if (!as->pdir_page) {
-			err = alloc_pdir(as, &flags);
+			err = alloc_pdir(as);
 			if (!err)
 				goto found;
 		}
-		spin_unlock_irqrestore(&as->lock, flags);
 		if (err != -EAGAIN)
 			break;
 	}

@@ -824,7 +827,7 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
 	return err;
 
 found:
-	spin_lock(&smmu->lock);
+	spin_lock_irqsave(&smmu->lock, flags);
 
 	/* Update PDIR register */
 	smmu_write(smmu, SMMU_PTB_ASID_CUR(as->asid), SMMU_PTB_ASID);

@@ -832,12 +835,12 @@ found:
 		   SMMU_MK_PDIR(as->pdir_page, as->pdir_attr), SMMU_PTB_DATA);
 	FLUSH_SMMU_REGS(smmu);
 
-	spin_unlock(&smmu->lock);
+	spin_unlock_irqrestore(&smmu->lock, flags);
 
-	spin_unlock_irqrestore(&as->lock, flags);
 	domain->priv = as;
 
 	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
+
 	return 0;
 }

Signature · 9 lines

1.7.9.5


-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Hiroshi Doyu

2012-07-17 12:25 UTC #6 lore raw

PATCH 3/3 iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

Hi Joerg,

Joerg Roedel <[email protected]> wrote @ Tue, 17 Jul 2012 12:09:01 +0200:

Quoted 83 lines · Hiroshi DOYU

Quoted 5 lines
Signed-off-by Hiroshi DOYU <[email protected]>
Reported-by Chris Wright <[email protected]>
Cc Chris Wright <[email protected]>
Acked-by Stephen Warren <[email protected]>
Applied patch 2 and 3 but not patch 1. The resulting conflicts are solved while merging the next branch. Also I am not happy with the way the as->lock is taken and released multiple times in patch 3. So I added another commit on-top. Please have a look at it as I can only compile-test that change:
From f9a4f063a88297e361fd6676986cf3e39b22de72 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <[email protected]>
Date: Tue, 17 Jul 2012 11:47:14 +0200
Subject: [PATCH] iommu/tegra: Don't call alloc_pdir with as->lock
Instead of taking as->lock before calling alloc_pdir() and releasing it in that function to allocate memory, just take the lock only in the alloc_pdir function and run the loop without any lock held. This simplifies the complicated lock->unlock->alloc->lock->unlock sequence into alloc->lock->unlock.
Signed-off-by Joerg Roedel <[email protected]>
---
 drivers/iommu/tegra-smmu.c |   29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)
drivers/iommu/tegra-smmu.c +12 -7
index 68a15a0..541d210 100644
@@ -553,11 +553,11 @@ static inline void put_signature(struct smmu_as *as,
 #endif
 
 /*
- * Caller must lock/unlock as
+ * Caller must not hold as->lock
  */
-static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
+static int alloc_pdir(struct smmu_as *as)
 {
-	unsigned long *pdir;
+	unsigned long *pdir, flags;
 	int pdn, err = 0;
 	u32 val;
 	struct smmu_device *smmu = as->smmu;
@@ -565,13 +565,14 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
 	unsigned int *cnt;
 
 	/*
-	 * do the allocation outside the as->lock
+	 * do the allocation, then grab as->lock
 	 */
-	spin_unlock_irqrestore(&as->lock, *flags);
 	cnt = devm_kzalloc(smmu->dev,
-			   sizeof(cnt[0]) * SMMU_PDIR_COUNT, GFP_KERNEL);
+			   sizeof(cnt[0]) * SMMU_PDIR_COUNT,
+			   GFP_KERNEL);
 	page = alloc_page(GFP_KERNEL | __GFP_DMA);
-	spin_lock_irqsave(&as->lock, *flags);
+
+	spin_lock_irqsave(&as->lock, flags);
 
 	if (as->pdir_page) {
 		/* We raced, free the redundant */
@@ -603,9 +604,13 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
 	smmu_write(smmu, val, SMMU_TLB_FLUSH);
 	FLUSH_SMMU_REGS(as->smmu);
 
+	spin_unlock_irqrestore(&as->lock, flags);
+
 	return 0;
 
 err_out:
+	spin_unlock_irqrestore(&as->lock, flags);
+
 	devm_kfree(smmu->dev, cnt);
 	if (page)
 		__free_page(page);

drivers/iommu/tegra-smmu.c static int smmu_iommu_domain_init(struct iommu_domain *domain) · line 820

Quoted 9 lines

drivers/iommu/tegra-smmu.c +1 -2

@@ -809,13 +814,11 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
 	/* Look for a free AS with lock held */
 	for  (i = 0; i < smmu->num_as; i++) {
 		as = &smmu->as[i];
-		spin_lock_irqsave(&as->lock, flags);
 		if (!as->pdir_page) {
-			err = alloc_pdir(as, &flags);
+			err = alloc_pdir(as);
 			if (!err)
 				goto found;

The above spin_lock is always necessary. "as->lock" should be held to protect "as->pdir_page". Only when "as->pdir_page" is NULL, "as->pdir_page" would be allocated in "alloc_pdir()". Without this lock, the following race could happen:

Without as->lock:
A: B:
i == 3
pdir_page == NULL

			i == 3
	     		pdir_page == NULL
pdir_page = a;
			pdir_page = b;	!!!!!! OVERWRITTEN !!!!!!

With as->lock:
A: B:
i == 3
lock(as->lock)
pdir_page == NULL
i == 3
Waiting lock released....
Waiting lock released....
pdir_page = a;
unlock(as->lock)

			lock(as->lock)
			pdir_page != NULL && continue
			unlock(as->lock)

			i == 4
			.....

This "lock, unlock, alloc, lock, check race" method was originally introduced by Russell King a few years ago(*1). And the same mechanism has been used in omap iommu for years(*2) at least as below:

drivers/iommu/omap-iommu.c:
.....
505 * do the allocation outside the page table lock
506 */

507         spin_unlock(&obj->page_table_lock);
508         iopte = kmem_cache_zalloc(iopte_cachep, GFP_KERNEL);
509         spin_lock(&obj->page_table_lock);

510
511 if (!*iopgd) {
512 if (!iopte)
513 return ERR_PTR(-ENOMEM);
514
515 *iopgd = virt_to_phys(iopte) | IOPGD_TABLE;
516 flush_iopgd_range(iopgd, iopgd);
517
518 dev_vdbg(obj->dev, "%s: a new pte:%p\n", __func__, iopte);
519 } else {
520 /* We raced, free the reduniovant table */
521 iopte_free(iopte);
522 }

Still we can do preallocation for pdir_page before this lock held, but if we do that, we have to change the function name, "alloc_pdir()" to something else because it doesn't allocate actually, and some other allocations also have to be done in advance too. At this moment, I'd rather keep the current structure with Russell's method.

*1:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg04007.html *2:
http://lxr.free-electrons.com/source/drivers/iommu/omap-iommu.c#L496

joerg.roedel at amd.com

2012-07-17 13:23 UTC #7 lore raw

PATCH 3/3 iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

Quoted 6 lines · Hiroshi Doyu

The above spin_lock is always necessary. "as->lock" should be held to protect "as->pdir_page". Only when "as->pdir_page" is NULL, "as->pdir_page" would be allocated in "alloc_pdir()". Without this lock, the following race could happen:

Quoted 9 lines

Without as->lock:
A: B:
i == 3
pdir_page == NULL
			i == 3
	     		pdir_page == NULL
pdir_page = a;
			pdir_page = b;	!!!!!! OVERWRITTEN !!!!!!

Unless I am missing something, this is not the correct situation with my patch. It would look more like this:

A: B:
i == 3
pdir_page == NULL
i == 3 pdir_page == NULL

take as->lock

/* race check */
pdir_page == NULL -> proceed /* spinning on as->lock */

pdir_page = a;

release as->lock

					 take as->lock

					 /* race check */
					 pdir_page != NULL -> return

This should be fine, no? Do I miss something?

Joerg

Hiroshi Doyu

2012-07-18 08:50 UTC #8 lore raw

PATCH 3/3 iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

"joerg.roedel at amd.com" <[email protected]> wrote @ Tue, 17 Jul 2012 15:23:00 +0200:

Quoted 36 lines · Hiroshi Doyu

Quoted 6 lines
The above spin_lock is always necessary. "as->lock" should be held to protect "as->pdir_page". Only when "as->pdir_page" is NULL, "as->pdir_page" would be allocated in "alloc_pdir()". Without this lock, the following race could happen:
Quoted 9 lines
Without as->lock:
A: B:
i == 3
pdir_page == NULL
			i == 3
	     		pdir_page == NULL
pdir_page = a;
			pdir_page = b;	!!!!!! OVERWRITTEN !!!!!!
Unless I am missing something, this is not the correct situation with my patch. It would look more like this:
A: B:
i == 3
pdir_page == NULL
(quoted diff) +0 -0
 					i == 3
 	     				pdir_page == NULL
 
 take as->lock
 
 /* race check */
 pdir_page == NULL -> proceed		/* spinning on as->lock */
 
 pdir_page = a;
 
 release as->lock

Quoted 6 lines

					 take as->lock

					 /* race check */
					 pdir_page != NULL -> return
This should be fine, no? Do I miss something?

You are right. I didn't get the point of your patch. In the case that you can return -EAGAIN, the complicated "lock,unlock,lock,check race" is not necessary as you did.

Verified the patch w/ Tegra3 based board. Please put this into next queue. Thanks.