Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6cb75bd724f45bca8371235fd51132e987fb0f8a.camel@linux.ibm.com>
Subject: Re: [PATCH v4 5/7] iommu/dma: Allow a single FQ in addition to
 per-CPU FQs
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
        Robin Murphy <robin.murphy@arm.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        Wenjia Zhang <wenjia@linux.ibm.com>
Cc:     Matthew Rosato <mjrosato@linux.ibm.com>,
        Gerd Bayer <gbayer@linux.ibm.com>,
        Pierre Morel <pmorel@linux.ibm.com>, iommu@lists.linux.dev,
        linux-s390@vger.kernel.org, borntraeger@linux.ibm.com,
        hca@linux.ibm.com, gor@linux.ibm.com,
        gerald.schaefer@linux.ibm.com, agordeev@linux.ibm.com,
        svens@linux.ibm.com, linux-kernel@vger.kernel.org,
        Julian Ruess <julianr@linux.ibm.com>
Date:   Thu, 19 Jan 2023 17:16:11 +0100
In-Reply-To: <5692d7fbc3d4bd2ac624c82a77f642c36a2cf04e.camel@linux.ibm.com>
References: <20230104120543.308933-1-schnelle@linux.ibm.com>
         <20230104120543.308933-6-schnelle@linux.ibm.com>
         <5692d7fbc3d4bd2ac624c82a77f642c36a2cf04e.camel@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk

On Thu, 2023-01-19 at 16:55 +0100, Niklas Schnelle wrote:
> On Wed, 2023-01-04 at 13:05 +0100, Niklas Schnelle wrote:
> > In some virtualized environments, including s390 paged memory guests,
> > IOTLB flushes are used to update IOMMU shadow tables. Due to this, they
> > are much more expensive than in typical bare metal environments or
> > non-paged s390 guests. In addition they may parallelize more poorly in
> > virtualized environments. This changes the trade off for flushing IOVAs
> > such that minimizing the number of IOTLB flushes trumps any benefit of
> > cheaper queuing operations or increased paralellism.
> >=20
> > In this scenario per-CPU flush queues pose several problems. Firstly
> > per-CPU memory is often quite limited prohibiting larger queues.
> > Secondly collecting IOVAs per-CPU but flushing via a global timeout
> > reduces the number of IOVAs flushed for each timeout especially on s390
> > where PCI interrupts may not be bound to a specific CPU.
> >=20
> > Thus let's introduce a single flush queue mode IOMMU_DOMAIN_DMA_SQ that
> > reuses the same queue logic but only allocates a single global queue
> > allowing larger batches of IOVAs to be freed at once and with larger
> > timeouts. This is to allow the common IOVA flushing code to more closely
> > resemble the global flush behavior used on s390's previous internal DMA
> > API implementation.
> >=20
> > As we now support two different variants of flush queues rename the
> > existing __IOMMU_DOMAIN_DMA_FQ to __IOMMU_DOMAIN_DMA_LAZY to indicate
> > the general case of having a flush queue and introduce separate
> > __IOMMU_DOMAIN_DMA_PERCPU_Q and __IOMMU_DOMAIN_DMA_SINGLE_Q bits to
> > indicate the two queue variants.
> >=20
> > Link: https://lore.kernel.org/linux-iommu/3e402947-61f9-b7e8-1414-fde00=
6257b6f@arm.com/
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> > v2 -> v3:
> > - Rename __IOMMU_DOMAIN_DMA_FQ to __IOMMU_DOMAIN_DMA_LAZY to make it mo=
re clear
> >   that this bit indicates flush queue use independent of the exact queu=
ing
> >   strategy
>=20
---8<---
>=20
> >=20=20
> > -		for (i =3D 0; i < IOVA_FQ_SIZE; i++)
> > -			INIT_LIST_HEAD(&fq->entries[i].freelist);
> > +	if (rc) {
> > +		pr_warn("iova flush queue initialization failed\n");
> > +		return rc;
> >  	}
> >=20
> ---8<---=20
> >=20
> >  	mutex_unlock(&group->mutex);
> > @@ -2896,10 +2900,10 @@ static int iommu_change_dev_def_domain(struct i=
ommu_group *group,
> >  	}
> >=20=20
> >  	/* We can bring up a flush queue without tearing down the domain */
> > -	if (type =3D=3D IOMMU_DOMAIN_DMA_FQ && prev_dom->type =3D=3D IOMMU_DO=
MAIN_DMA) {
> > +	if (!!(type & __IOMMU_DOMAIN_DMA_LAZY) && prev_dom->type =3D=3D IOMMU=
_DOMAIN_DMA) {
> >  		ret =3D iommu_dma_init_fq(prev_dom);
> >  		if (!ret)
> > -			prev_dom->type =3D IOMMU_DOMAIN_DMA_FQ;
> > +			prev_dom->type =3D type;
>=20
> Here domain->type is set only after calling iommu_dma_init_fq().

Actually I think even in the current code the above and the similar
code in iommu.c isn't ideal. When going from DMA to DMA-FQ with a bound
driver the flush queue is used from the moment that WRITE_ONCE(cookie-
>fq_domain, domain) executes in iommu_dma_init_fq() so there is a
window where the flush queue is already used but domain->type is still
DMA. By adding a type parameter to iommu_dma_init_fq() we can set
domain->type before the WRITE_ONCE() and thus close this window and it
even makes the callsites of iommu_dma_init_fq() simpler.

>=20
> >  		goto out;
> >  	}
> >=20=20
>=20