Re: [RFC PATCH v2 18/19] iommu/arm-smmu-v3: Reduce contention during command-queue insertion

From: John Garry <john.garry@huawei.com>
To: Will Deacon <will@kernel.org>
Cc: Vijay Kilary <vkilari@codeaurora.org>,
	Jean-Philippe Brucker <jean-philippe.brucker@arm.com>,
	Jon Masters <jcm@redhat.com>, Jan Glauber <jglauber@marvell.com>,
	Alex Williamson <alex.williamson@redhat.com>,
	iommu@lists.linux-foundation.org,
	Jayachandran Chandrasekharan Nair <jnair@marvell.com>,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [RFC PATCH v2 18/19] iommu/arm-smmu-v3: Reduce contention during command-queue insertion
Date: Thu, 25 Jul 2019 12:31:05 +0100	[thread overview]
Message-ID: <de49178c-0628-e759-fcc0-aabd7e86337c@huawei.com> (raw)
In-Reply-To: <20190724143355.r2zw6z37igwav2ki@willie-the-truck>

Hi Will,

>>> +		old = cmpxchg_relaxed(&cmdq->q.llq.val, llq.val, head.val);
>>
>> I added some basic debug to the stress test on your branch, and this cmpxchg
>> was failing ~10 times on average on my D06.
>>
>> So we're not using the spinlock now, but this cmpxchg may lack fairness.
>
> It definitely lacks fairness, although that's going to be the case in many
> other places where locking is elided as well. If it shows up as an issue, we
> should try to address it, but queueing means serialisation and that largely
> defeats the point of this patch. I also don't expect it to be a problem
> unless the cmpxchg() is heavily contended, which shouldn't be the case if
> we're batching.
>
>> Since we're batching commands, I wonder if it's better to restore the
>> spinlock and send batched commands + CMD_SYNC under the lock, and then wait
>> for the CMD_SYNC completion outside the lock.
>
> Again, we'd need some numbers, but my concern with that approach is that
> we're serialising CPUs which is what I've been trying hard to avoid.

That makes sense about the serialisation.

I'm just concerned with the scenario when the queue is heavily 
contented; here we may be gathering multiple batches of commands, and 
owners and non-owners a. may delay each other in writing the commands b. 
also pend the final CMD_SYNC consumption, which is for all gathered 
commands, and not just the commands which a particular CPU is interested in.

It
> also doesn't let you straightforwardly remove the cmpxchg() loop, since
> the owner clears the OWNED flag and you probably wouldn't want to hold
> the lock for that.
>
>> I don't know if it improves the queue contetion, but at least the prod
>> pointer would be more closely track the issued commands, such that we're not
>> waiting to kick off many gathered batches of commands, while the SMMU HW may
>> be idle (in terms of command processing).
>
> Again, probably going to need some numbers to change this, although it
> sounds like your other suggestion about having the owner move prod twice
> would largely address your concerns.

Limited initial testing shows that it makes little difference...

  Reintroducing the lock, on the other
> hand, feels like a big step backwards to me, and the whole reason I started
> down the current route was because of vague claims that the locking was a
> problem for large systems.

To me, the biggest improvement is batching of the commands, and it may 
not make much difference either way in how we deal with submitting them 
to the queue.

Cheers,
John

>
> Thanks,
>
> Will
>
> .
>

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu