From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.7 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 76575C31E46 for ; Wed, 12 Jun 2019 09:52:41 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4D9CC20866 for ; Wed, 12 Jun 2019 09:52:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="W9AITljL" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4D9CC20866 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=W40G2t045Wejhc5VRjzufWxH5KVxouhWtkMR5lXVy9c=; b=W9AITljLfJdaOe Ywhpky0PHQ0I9TDr5nU1HCvm1lgGTNfS/olPNoffidrHD1Ve7eMBYCgUqGfZROgR2p4Y7uQmykXSd kktmIsZyCPdQzUrVzUz4ReRyiEPsUBc7iPsYcM6932wet+KceHP09Ek9lqv8gc2mD0Yv8nc7CAlBK i7qyyPTFtjy1cFFBoDqGq0upWAmhUebxDhkkcN5kcmS/WI+/Ds9qZaCLasNQRzf81VpIJjOZ4A2uY kBv0wpXamr2xY9PoxNNT6hAkH97vHUYdavpOLEoF74vD1iKMW7ex0/Thw7/gr8ssejRBOIOZvRvRC Hy08plvR9nGspfNDd6tw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hazvs-0002fk-BJ; Wed, 12 Jun 2019 09:52:40 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hazvp-0002fA-TW for linux-arm-kernel@lists.infradead.org; Wed, 12 Jun 2019 09:52:39 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3A76B28; Wed, 12 Jun 2019 02:52:36 -0700 (PDT) Received: from big-swifty.misterjones.org (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id CDC4C3F246; Wed, 12 Jun 2019 02:54:15 -0700 (PDT) Date: Wed, 12 Jun 2019 10:52:25 +0100 Message-ID: <86ef3zgmg6.wl-marc.zyngier@arm.com> From: Marc Zyngier To: Julien Thierry Subject: Re: [PATCH v2 1/9] KVM: arm/arm64: vgic: Add LPI translation cache definition In-Reply-To: <54c8547a-51fb-8ae5-975f-261d3934221a@arm.com> References: <20190611170336.121706-1-marc.zyngier@arm.com> <20190611170336.121706-2-marc.zyngier@arm.com> <54c8547a-51fb-8ae5-975f-261d3934221a@arm.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Organization: ARM Ltd MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190612_025238_050632_CBF25BF4 X-CRM114-Status: GOOD ( 28.53 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kvm@vger.kernel.org, Suzuki K Poulose , "Raslan, KarimAllah" , Christoffer Dall , Eric Auger , James Morse , "Saidi, Ali" , Zenghui Yu , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Julien, On Wed, 12 Jun 2019 09:16:21 +0100, Julien Thierry 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 > > --- > > 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel