diff mbox

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

Message ID 20120628.213554.1189327754979133382.hdoyu@nvidia.com
State Not Applicable, archived
Headers show

Commit Message

Hiroshi Doyu June 28, 2012, 6:35 p.m. UTC 
Stephen Warren <swarren@wwwdotorg.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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Stephen Warren June 28, 2012, 7:08 p.m. UTC | #1 
On 06/28/2012 12:35 PM, Hiroshi Doyu wrote:
> Stephen Warren <swarren@wwwdotorg.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;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hiroshi Doyu June 28, 2012, 7:19 p.m. UTC | #2 
Stephen Warren <swarren@wwwdotorg.org> wrote @ Thu, 28 Jun 2012 21:08:12 +0200:

> On 06/28/2012 12:35 PM, Hiroshi Doyu wrote:
> > Stephen Warren <swarren@wwwdotorg.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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)