RE: [PATCH v3 5/5] intel_iommu: Optimize out some unnecessary UNMAP calls

[Duan, Zhenzhong]

>-----Original Message-----
>From: Duan, Zhenzhong
>Sent: Friday, June 9, 2023 12:02 PM
>To: Peter Xu <peterx@redhat.com>
>Cc: qemu-devel@nongnu.org; mst@redhat.com; jasowang@redhat.com;
>pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net;
>marcel.apfelbaum@gmail.com; alex.williamson@redhat.com;
>clg@redhat.com; david@redhat.com; philmd@linaro.org;
>kwankhede@nvidia.com; cjia@nvidia.com; Liu, Yi L <yi.l.liu@intel.com>; Peng,
>Chao P <chao.p.peng@intel.com>
>Subject: RE: [PATCH v3 5/5] intel_iommu: Optimize out some unnecessary
>UNMAP calls
>
>
>
>>-----Original Message-----
>>From: Peter Xu <peterx@redhat.com>
>>Sent: Friday, June 9, 2023 4:34 AM
>>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>>Cc: qemu-devel@nongnu.org; mst@redhat.com; jasowang@redhat.com;
>>pbonzini@redhat.com; richard.henderson@linaro.org;
>eduardo@habkost.net;
>>marcel.apfelbaum@gmail.com; alex.williamson@redhat.com;
>clg@redhat.com;
>>david@redhat.com; philmd@linaro.org; kwankhede@nvidia.com;
>>cjia@nvidia.com; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao P
>><chao.p.peng@intel.com>
>>Subject: Re: [PATCH v3 5/5] intel_iommu: Optimize out some unnecessary
>>UNMAP calls
>>
>>On Thu, Jun 08, 2023 at 05:52:31PM +0800, Zhenzhong Duan wrote:
>>>      while (remain >= VTD_PAGE_SIZE) {
>>> -        IOMMUTLBEvent event;
>>>          uint64_t mask = dma_aligned_pow2_mask(start, end, s->aw_bits);
>>>          uint64_t size = mask + 1;
>>>
>>>          assert(size);
>>>
>>> -        event.type = IOMMU_NOTIFIER_UNMAP;
>>> -        event.entry.iova = start;
>>> -        event.entry.addr_mask = mask;
>>> -        event.entry.target_as = &address_space_memory;
>>> -        event.entry.perm = IOMMU_NONE;
>>> -        /* This field is meaningless for unmap */
>>> -        event.entry.translated_addr = 0;
>>> -
>>> -        memory_region_notify_iommu_one(n, &event);
>>> +        map.iova = start;
>>> +        map.size = mask;
>>> +        if (iova_tree_find(as->iova_tree, &map)) {
>>> +            event.entry.iova = start;
>>> +            event.entry.addr_mask = mask;
>>> +            memory_region_notify_iommu_one(n, &event);
>>> +        }
>>
>>Ah one more thing: I think this path can also be triggered by notifiers
>>without MAP event registered, whose iova tree will always be empty.  So
>>we may only do this for MAP, then I'm not sure whether it'll be worthwhile..
>
>Hmm, yes, my change will lead to vhost missed to receive some invalidation
>request in device-tlb disabled case as iova tree is empty. Thanks for point 
>out.
>
>Let me collect time diff spent in unmap AS for you to decide if it still worth 
>or
>not.
I used below changes to collect time spent:

@@ -3739,12 +3739,14 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, 
PCIBus *bus,
 /* Unmap the whole range in the notifier's scope. */
 static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
 {
+    int64_t start_tv, delta_tv;
     hwaddr size, remain;
     hwaddr start = n->start;
     hwaddr end = n->end;
     IntelIOMMUState *s = as->iommu_state;
     DMAMap map;

+    start_tv = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
     /*
      * Note: all the codes in this function has a assumption that IOVA
      * bits are no more than VTD_MGAW bits (which is restricted by
@@ -3793,6 +3795,9 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, 
IOMMUNotifier *n)
     map.iova = n->start;
     map.size = size;
     iova_tree_remove(as->iova_tree, map);
+
+    delta_tv = qemu_clock_get_us(QEMU_CLOCK_REALTIME) - start_tv;
+    printf("************ delta_tv %lu us **************\n", delta_tv);
 }

With legacy container(vfio-pci,host=81:11.1,id=vfio1,bus=root1)
Hotplug:
************ delta_tv 12 us **************
************ delta_tv 8 us **************
Unplug:
************ delta_tv 12 us **************
************ delta_tv 3 us **************

After fix:
Hotplug: empty
Unplug:
************ delta_tv 2 us **************
************ delta_tv 1 us **************

With iommufd 
container(vfio-pci,host=81:11.1,id=vfio1,bus=root1,iommufd=iommufd0)
Hotplug:
************ delta_tv 25 us **************
************ delta_tv 23 us ************** 
Unplug:
************ delta_tv 15 us **************
************ delta_tv 5 us **************

After fix:
Hotplug: empty
Unplug:
************ delta_tv 2 us **************
************ delta_tv 1 us **************

It looks the benefit of this patch is negligible for legacy container and 
iommufd.
I'd like to drop this patch as it makes no difference, your opinion?

Thanks
Zhenzhong