Re: [PATCH v2] iommu/iova: put free_iova_mem() outside of spinlock iova_rbtree_lock

[chenxiang (M)]

在 2021/4/15 17:49, John Garry 写道:
On 15/04/2021 04:52, chenxiang wrote:
From: Xiang Chen <chenxian...@hisilicon.com>

It is not necessary to put free_iova_mem() inside of spinlock/unlock
iova_rbtree_lock which only leads to more completion for the spinlock.
It has a small promote on the performance after the change. And also
rename private_free_iova() as remove_iova() because the function will 
not
free iova after that change.

Signed-off-by: Xiang Chen <chenxian...@hisilicon.com>
---
  drivers/iommu/iova.c | 13 +++++++------
  1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index b7ecd5b..c10af23 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -412,12 +412,11 @@ private_find_iova(struct iova_domain *iovad, 
unsigned long pfn)
      return NULL;
  }
  -static void private_free_iova(struct iova_domain *iovad, struct 
iova *iova)
+static void remove_iova(struct iova_domain *iovad, struct iova *iova)
  {
      assert_spin_locked(&iovad->iova_rbtree_lock);
      __cached_rbnode_delete_update(iovad, iova);
      rb_erase(&iova->node, &iovad->rbroot);
-    free_iova_mem(iova);
  }
    /**
@@ -452,8 +451,9 @@ __free_iova(struct iova_domain *iovad, struct 
iova *iova)
      unsigned long flags;
        spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
-    private_free_iova(iovad, iova);
+    remove_iova(iovad, iova);
      spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+    free_iova_mem(iova);
  }
  EXPORT_SYMBOL_GPL(__free_iova);
  @@ -473,9 +473,9 @@ free_iova(struct iova_domain *iovad, unsigned 
long pfn)
      spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
      iova = private_find_iova(iovad, pfn);
      if (iova)
-        private_free_iova(iovad, iova);
+        remove_iova(iovad, iova);
      spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
-
+    free_iova_mem(iova);

you need to make this:
    if (iova)
        free_iova_mem(iova);

as free_iova_mem(iova) dereferences iova:

if (iova->pfn_lo != IOVA_ANCHOR)
    kmem_cache_free(iova_cache, iova)

So if iova were NULL, we crash.

Or you can make free_iova_mem() NULL safe.

Right, it has the issue. What about changing it as below?

@@ -472,10 +472,13 @@ free_iova(struct iova_domain *iovad, unsigned long 
pfn)

        spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
        iova = private_find_iova(iovad, pfn);
-       if (iova)
-               private_free_iova(iovad, iova);
+       if (!iova) {
+ spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+               return;
+       }
+       remove_iova(iovad, iova);
        spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
-
+       free_iova_mem(iova);
 }
 EXPORT_SYMBOL_GPL(free_iova);


  }
  EXPORT_SYMBOL_GPL(free_iova);
  @@ -825,7 +825,8 @@ iova_magazine_free_pfns(struct iova_magazine 
*mag, struct iova_domain *iovad)
          if (WARN_ON(!iova))
              continue;
  -        private_free_iova(iovad, iova);
+        remove_iova(iovad, iova);
+        free_iova_mem(iova);
      }
        spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);



.



_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu