Re: [PATCH v2 7/9] KVM: arm/arm64: vgic-its: Cache successful MSI->LPI translation

From: Marc Zyngier <marc.zyngier@arm.com>
To: Zenghui Yu <yuzenghui@huawei.com>,
	linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>,
	"Raslan, KarimAllah" <karahmed@amazon.de>,
	Julien Thierry <julien.thierry@arm.com>,
	Christoffer Dall <christoffer.dall@arm.com>,
	Eric Auger <eric.auger@redhat.com>,
	James Morse <james.morse@arm.com>,
	"Saidi, Ali" <alisaidi@amazon.com>
Subject: Re: [PATCH v2 7/9] KVM: arm/arm64: vgic-its: Cache successful MSI->LPI translation
Date: Tue, 25 Jun 2019 13:31:36 +0100	[thread overview]
Message-ID: <169cc847-ebfa-44b6-00e7-c69dccdbbd62@arm.com> (raw)
In-Reply-To: <53de88e9-3550-bd7f-8266-35c5e75fae4e@huawei.com>

Hi Zenghui,

On 25/06/2019 12:50, Zenghui Yu wrote:
> Hi Marc,
> 
> On 2019/6/12 1:03, Marc Zyngier wrote:
>> On a successful translation, preserve the parameters in the LPI
>> translation cache. Each translation is reusing the last slot
>> in the list, naturally evincting the least recently used entry.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>   virt/kvm/arm/vgic/vgic-its.c | 86 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 86 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 0aa0cbbc3af6..62932458476a 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -546,6 +546,90 @@ static unsigned long vgic_mmio_read_its_idregs(struct kvm *kvm,
>>   	return 0;
>>   }
>>   
>> +static struct vgic_irq *__vgic_its_check_cache(struct vgic_dist *dist,
>> +					       phys_addr_t db,
>> +					       u32 devid, u32 eventid)
>> +{
>> +	struct vgic_translation_cache_entry *cte;
>> +	struct vgic_irq *irq = NULL;
>> +
>> +	list_for_each_entry(cte, &dist->lpi_translation_cache, entry) {
>> +		/*
>> +		 * If we hit a NULL entry, there is nothing after this
>> +		 * point.
>> +		 */
>> +		if (!cte->irq)
>> +			break;
>> +
>> +		if (cte->db == db &&
>> +		    cte->devid == devid &&
>> +		    cte->eventid == eventid) {
>> +			/*
>> +			 * Move this entry to the head, as it is the
>> +			 * most recently used.
>> +			 */
>> +			list_move(&cte->entry, &dist->lpi_translation_cache);
> 
> Only for performance reasons: if we hit at the "head" of the list, we
> don't need to do a list_move().
> In our tests, we found that a single list_move() takes nearly (sometimes
> even more than) one microsecond, for some unknown reason...

Huh... That's odd.

Can you narrow down under which conditions this happens? I'm not sure if
checking for the list head would be more efficient, as you end-up
fetching the head anyway. Can you try replacing this line with:

	if (!list_is_first(&cte->entry, &dist->lpi_translation_cache))
		list_move(&cte->entry, &dist->lpi_translation_cache);

and let me know whether it helps?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel