Received: by 2002:a05:6358:1087:b0:cb:c9d3:cd90 with SMTP id j7csp1662528rwi; Wed, 19 Oct 2022 13:41:56 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7UAULNbRNglHIniWnEm1PIK8ANgnLjsWUU53rNPJ0XofrIXmFRw/aiGsX21bZhRAxdDMAc X-Received: by 2002:a63:1b20:0:b0:464:a9a6:571b with SMTP id b32-20020a631b20000000b00464a9a6571bmr8554415pgb.560.1666212116102; Wed, 19 Oct 2022 13:41:56 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666212116; cv=none; d=google.com; s=arc-20160816; b=R8mZt/oFGo8c6drnyWYYXd2ZLYCwwig0FeUVPCU3Y4bN6tR9iaKvS0KzXKSVPr8G46 Q48RkQ6oqp4p5GF4PuC7+vZmcKWpfGx4Jhu38xcCMqVgLYM9LV/fdBZ6OnzISY09xykS DkTVVipI2oSxBSJWdJrkzF6G/fLMI/Jk0eJDzu63tnY0iY1/GSWBfiwGNU+J645ifY9Z 8bBOTbF/WbzRnWbzYN4w4G4LoO9oZ0F5fKTH+oOQguE5Sb1oZg9mioTs2ubPM/FfmlEP Ajud7cU3AxlN/70EnmyHptzhInWCoJCqvCW5wYn5BJeOKcIN4chibtEN69haTCV2iiD2 tNDw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:in-reply-to:subject :from:references:cc:to:content-language:user-agent:mime-version:date :message-id; bh=vMD7Z96IH6IhDMCk23K/eVq/HjdjmoxHTQsYTkUg5OA=; b=M4sK8cfDGAptfNzP4k6laTGAoQjXkTOviIaVTd9tPq2AwQRcxwzGcYBbZu6KddfWz+ BenWjo5fp0UMlI3qxmo6Z/sCK60AafO8U+BU43A6y3IlmbVnqsC9ng6dGPihmyaI4OKP Ry/pk+VcpJbw5SkgmCY3tzq7BecASfTdLV04kd0Ie9JqUb5vl/C5M10gNplR2yhS7l82 CPiDQ7JHlo4f6+RwQCWLu5peo6Phtz3WnPXV+zNGKOYd38K3QQez51cQgO445S6DrUNQ 8P5QQsdVBMueDuzEFAr0zcpvt2kKqOFd8waVjH1BcentWH0Xe152cDyT/Vv4+xzDSxYh Xlvw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id i185-20020a6387c2000000b0045fa2a39f4bsi17448326pge.247.2022.10.19.13.41.44; Wed, 19 Oct 2022 13:41:56 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231150AbiJSUSz (ORCPT + 99 others); Wed, 19 Oct 2022 16:18:55 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46654 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229935AbiJSUSw (ORCPT ); Wed, 19 Oct 2022 16:18:52 -0400 Received: from vps-vb.mhejs.net (vps-vb.mhejs.net [37.28.154.113]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3B0631A0C2F; Wed, 19 Oct 2022 13:18:49 -0700 (PDT) Received: from MUA by vps-vb.mhejs.net with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1olFWW-0003EF-DJ; Wed, 19 Oct 2022 22:18:44 +0200 Message-ID: Date: Wed, 19 Oct 2022 22:18:38 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Content-Language: en-US, pl-PL To: Gavin Shan Cc: kvm@vger.kernel.org, maz@kernel.org, linux-kernel@vger.kernel.org, zhenyzha@redhat.com, shan.gavin@gmail.com, kvmarm@lists.linux.dev, pbonzini@redhat.com, shuah@kernel.org, kvmarm@lists.cs.columbia.edu, ajones@ventanamicro.com References: <20221014071914.227134-1-gshan@redhat.com> <20221014071914.227134-5-gshan@redhat.com> <3eecebca-a526-d10a-02d3-496ce919d577@maciej.szmigiero.name> <5bfbe050-a654-8400-e1f1-dcfa4dba13e6@redhat.com> From: "Maciej S. Szmigiero" Subject: Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size In-Reply-To: <5bfbe050-a654-8400-e1f1-dcfa4dba13e6@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 19.10.2022 02:26, Gavin Shan wrote: > On 10/18/22 11:56 PM, Maciej S. Szmigiero wrote: >> On 18.10.2022 02:51, Gavin Shan wrote: >>> On 10/18/22 8:46 AM, Gavin Shan wrote: >>>> On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote: >>>>> On 14.10.2022 09:19, Gavin Shan wrote: >>>>>> The test case is obviously broken on aarch64 because non-4KB guest >>>>>> page size is supported. The guest page size on aarch64 could be 4KB, >>>>>> 16KB or 64KB. >>>>>> >>>>>> This supports variable guest page size, mostly for aarch64. >>>>>> >>>>>>    - The host determines the guest page size when virtual machine is >>>>>>      created. The value is also passed to guest through the synchronization >>>>>>      area. >>>>>> >>>>>>    - The number of guest pages are unknown until the virtual machine >>>>>>      is to be created. So all the related macros are dropped. Instead, >>>>>>      their values are dynamically calculated based on the guest page >>>>>>      size. >>>>>> >>>>>>    - The static checks on memory sizes and pages becomes dependent >>>>>>      on guest page size, which is unknown until the virtual machine >>>>>>      is about to be created. So all the static checks are converted >>>>>>      to dynamic checks, done in check_memory_sizes(). >>>>>> >>>>>>    - As the address passed to madvise() should be aligned to host page, >>>>>>      the size of page chunk is automatically selected, other than one >>>>>>      page. >>>>>> >>>>>>    - All other changes included in this patch are almost mechanical >>>>>>      replacing '4096' with 'guest_page_size'. >>>>>> >>>>>> Signed-off-by: Gavin Shan >>>>>> --- >>>>>>   .../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++------- >>>>>>   1 file changed, 115 insertions(+), 76 deletions(-) >>>>>> >>>>>> diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c >>>>>> index d5aa9148f96f..d587bd952ff9 100644 >>>>>> --- a/tools/testing/selftests/kvm/memslot_perf_test.c >>>>>> +++ b/tools/testing/selftests/kvm/memslot_perf_test.c >> (...) >>>>>> @@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES % >>>>>>    * for the total size of 25 pages. >>>>>>    * Hence, the maximum size here is 50 pages. >>>>>>    */ >>>>>> -#define MEM_TEST_MOVE_SIZE_PAGES    (50) >>>>>> -#define MEM_TEST_MOVE_SIZE        (MEM_TEST_MOVE_SIZE_PAGES * 4096) >>>>>> +#define MEM_TEST_MOVE_SIZE        0x32000 >>>>> >>>>> The above number seems less readable than an explicit value of 50 pages. >>>>> >>>>> In addition to that, it's 50 pages only with 4k page size, so at least >>>>> the comment above needs to be updated to reflect this fact. >>>>> >>>> >>>> Yeah, I will change the comments like below in next revision. >>>> >>>>   /* >>>>    * When running this test with 32k memslots, actually 32763 excluding >>>>    * the reserved memory slot 0, the memory for each slot is 0x4000 bytes. >>>>    * The last slot contains 0x19000 bytes memory. Hence, the maximum size >>>>    * here is 0x32000 bytes. >>>>    */ >>>> >>> >>> I will replace those numbers with readable ones like below :) >>> >>> /* >>>   * When running this test with 32k memslots, actually 32763 excluding >>>   * the reserved memory slot 0, the memory for each slot is 16KB. The >>>   * last slot contains 100KB memory with the remaining 84KB. Hence, >>>   * the maximum size is double of that (200KB) >>>   */ >> >> Still, these numbers are for x86, which has KVM_INTERNAL_MEM_SLOTS >> defined as 3. >> >> As far as I can see aarch64 has KVM_INTERNAL_MEM_SLOTS equal to 0, so >> this arch has 32766 slot available for the test memory. >> >> Quick calculations show that this will result in 112 KiB of memory in >> the last slot for 4 KiB page size (while for 64 KiB page size the >> maximum slot count for this test is 8192 anyway - not counting slot 0). >> > > It seems your calculation had (512MB+64KB), instead of (512MB+4KB). > In this particular patch, we still have (512MB+4KB). How about to change > like below in this patch. In next patch, it's adjusted accordingly after > we have (512MB+64KB). My review comment above referred to the final MEM_SIZE value after the whole series, so 512 MiB + 64 KiB. I placed that review comment on patch 4 since it's the only patch in this series that modified the code comment about MEM_TEST_MOVE_SIZE. > > (1) In this patch, the comment is changed to as below > >     /* >      * We have different number of memory slots, excluding the reserved >      * memory slot 0, on various architectures and configurations. The >      * memory size in this test is calculated by doubling the maximal >      * memory size in last memory slot, with alignment to the largest >      * supported page size (64KB). >      * >      * architecture   slots    memory-per-slot    memory-on-last-slot >      * -------------------------------------------------------------- >      * x86-4KB        32763    16KB               100KB >      * arm64-4KB      32766    16KB               52KB >      * arm64-64KB     8192     64KB               64KB >      */ >     #define MEM_TEST_MOVE_SIZE    0x40000           /* 256KB */ > > (2) In the next patch, where we have (512MB+64KB) after the various >     memory sizes are consolidated, It is adjusted accordingly as below. > >     /* >      * We have different number of memory slots, excluding the reserved >      * memory slot 0, on various architectures and configurations. The >      * memory size in this test is calculated by doubling the maximal >      * memory size in last memory slot, with alignment to the largest >      * supported page size (64KB). >      * >      * architecture   slots    memory-per-slot    memory-on-last-slot >      * -------------------------------------------------------------- >      * x86-4KB        32763    16KB               160KB >      * arm64-4KB      32766    16KB               112KB >      * arm64-64KB     8192     64KB               128KB >      */ >     #define MEM_TEST_MOVE_SIZE    0x50000           /* 320KB */ Now MEM_TEST_MOVE_SIZE is too high for arm64-4KB and arm64-64KB cases (it needs 160 KiB in the last slot but has less available in these two cases). Using a test size of 192 KiB instead seems like a small difference from the original size of 200 KiB, while still being aligned to 64 KiB. The move benchmarks runtime difference on x86-4KB with this size (compared to sizes of 200 KiB and 320 KiB) seems to be negligible. Since it's an odd number of 64 KiB pages (3) the code that halves this number of pages will need to be adjusted to operate on raw sizes instead. I can see a single block of code that will need such adjustment: > if (lastpages < move_pages / 2) { > *maxslots = 0; > return false; > } Similar remark goes for the case (1) above, where you'll probably need to use 64 KiB test area size (it's only an intermediate form of code before the final patch changes this value so it's fine if it doesn't perform as well as the final form of the code). > Thanks, > Gavin > Thanks, Maciej