tegra alloc_pdir

tegra alloc_pdir

[Chris Wright]

This looks buggy to me:

drivers/iommu/tegra-smmu.c::smmu_iommu_domain_init()

  spin_lock_irqsave(&tmp->lock, flags);   <-- lock held, interrupts off
  alloc_pdir(as = tmp)
    devm_kzalloc(..., GFP_KERNEL);        <-- potentially sleeping allocation
    ...
    alloc_page(..., GFP_KERNEL | __GFP_DMA);   <-- same here

thanks,
-chris

Re: tegra alloc_pdir

[Hiroshi Doyu]

Hi Chris,

Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org> wrote @ Tue, 26 Jun 2012 20:15:55 +0200:

> This looks buggy to me:
> 
> drivers/iommu/tegra-smmu.c::smmu_iommu_domain_init()
> 
>   spin_lock_irqsave(&tmp->lock, flags);   <-- lock held, interrupts off
>   alloc_pdir(as = tmp)
>     devm_kzalloc(..., GFP_KERNEL);        <-- potentially sleeping allocation
>     ...
>     alloc_page(..., GFP_KERNEL | __GFP_DMA);   <-- same here

Right. I totally agree. Thank you for pointing this out.

The fix patch follows.

[PATCH 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation

[Hiroshi DOYU]

allo_pdir() is called in smmu_iommu_domain_init() with spin_lock
held. memory allocations in it have to be atomic/unsleepable.

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Reported-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ecd6790..3f3d09d 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -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_KERNEL);
+		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_ATOMIC);
 	if (!as->pte_count) {
 		dev_err(smmu->dev,
 			"failed to allocate smmu_device PTE cunters\n");
 		return -ENOMEM;
 	}
-	as->pdir_page = alloc_page(GFP_KERNEL | __GFP_DMA);
+	as->pdir_page = alloc_page(GFP_ATOMIC | __GFP_DMA);
 	if (!as->pdir_page) {
 		dev_err(smmu->dev,
 			"failed to allocate smmu_device page directory\n");
-- 
1.7.5.4

Re: [PATCH 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation

[Chris Wright]

* Hiroshi DOYU (hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org) wrote:
> allo_pdir() is called in smmu_iommu_domain_init() with spin_lock
> held. memory allocations in it have to be atomic/unsleepable.
> 
> Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Reported-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>

Acked-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>

Re: [PATCH 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation

[Stephen Warren]

On 06/27/2012 03:54 AM, Hiroshi DOYU wrote:
> allo_pdir() is called in smmu_iommu_domain_init() with spin_lock
> held. memory allocations in it have to be atomic/unsleepable.

Presumably this will cause allocation failures in situations with heavy
memory pressure, rather than triggering swapping. Is it normal for IOMMU
drivers to work that way? If so, I'm fine with it. Otherwise, can the
allocations be performed before the spinlock is taken?

Re: [PATCH 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation

[Hiroshi Doyu]

On Wed, 27 Jun 2012 18:59:48 +0200
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> On 06/27/2012 03:54 AM, Hiroshi DOYU wrote:
> > allo_pdir() is called in smmu_iommu_domain_init() with spin_lock
> > held. memory allocations in it have to be atomic/unsleepable.
> 
> Presumably this will cause allocation failures in situations with heavy
> memory pressure, rather than triggering swapping. Is it normal for IOMMU
> drivers to work that way? If so, I'm fine with it. Otherwise, can the
> allocations be performed before the spinlock is taken?

There are some requirement for atomic allocation from DMA mapping API,
but it's not normla for IOMMU, especiall not for
iommu_domain_alloc(). It has already used GFP_KERNEL and then, calls
->iommu_domain_init() in it.

The update patch v2 follows.

[v2 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Hiroshi DOYU]

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

Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Reported-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
Cc: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c |   49 +++++++++++++++++++++++++++++--------------
 1 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ecd6790..ec656ec 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -539,31 +539,42 @@ 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;
 
 	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) {
-		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 = -ENODEV;
+		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);
 
@@ -580,6 +591,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)
@@ -791,7 +808,7 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
 	return -ENODEV;
 
 found:
-	if (alloc_pdir(as) < 0)
+	if (alloc_pdir(as, &flags) < 0)
 		goto err_alloc_pdir;
 
 	spin_lock(&smmu->lock);
-- 
1.7.5.4

Re: [v2 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Stephen Warren]

On 06/28/2012 04:51 AM, Hiroshi DOYU wrote:
> alloc_pdir() is called from smmu_iommu_domain_init() with spin_lock
> held. memory allocations in alloc_pdir() had to be
> atomic/unsleepable. Instead of converting into atomic allocation, this
> patch once releases a lock, do the allocation, hold the lock again and
> then see if it's raced or not in order to avoid introducing mutex.

> ---

You'd typically want to include a brief description of what changed from
v1->v2 here, as a hint to reviewers re: what to concentrate on.

> +static int alloc_pdir(struct smmu_as *as, unsigned long *flags)

> +	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 = -ENODEV;
> +		goto err_out;
>  	}

Is that really an error; it just means that something else allocated the
same pdir already. Since the top of the function does:

 	if (as->pdir_page)
 		return 0;

I'd expect to s/err = -ENODEV/err = 0/ inside that if condition that I
quoted above, but still of cause "goto err_out" to free the now unneeded
allocations.

Aside from that, I think this looks reasonable.

Re: [v2 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Thu, 28 Jun 2012 18:57:18 +0200:

> On 06/28/2012 04:51 AM, Hiroshi DOYU wrote:
> > alloc_pdir() is called from smmu_iommu_domain_init() with spin_lock
> > held. memory allocations in alloc_pdir() had to be
> > atomic/unsleepable. Instead of converting into atomic allocation, this
> > patch once releases a lock, do the allocation, hold the lock again and
> > then see if it's raced or not in order to avoid introducing mutex.
> 
> > ---
> 
> You'd typically want to include a brief description of what changed from
> v1->v2 here, as a hint to reviewers re: what to concentrate on.

Ok, I'll.

> > +static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
> 
> > +	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 = -ENODEV;
> > +		goto err_out;
> >  	}
> 
> Is that really an error; it just means that something else allocated the
> same pdir already. Since the top of the function does:
> 
>  	if (as->pdir_page)
>  		return 0;
> 
> I'd expect to s/err = -ENODEV/err = 0/ inside that if condition that I
> quoted above, but still of cause "goto err_out" to free the now unneeded
> allocations.
> 
> Aside from that, I think this looks reasonable.

I think that, in the case of race condition, the one which comes
later, should retry with the another ASID, which is incremented as
below. So I modified that the latter one returns with -EAGAIN, and try
with another ASID.

The complete patch follows this mail.

Changes:
	Modified drivers/iommu/tegra-smmu.c
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ec656ec..f2c18fa 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -562,7 +562,7 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
 
 	if (as->pdir_page) {
 		/* We raced, free the redundant */
-		err = -ENODEV;
+		err = -EAGAIN;
 		goto err_out;
 	}
 
@@ -799,8 +799,15 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
 
 		spin_lock_irqsave(&tmp->lock, flags);
 		if (!tmp->pdir_page) {
-			as = tmp;
-			goto found;
+			int err;
+
+			err = alloc_pdir(tmp, &flags);
+			if (!err) {
+				as = tmp;
+				goto found;
+			}
+			if (err == -EAGAIN)
+				continue;
 		}
 		spin_unlock_irqrestore(&tmp->lock, flags);
 	}
@@ -808,9 +815,6 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
 	return -ENODEV;
 
 found:
-	if (alloc_pdir(as, &flags) < 0)
-		goto err_alloc_pdir;
-
 	spin_lock(&smmu->lock);
 
 	/* Update PDIR register */
@@ -826,10 +830,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)

Re: [v2 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Stephen Warren]

On 06/28/2012 12:35 PM, Hiroshi Doyu wrote:
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Thu, 28 Jun 2012 18:57:18 +0200:
>> On 06/28/2012 04:51 AM, Hiroshi DOYU wrote:
>>> alloc_pdir() is called from smmu_iommu_domain_init() with spin_lock
>>> held. memory allocations in alloc_pdir() had to be
>>> atomic/unsleepable. Instead of converting into atomic allocation, this
>>> patch once releases a lock, do the allocation, hold the lock again and
>>> then see if it's raced or not in order to avoid introducing mutex.

>>> +static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
>>
>>> +	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 = -ENODEV;
>>> +		goto err_out;
>>>  	}
>>
>> Is that really an error; it just means that something else allocated the
>> same pdir already. Since the top of the function does:
>>
>>  	if (as->pdir_page)
>>  		return 0;
>>
>> I'd expect to s/err = -ENODEV/err = 0/ inside that if condition that I
>> quoted above, but still of cause "goto err_out" to free the now unneeded
>> allocations.
>>
>> Aside from that, I think this looks reasonable.
> 
> I think that, in the case of race condition, the one which comes
> later, should retry with the another ASID, which is incremented as
> below. So I modified that the latter one returns with -EAGAIN, and try
> with another ASID.
> 
> The complete patch follows this mail.

incremental rather than complete, right?

> 
> Changes:
> 	Modified drivers/iommu/tegra-smmu.c
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index ec656ec..f2c18fa 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -562,7 +562,7 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
>  
>  	if (as->pdir_page) {
>  		/* We raced, free the redundant */
> -		err = -ENODEV;
> +		err = -EAGAIN;
>  		goto err_out;
>  	}
>  
> @@ -799,8 +799,15 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
>  
>  		spin_lock_irqsave(&tmp->lock, flags);
>  		if (!tmp->pdir_page) {
> -			as = tmp;
> -			goto found;
> +			int err;
> +
> +			err = alloc_pdir(tmp, &flags);
> +			if (!err) {
> +				as = tmp;
> +				goto found;
> +			}
> +			if (err == -EAGAIN)
> +				continue;

That loop is going to continue anyway, since that code is right at the
end of the loop. Don't you want to replace that if block with:

if (err != -EAGAIN)
    goto err_alloc_pdir;

?

Also, the first thinig that alloc_pdir does is:

        if (as->pdir_page)
                return 0;

It seems that should be removed completely, right? Since having the
pdir_page already allocated is an error.

>  		}
>  		spin_unlock_irqrestore(&tmp->lock, flags);
>  	}
> @@ -808,9 +815,6 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
>  	return -ENODEV;
>  
>  found:
> -	if (alloc_pdir(as, &flags) < 0)
> -		goto err_alloc_pdir;
> -
>  	spin_lock(&smmu->lock);
>  
>  	/* Update PDIR register */
> @@ -826,10 +830,6 @@ found:
>  
>  	dev_dbg(smmu->dev, "smmu_as@%p\n", as);
>  	return 0;
> -
> -err_alloc_pdir:
> -	spin_unlock_irqrestore(&as->lock, flags);
> -	return -ENODEV;
>  }

Re: [v2 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Hiroshi Doyu]

Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Thu, 28 Jun 2012 21:08:12 +0200:

> On 06/28/2012 12:35 PM, Hiroshi Doyu wrote:
> > Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote @ Thu, 28 Jun 2012 18:57:18 +0200:
> >> On 06/28/2012 04:51 AM, Hiroshi DOYU wrote:
> >>> alloc_pdir() is called from smmu_iommu_domain_init() with spin_lock
> >>> held. memory allocations in alloc_pdir() had to be
> >>> atomic/unsleepable. Instead of converting into atomic allocation, this
> >>> patch once releases a lock, do the allocation, hold the lock again and
> >>> then see if it's raced or not in order to avoid introducing mutex.
> 
> >>> +static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
> >>
> >>> +	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 = -ENODEV;
> >>> +		goto err_out;
> >>>  	}
> >>
> >> Is that really an error; it just means that something else allocated the
> >> same pdir already. Since the top of the function does:
> >>
> >>  	if (as->pdir_page)
> >>  		return 0;
> >>
> >> I'd expect to s/err = -ENODEV/err = 0/ inside that if condition that I
> >> quoted above, but still of cause "goto err_out" to free the now unneeded
> >> allocations.
> >>
> >> Aside from that, I think this looks reasonable.
> > 
> > I think that, in the case of race condition, the one which comes
> > later, should retry with the another ASID, which is incremented as
> > below. So I modified that the latter one returns with -EAGAIN, and try
> > with another ASID.
> > 
> > The complete patch follows this mail.
> 
> incremental rather than complete, right?

Actually I was working on it.

> > Changes:
> > 	Modified drivers/iommu/tegra-smmu.c
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > index ec656ec..f2c18fa 100644
> > --- a/drivers/iommu/tegra-smmu.c
> > +++ b/drivers/iommu/tegra-smmu.c
> > @@ -562,7 +562,7 @@ static int alloc_pdir(struct smmu_as *as, unsigned long *flags)
> >  
> >  	if (as->pdir_page) {
> >  		/* We raced, free the redundant */
> > -		err = -ENODEV;
> > +		err = -EAGAIN;
> >  		goto err_out;
> >  	}
> >  
> > @@ -799,8 +799,15 @@ static int smmu_iommu_domain_init(struct iommu_domain *domain)
> >  
> >  		spin_lock_irqsave(&tmp->lock, flags);
> >  		if (!tmp->pdir_page) {
> > -			as = tmp;
> > -			goto found;
> > +			int err;
> > +
> > +			err = alloc_pdir(tmp, &flags);
> > +			if (!err) {
> > +				as = tmp;
> > +				goto found;
> > +			}
> > +			if (err == -EAGAIN)
> > +				continue;
> 
> That loop is going to continue anyway, since that code is right at the
> end of the loop. Don't you want to replace that if block with:
> 
> if (err != -EAGAIN)
>     goto err_alloc_pdir;
> 
> ?

Yes, I also found. I'll send the update one tomorrow.

> Also, the first thinig that alloc_pdir does is:
> 
>         if (as->pdir_page)
>                 return 0;
> 
> It seems that should be removed completely, right? Since having the
> pdir_page already allocated is an error.

Yes. I thought that could be another patch. Same for the original code.

[v3 1/2] iommu/tegra: smmu: Remove unnecessary sanity check at alloc_pdir()

[Hiroshi DOYU]

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 <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ecd6790..532c8a4 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -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) {
-- 
1.7.5.4

[v3 2/2] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

[Hiroshi DOYU]

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 <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Reported-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
Cc: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
---
v3:
Added -EAGAIN in alloc_pdir when raced to retry.
---
 drivers/iommu/tegra-smmu.c |   77 +++++++++++++++++++++++++------------------
 1 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 532c8a4..dbba94c 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -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)
-- 
1.7.5.4

Re: [v3 1/2] iommu/tegra: smmu: Remove unnecessary sanity check at alloc_pdir()

[Stephen Warren]

On 06/29/2012 12:15 AM, Hiroshi DOYU wrote:
> alloc_pdir() is called with smmu->as[?].pdir_page == NULL. No need to
> check pdir_page again inside alloc_pdir().

Both patches,
Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

Re: [PATCH 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation

[Joerg Roedel]

On Wed, Jun 27, 2012 at 12:54:01PM +0300, Hiroshi DOYU wrote:
> allo_pdir() is called in smmu_iommu_domain_init() with spin_lock
> held. memory allocations in it have to be atomic/unsleepable.
> 
> Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Reported-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/iommu/tegra-smmu.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index ecd6790..3f3d09d 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -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_KERNEL);
> +		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_ATOMIC);
>  	if (!as->pte_count) {
>  		dev_err(smmu->dev,
>  			"failed to allocate smmu_device PTE cunters\n");
>  		return -ENOMEM;
>  	}
> -	as->pdir_page = alloc_page(GFP_KERNEL | __GFP_DMA);
> +	as->pdir_page = alloc_page(GFP_ATOMIC | __GFP_DMA);
>  	if (!as->pdir_page) {
>  		dev_err(smmu->dev,
>  			"failed to allocate smmu_device page directory\n");

Okay, I queued this one for v3.5. It is small enough so that it counts
as a fix. The other two patches are 3.6 stuff.


	Joerg

-- 
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

Re: [PATCH 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation

[Hiroshi Doyu]

Hi Joerg,

Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org> wrote @ Mon, 2 Jul 2012 11:57:47 +0200:

> On Wed, Jun 27, 2012 at 12:54:01PM +0300, Hiroshi DOYU wrote:
> > allo_pdir() is called in smmu_iommu_domain_init() with spin_lock
> > held. memory allocations in it have to be atomic/unsleepable.
> > 
> > Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Reported-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
> > ---
> >  drivers/iommu/tegra-smmu.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > index ecd6790..3f3d09d 100644
> > --- a/drivers/iommu/tegra-smmu.c
> > +++ b/drivers/iommu/tegra-smmu.c
> > @@ -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_KERNEL);
> > +		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_ATOMIC);
> >  	if (!as->pte_count) {
> >  		dev_err(smmu->dev,
> >  			"failed to allocate smmu_device PTE cunters\n");
> >  		return -ENOMEM;
> >  	}
> > -	as->pdir_page = alloc_page(GFP_KERNEL | __GFP_DMA);
> > +	as->pdir_page = alloc_page(GFP_ATOMIC | __GFP_DMA);
> >  	if (!as->pdir_page) {
> >  		dev_err(smmu->dev,
> >  			"failed to allocate smmu_device page directory\n");
> 
> Okay, I queued this one for v3.5. It is small enough so that it counts
> as a fix. The other two patches are 3.6 stuff.

The other 2 patehs are the replacement of this one. They are
exclusive. Both of them are basically for the same fix.

Re: [PATCH 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation

[Hiroshi Doyu]

Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Mon, 2 Jul 2012 12:13:26 +0200:

> Hi Joerg,
> 
> Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org> wrote @ Mon, 2 Jul 2012 11:57:47 +0200:
> 
> > On Wed, Jun 27, 2012 at 12:54:01PM +0300, Hiroshi DOYU wrote:
> > > allo_pdir() is called in smmu_iommu_domain_init() with spin_lock
> > > held. memory allocations in it have to be atomic/unsleepable.
> > > 
> > > Signed-off-by: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > Reported-by: Chris Wright <chrisw-69jw2NvuJkxg9hUCZPvPmw@public.gmane.org>
> > > ---
> > >  drivers/iommu/tegra-smmu.c |    4 ++--
> > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > > index ecd6790..3f3d09d 100644
> > > --- a/drivers/iommu/tegra-smmu.c
> > > +++ b/drivers/iommu/tegra-smmu.c
> > > @@ -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_KERNEL);
> > > +		     sizeof(as->pte_count[0]) * SMMU_PDIR_COUNT, GFP_ATOMIC);
> > >  	if (!as->pte_count) {
> > >  		dev_err(smmu->dev,
> > >  			"failed to allocate smmu_device PTE cunters\n");
> > >  		return -ENOMEM;
> > >  	}
> > > -	as->pdir_page = alloc_page(GFP_KERNEL | __GFP_DMA);
> > > +	as->pdir_page = alloc_page(GFP_ATOMIC | __GFP_DMA);
> > >  	if (!as->pdir_page) {
> > >  		dev_err(smmu->dev,
> > >  			"failed to allocate smmu_device page directory\n");
> > 
> > Okay, I queued this one for v3.5. It is small enough so that it counts
> > as a fix. The other two patches are 3.6 stuff.
> 
> The other 2 patehs are the replacement of this one. They are
> exclusive. Both of them are basically for the same fix.

I might not get your point. If you plan to take the this(the original)
as immediate fix for v3.5  you need tok take the others for v3.6 after
*reverting* the original. This would also work.

Re: [PATCH 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation

[joerg.roedel-5C7GfCeVMHo]

On Mon, Jul 02, 2012 at 12:19:03PM +0200, Hiroshi Doyu wrote:
> Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Mon, 2 Jul 2012 12:13:26 +0200:
> > The other 2 patehs are the replacement of this one. They are
> > exclusive. Both of them are basically for the same fix.
> 
> I might not get your point. If you plan to take the this(the original)
> as immediate fix for v3.5  you need tok take the others for v3.6 after
> *reverting* the original. This would also work.

Or you rebase the other two patches on-top of the first patch. Should
not be too difficult and you can also include the Ack from Steven while
at it :)


	Joerg

-- 
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

Re: [PATCH 1/1] iommu/tegra: smmu: Fix unsleepable memory allocation

[Hiroshi Doyu]

"joerg.roedel-5C7GfCeVMHo@public.gmane.org" <joerg.roedel-5C7GfCeVMHo@public.gmane.org> wrote @ Mon, 2 Jul 2012 12:42:31 +0200:

> On Mon, Jul 02, 2012 at 12:19:03PM +0200, Hiroshi Doyu wrote:
> > Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote @ Mon, 2 Jul 2012 12:13:26 +0200:
> > > The other 2 patehs are the replacement of this one. They are
> > > exclusive. Both of them are basically for the same fix.
> > 
> > I might not get your point. If you plan to take the this(the original)
> > as immediate fix for v3.5  you need tok take the others for v3.6 after
> > *reverting* the original. This would also work.
> 
> Or you rebase the other two patches on-top of the first patch. Should
> not be too difficult and you can also include the Ack from Steven while
> at it :)

I'm sending the following patch again. The first one is for v3.5. The
rest goes to v3.6 as Joerg suggested.

  v3.5
	[1/1] iommu/tegra: smmu: Fix unsleepable memory allocation
  v3.6
	[1/3] Revert "iommu/tegra: smmu: Fix unsleepable memory allocation"
	[2/3] iommu/tegra: smmu: Remove unnecessary sanity check at alloc_pdir()
	[3/3] iommu/tegra: smmu: Fix unsleepable memory allocation at alloc_pdir()

Is this OK?