KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
From: Marc Zyngier <marc.zyngier@arm.com>
To: Julien Thierry <julien.thierry@arm.com>
Cc: kvm@vger.kernel.org, "Raslan, KarimAllah" <karahmed@amazon.de>,
	"Saidi, Ali" <alisaidi@amazon.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 1/9] KVM: arm/arm64: vgic: Add LPI translation cache definition
Date: Wed, 12 Jun 2019 10:52:25 +0100
Message-ID: <86ef3zgmg6.wl-marc.zyngier@arm.com> (raw)
In-Reply-To: <54c8547a-51fb-8ae5-975f-261d3934221a@arm.com>

Hi Julien,

On Wed, 12 Jun 2019 09:16:21 +0100,
Julien Thierry <julien.thierry@arm.com> wrote:
> 
> Hi Marc,
> 
> On 11/06/2019 18:03, Marc Zyngier wrote:
> > Add the basic data structure that expresses an MSI to LPI
> > translation as well as the allocation/release hooks.
> > 
> > THe size of the cache is arbitrarily defined as 4*nr_vcpus.
> >
> 
> The size has been arbitrarily changed to 16*nr_vcpus :) .

Well spotted! ;-)

> 
> Nit: The*

Ah, usual lazy finger on the Shift key... One day I'll learn to type.

> 
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> >  include/kvm/arm_vgic.h        |  3 +++
> >  virt/kvm/arm/vgic/vgic-init.c |  5 ++++
> >  virt/kvm/arm/vgic/vgic-its.c  | 49 +++++++++++++++++++++++++++++++++++
> >  virt/kvm/arm/vgic/vgic.h      |  2 ++
> >  4 files changed, 59 insertions(+)
> > 
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index c36c86f1ec9a..ca7bcf52dc85 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -260,6 +260,9 @@ struct vgic_dist {
> >  	struct list_head	lpi_list_head;
> >  	int			lpi_list_count;
> >  
> > +	/* LPI translation cache */
> > +	struct list_head	lpi_translation_cache;
> > +
> >  	/* used by vgic-debug */
> >  	struct vgic_state_iter *iter;
> >  
> > diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> > index 3bdb31eaed64..c7c4c77dd430 100644
> > --- a/virt/kvm/arm/vgic/vgic-init.c
> > +++ b/virt/kvm/arm/vgic/vgic-init.c
> > @@ -64,6 +64,7 @@ void kvm_vgic_early_init(struct kvm *kvm)
> >  	struct vgic_dist *dist = &kvm->arch.vgic;
> >  
> >  	INIT_LIST_HEAD(&dist->lpi_list_head);
> > +	INIT_LIST_HEAD(&dist->lpi_translation_cache);
> >  	raw_spin_lock_init(&dist->lpi_list_lock);
> >  }
> >  
> > @@ -305,6 +306,7 @@ int vgic_init(struct kvm *kvm)
> >  	}
> >  
> >  	if (vgic_has_its(kvm)) {
> > +		vgic_lpi_translation_cache_init(kvm);
> >  		ret = vgic_v4_init(kvm);
> >  		if (ret)
> >  			goto out;
> > @@ -346,6 +348,9 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
> >  		INIT_LIST_HEAD(&dist->rd_regions);
> >  	}
> >  
> > +	if (vgic_has_its(kvm))
> > +		vgic_lpi_translation_cache_destroy(kvm);
> > +
> >  	if (vgic_supports_direct_msis(kvm))
> >  		vgic_v4_teardown(kvm);
> >  }
> > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> > index 44ceaccb18cf..ce9bcddeb7f1 100644
> > --- a/virt/kvm/arm/vgic/vgic-its.c
> > +++ b/virt/kvm/arm/vgic/vgic-its.c
> > @@ -149,6 +149,14 @@ struct its_ite {
> >  	u32 event_id;
> >  };
> >  
> > +struct vgic_translation_cache_entry {
> > +	struct list_head	entry;
> > +	phys_addr_t		db;
> > +	u32			devid;
> > +	u32			eventid;
> > +	struct vgic_irq		*irq;
> > +};
> > +
> >  /**
> >   * struct vgic_its_abi - ITS abi ops and settings
> >   * @cte_esz: collection table entry size
> > @@ -1668,6 +1676,45 @@ static int vgic_register_its_iodev(struct kvm *kvm, struct vgic_its *its,
> >  	return ret;
> >  }
> >  
> > +/* Default is 16 cached LPIs per vcpu */
> > +#define LPI_DEFAULT_PCPU_CACHE_SIZE	16
> > +
> > +void vgic_lpi_translation_cache_init(struct kvm *kvm)
> > +{
> > +	struct vgic_dist *dist = &kvm->arch.vgic;
> > +	unsigned int sz;
> > +	int i;
> > +
> > +	if (!list_empty(&dist->lpi_translation_cache))
> > +		return;
> > +
> > +	sz = atomic_read(&kvm->online_vcpus) * LPI_DEFAULT_PCPU_CACHE_SIZE;
> > +
> > +	for (i = 0; i < sz; i++) {
> > +		struct vgic_translation_cache_entry *cte;
> > +
> > +		/* An allocation failure is not fatal */
> > +		cte = kzalloc(sizeof(*cte), GFP_KERNEL);
> > +		if (WARN_ON(!cte))
> > +			break;
> > +
> > +		INIT_LIST_HEAD(&cte->entry);
> > +		list_add(&cte->entry, &dist->lpi_translation_cache);
> 
> Going through the series, it looks like this list is either empty
> (before the cache init) or has a fixed number
> (LPI_DEFAULT_PCPU_CACHE_SIZE * nr_cpus) of entries.

Well, it could also fail when allocating one of the entry, meaning we
can have an allocation ranging from 0 to (LPI_DEFAULT_PCPU_CACHE_SIZE
* nr_cpus) entries.

> And the list never grows nor shrinks throughout the series, so it
> seems odd to be using a list here.
> 
> Is there a reason for not using a dynamically allocated array instead of
> the list? (does list_move() provide a big perf advantage over swapping
> the data from one array entry to another? Or is there some other
> facility I am missing?

The idea was to make the LRU policy cheap, on the assumption that
list_move (which is only a couple of pointer updates) is cheaper than
a memmove if you want to keep the array ordered. If we exclude the
list head, we end-up with 24 bytes per entry to move down to make room
for the new entry at the head of the array. For large caches that miss
very often, this will hurt badly. But is that really a problem? I
don't know.

We could allocate an array as you suggest, and use a linked list
inside the array. Or something else. I'm definitely open to
suggestion!

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

  parent reply index

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-11 17:03 [PATCH v2 0/9] KVM: arm/arm64: vgic: ITS translation cache Marc Zyngier
2019-06-11 17:03 ` [PATCH v2 1/9] KVM: arm/arm64: vgic: Add LPI translation cache definition Marc Zyngier
2019-06-12  8:16   ` Julien Thierry
2019-06-12  8:49     ` Julien Thierry
2019-06-12  9:52     ` Marc Zyngier [this message]
2019-06-12 10:58       ` Julien Thierry
2019-06-12 12:28         ` Julien Thierry
2019-07-23 12:43   ` Auger Eric
2019-06-11 17:03 ` [PATCH v2 2/9] KVM: arm/arm64: vgic: Add __vgic_put_lpi_locked primitive Marc Zyngier
2019-06-11 17:03 ` [PATCH v2 3/9] KVM: arm/arm64: vgic-its: Add MSI-LPI translation cache invalidation Marc Zyngier
2019-07-23 12:39   ` Auger Eric
2019-06-11 17:03 ` [PATCH v2 4/9] KVM: arm/arm64: vgic-its: Invalidate MSI-LPI translation cache on specific commands Marc Zyngier
2019-07-01 12:38   ` Auger Eric
2019-07-22 10:54     ` Marc Zyngier
2019-07-23 12:25       ` Auger Eric
2019-07-23 12:43         ` Marc Zyngier
2019-07-23 12:47           ` Auger Eric
2019-07-23 12:50             ` Marc Zyngier
2019-06-11 17:03 ` [PATCH v2 5/9] KVM: arm/arm64: vgic-its: Invalidate MSI-LPI translation cache on disabling LPIs Marc Zyngier
2019-07-23 15:09   ` Auger Eric
2019-06-11 17:03 ` [PATCH v2 6/9] KVM: arm/arm64: vgic-its: Invalidate MSI-LPI translation cache on vgic teardown Marc Zyngier
2019-07-23 15:10   ` Auger Eric
2019-06-11 17:03 ` [PATCH v2 7/9] KVM: arm/arm64: vgic-its: Cache successful MSI->LPI translation Marc Zyngier
2019-06-25 11:50   ` Zenghui Yu
2019-06-25 12:31     ` Marc Zyngier
2019-06-25 16:00       ` Zenghui Yu
2019-06-26  3:54         ` Zenghui Yu
2019-06-26  7:55         ` Marc Zyngier
2019-07-23 15:21   ` Auger Eric
2019-06-11 17:03 ` [PATCH v2 8/9] KVM: arm/arm64: vgic-its: Check the LPI translation cache on MSI injection Marc Zyngier
2019-07-23 15:10   ` Auger Eric
2019-07-23 15:45     ` Marc Zyngier
2019-07-24  7:41       ` Auger Eric
2019-06-11 17:03 ` [PATCH v2 9/9] KVM: arm/arm64: vgic-irqfd: Implement kvm_arch_set_irq_inatomic Marc Zyngier
2019-07-23 15:14   ` Auger Eric
2019-07-25  8:24     ` Marc Zyngier
2019-07-23 11:14 ` [PATCH v2 0/9] KVM: arm/arm64: vgic: ITS translation cache Andre Przywara
2019-07-25  8:50   ` Marc Zyngier
2019-07-25 10:01     ` Andre Przywara
2019-07-25 15:37       ` Marc Zyngier

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=86ef3zgmg6.wl-marc.zyngier@arm.com \
    --to=marc.zyngier@arm.com \
    --cc=alisaidi@amazon.com \
    --cc=julien.thierry@arm.com \
    --cc=karahmed@amazon.de \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM ARM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvmarm/0 kvmarm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvmarm kvmarm/ https://lore.kernel.org/kvmarm \
		kvmarm@lists.cs.columbia.edu kvmarm@archiver.kernel.org
	public-inbox-index kvmarm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/edu.columbia.cs.lists.kvmarm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox