Message-ID: <9d7819fd165a1e87ea1dd66f9d0d841f45012264.camel@linux.ibm.com>
Date:   Fri, 25 Aug 2023 14:22:38 +0200
From:   Niklas Schnelle <schnelle@...ux.ibm.com>
To:     Jean-Philippe Brucker <jean-philippe@...aro.org>
Cc:     Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        Matthew Rosato <mjrosato@...ux.ibm.com>,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: [RFC] iommu/virtio: Use single flush queue (EXPERIMENTAL)

On Wed, 2023-08-02 at 13:36 +0100, Jean-Philippe Brucker wrote:
> Hi Niklas,
> 
> On Wed, Jul 26, 2023 at 01:14:33PM +0200, Niklas Schnelle wrote:
> > Just like on paged s390 guests with their virtual IOMMU, syncing
> > mappings via virtio-iommu is quite expensive. It can thus benefit from
> > queueing unmapped IOVAs and flushing them in batches but less so from
> > parallel flushes which is what the shadow_on_flush flag introduced for
> > s390 tunes dma-iommu to do.
> > 
> > For this to work .flush_iotlb_all is implemented. Furthermore
> > .iotlb_sync_map is also implemented and used to pull the sync out of the
> > mapping operation for some additional batching and performance gain.
> > 
> > In a basic test with NVMe pass-through to a KVM guest on a Ryzen 3900X
> > these changes together lead to about 19% more IOPS in a fio test and
> > slightly more bandwidth too.
> 
> Nice, thank you for testing this. I played with a NVMe on an Intel desktop
> and can confirm similar results. With "sq" meaning single flush queue and
> "mq" percpu flush queue, "+map" is with .iotlb_sync_map() enabled.
> 
> Multithread block randwrite job [1]:
> 
>                          BW compared to host    Confidence
>                           (higher better)
>               host           100.0%               ±0.0%
>           noviommu            99.9                 0.0
>     viommu lazy sq +map       99.9                 0.1
>     viommu lazy mq +map       99.9                 0.1
>     viommu lazy sq            92.2                 0.9
>     viommu lazy mq            91.5                 0.9
>      viommu strict +map       92.7                 0.9
>      viommu strict            81.3                 1.0
> 
> 
> Single page randrw:
> 
>                         Latency compared to host    Confidence
>                            (lower better)
>               host            x1.00                   ±.04
>           noviommu             1.23                    .04
>     viommu lazy sq +map        7.09                    .05
>     viommu lazy mq +map        7.07                    .07
>     viommu lazy sq             7.15                    .04
>     viommu lazy mq             7.11                    .05
>      viommu strict +map        8.82                    .05
>      viommu strict             8.82                    .04
> 
> So with lazy+map we get the maximum bandwidth reachable on this disk
> (2.5GiB/s) even with a heavy iommu_map/unmap usage, which is cool.
> Random access latency also improves with lazy mode.
> 
> The difference between single and percpu flush queue isn't really
> measurable in my multithread test. There is a difference between Lazy sq
> and mq but the variation between samples outweighs it.

Interesting that there is no advantage when using a single large queue.
Maybe virtio-iommu does handle the per-CPU flushing well because of a
stronger CPU affinity than on s390x or maybe it has to do with the sync
being just a queue drain making only some syncs slow.

> 
> > 
> > Signed-off-by: Niklas Schnelle <schnelle@...ux.ibm.com>
> > ---
> > Note:
> > The idea of using the single flush queue scheme from my series "iommu/dma: s390
> > DMA API conversion and optimized IOTLB flushing"[0] for virtio-iommu was already
> > mentioned in the cover letter. I now wanted to explore this with this patch
> > which may also serve as a test vehicle for the single flush queue scheme usable
> > on non-s390.
> > 
> > Besides limited testing, this is marked experimental mainly because the use of
> > queuing needs to be a concious decision as it allows continued access to
> > unmapped pages for up to a second with the currently proposed single flush
> > queue mechanism.
> 
> It fits with the iommu.strict=0 / CONFIG_IOMMU_DEFAULT_DMA_LAZY setting,
> which selects DMA_FQ domains. That option allows a misbehaving device to
> access memory that has been freed/reallocated, which is what we're
> enabling here. I believe the risk is pretty much the same for deferred
> UNMAP as for deferred TLBI, since mappings that we're removing were likely
> cached in the TLB. Increasing the timeout does make it easier to exploit,
> but I don't think that changes the policy from an admin's perspective:
> only enable lazy mode if you trust device and driver.

I'm with Jason here. I think for non-CoCo hypervisor's
the hypervisor is trusted so may just forward the trust level of the
relevant device. This may of course be different for CoCo VMs but then
the VM knows and could e.g. default to iommu.strict=1 or it needs to
use swiotlb anyway.

> 
> On bare metal, we disable DMA_FQ for devices that can be easily hotplugged
> into unattended machines (through external-facing ports like thunderbolt).
> On VMs, the concern isn't really about external devices, since they don't
> automatically get plugged into a VM without user intervention. Here I
> guess the devices we don't trust will be virtual devices implemented by
> other VMs. We don't have any method to identify them yet, so
> iommu.strict=1 and CONFIG_IOMMU_DEFAULT_DMA_STRICT is the best we can do
> at the moment.
> 
> I'm not so sure about enabling shadow_on_flush by default, since the
> performance difference was too small in my tests. Maybe a module parameter
> for dma-iommu could configure the flush queue?
> 
> > Also it might make sense to split this patch to do the
> > introduction and use of .iotlb_sync_map separately but as a test vehicle
> > I found it easier to consume as a single patch.
> 
> Yes, both changes are useful but should be in two patches
> 
> Thanks,
> Jean
> 

After sending v12 of my series[0] I'm now looking into sending this
separately. Rebasing on v6.5-rc7 however I noticed that without my
patch "iommu: Allow .iotlb_sync_map to fail and handle s390's -ENOMEM
return" we not only get a conflict with my series but also potentially
miss errors on map since just like on s390x virtio-iommu's
.iotlb_sync_map can fail. So I think it makes most sense to send this
based on my series. I hope that's okay for you.

[0]
https://lore.kernel.org/lkml/20230825-dma_iommu-v12-0-4134455994a7@linux.ibm.com/

Powered by blists - more mailing lists