Message ID | 20120628.213554.1189327754979133382.hdoyu@nvidia.com |
---|---|
State | Not Applicable, archived |
Headers | show |
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
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 --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)