Re: [RFC v5 3/3] intel_iommu: Do not notify regular iotlb to device-iotlb notifiers

[Peter Xu <peterx@redhat.com>]

On Mon, Aug 24, 2020 at 12:47:38PM +0200, Eugenio Pérez wrote:
> This improves performance in case of netperf with vhost-net:
> * TCP_STREAM: From 1374.81Mbit/s to 1845Mbit/s (37%)
> * TCP_RR: From 6559.36 trans/s to 7916.29/s (21%)
> * UDP_RR: From 6705.39 trans/s to 8199.39/s (22%)
> * UDP_STREAM: No change observed (not significant 0.1% improvement)

Good to know that we can get such a perf boost by removing the extra
invalidations!

> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 2ad6b9d796..d539a9f281 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -1959,6 +1959,12 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id)
>      vtd_iommu_unlock(s);
>  
>      QLIST_FOREACH(vtd_as, &s->vtd_as_with_notifiers, next) {
> +        if (vtd_as->iommu.iommu_notify_flags & IOMMU_NOTIFIER_DEVIOTLB) {
> +            /* If IOMMU memory region is DEVICE IOTLB type, it does not make
> +             * sense to send regular IOMMU notifications. */
> +            continue;
> +        }

Though here IMHO a cleaner approach (rather than checking explicitly against
DEVIOTLB flag) is to add a notification type into IOMMUTLBEntry.

Then for domain invalidation, the caller is responsible to pass the type
IOMMU_NOTIFIER_UNMAP into the type field. memory_region_notify_iommu_one() will
automatically skip the message if not registerd by the notifier.

Also, it would be nice to have this new type before or when introducing the
DEVIOTLB message, otherwise the DEVIOTLB patch can be still slightly confusing
by itself when notified as UNMAP.

So here's some patch layout I'm thinking:

  - patch 1: rename function, we can keep it

  - patch 2: introduce "type" for IOMMUTLBEntry, so far we only have MAP/UNMAP.
    Modify all callers of memory_region_notify_iommu*() to fill in the type
    correctly into IOMMUTLBEntry with map/unmap.  Won't hurt if we still keep
    filling in UNMAP even for DEVIOTLB since that's after all the same old
    behavior.

  - patch 3: introduce DEVIOTLB, switch the type field of dev-iotlb
    notifications to the new type and let vhost registers only to that.

  - patch 4: at last, fix the assertion since we've got the DEVIOTLB messages.

Would this be slightly cleaner?

Thanks,

-- 
Peter Xu