>-----Original Message-----
>From: Duan, Zhenzhong
>Sent: Friday, June 9, 2023 12:02 PM
>To: Peter Xu <[email protected]>
>Cc: [email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected];
>[email protected]; [email protected]; [email protected];
>[email protected]; [email protected]; Liu, Yi L <[email protected]>; Peng,
>Chao P <[email protected]>
>Subject: RE: [PATCH v3 5/5] intel_iommu: Optimize out some unnecessary
>UNMAP calls
>
>
>
>>-----Original Message-----
>>From: Peter Xu <[email protected]>
>>Sent: Friday, June 9, 2023 4:34 AM
>>To: Duan, Zhenzhong <[email protected]>
>>Cc: [email protected]; [email protected]; [email protected];
>>[email protected]; [email protected];
>[email protected];
>>[email protected]; [email protected];
>[email protected];
>>[email protected]; [email protected]; [email protected];
>>[email protected]; Liu, Yi L <[email protected]>; Peng, Chao P
>><[email protected]>
>>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
