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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id A851AC54EBD for ; Fri, 6 Jan 2023 17:54:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 16D6B8E0003; Fri, 6 Jan 2023 12:54:54 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 11E398E0001; Fri, 6 Jan 2023 12:54:54 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F28C08E0003; Fri, 6 Jan 2023 12:54:53 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id E2C228E0001 for ; Fri, 6 Jan 2023 12:54:53 -0500 (EST) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id C241680D36 for ; Fri, 6 Jan 2023 17:54:53 +0000 (UTC) X-FDA: 80325124866.28.1C1F4C3 Received: from mail-ej1-f46.google.com (mail-ej1-f46.google.com [209.85.218.46]) by imf26.hostedemail.com (Postfix) with ESMTP id 1E89E140015 for ; Fri, 6 Jan 2023 17:54:51 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=V+aRZQXX; spf=pass (imf26.hostedemail.com: domain of fvdl@google.com designates 209.85.218.46 as permitted sender) smtp.mailfrom=fvdl@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1673027692; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=O6kNrh4iwO9yEdUtshiFKw02KLfvcpeN/+4jwZYGd74=; b=m1qn0ETKyGUFZrIZ1oJHk2cAxb/XlJ5rmwCr/b/b7zgESs4H0MKcIx9hr79Aes2IOku0LS +/OLmisRDNsoj2twL03tg7oqH48I1xz+2GgGsZDBqdK1xEmBIeZTLb7EwY5VqhK/Njr2KX 07vjZstdc1TsKCixUibj01ihYfa3hiY= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=V+aRZQXX; spf=pass (imf26.hostedemail.com: domain of fvdl@google.com designates 209.85.218.46 as permitted sender) smtp.mailfrom=fvdl@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1673027692; a=rsa-sha256; cv=none; b=DysZ7v4WxgdhrAl09u4CBULr1OIZS++iX7P4wboDNtwEKDzQkDu5quAh96EqyIzIGnBPMK tPlsggE1hbJhPY0ChuU02p44qcEYvlV/5LUQtIbuMNFe6jioyghJeJcm6ZUCJa2fZVzMhx vt19vSCpZIfbBHrcfya1G9BbsxZYHxo= Received: by mail-ej1-f46.google.com with SMTP id u9so5187142ejo.0 for ; Fri, 06 Jan 2023 09:54:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=O6kNrh4iwO9yEdUtshiFKw02KLfvcpeN/+4jwZYGd74=; b=V+aRZQXXUZGQOxGSgWjQDIvUJeDRo+awog3DIZw3mUvN4v0cqab5RgU23Lb7A8+NZ+ /IyP/O0l+LLSy/E9RIYGVluB8iZgsihHT+8ebRmh747DY+Bk2G9NI5LZYwD95McSrTvL sZSInI9YrMZ1L4u+XUyfBv31mps7oEiSDmOlJyLu5g0grgYG83yuQuV+WLrQir2FCqTd P0dOU1YOquNyjqAkWIu+OKIge8M+KatvBWQOJJibRKYsAKsfLo1kD+zpSJKVwPS4/C7r Up2rukwNxf00YV/M4q7zDX97cdSWO2IMfTV1to8z9rpJOLQCe3uHRzPRDNZ5Bembfrrt 6j8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=O6kNrh4iwO9yEdUtshiFKw02KLfvcpeN/+4jwZYGd74=; b=WDb5OeNxb7jpmzBU219RGTIDcTSP4yIClXb3PMkDI4p25yAq8f+0PlvudE1myGzBvj SyXAa3kTL/H8m2cQPn5T4o4YOPOs5K+Csd27gAFbNH76IcFtsPYFTr86D/SNJ5XBTeCU HduNgzxJdXLhf+DLNAuGPk4k2ul17jv6RhH6Ol5myBURmb1eb9z99ZLZCVvSTXU30OWG AyJdkzsOgB76slUDqrAcvm8KbS/5/SbGRdBzB1M8EoBE8gAlRxEdRbJBdzNt1yB/L8vm IKrDpwQmCQ3YQ3X2qlqiCmuRgDQf9DcYzqv6cYWrYb+48156QdsP3ggbXZapJ0FxQzz0 h0Rw== X-Gm-Message-State: AFqh2krHchof90Dnfk9e3PbbfBYvbiUAbWPBU02JIeGKoiJjspZRQpYJ gQsTjzLFxaKpklaDSbglo8RtvQCYEARCPJXnRmpc3g== X-Google-Smtp-Source: AMrXdXsYWcn9v8UYk2f8wGFbBsTIJscd0jWW5Ig7UlRxEXF41lGDdYALz+B93srDqUriFrlAonaI0pt2Bl37p7BxTYA= X-Received: by 2002:a17:907:6f18:b0:78d:b819:e0e7 with SMTP id sy24-20020a1709076f1800b0078db819e0e7mr4241534ejc.83.1673027690499; Fri, 06 Jan 2023 09:54:50 -0800 (PST) MIME-Version: 1.0 References: <20230105215025.422635-1-fvdl@google.com> <083aa5a9-9209-7e06-a00f-dc9657acf1e6@arm.com> In-Reply-To: <083aa5a9-9209-7e06-a00f-dc9657acf1e6@arm.com> From: Frank van der Linden Date: Fri, 6 Jan 2023 09:54:39 -0800 Message-ID: Subject: Re: [PATCH] mm/debug: use valid physical memory for pmd/pud tests To: Anshuman Khandual Cc: Andrew Morton , linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 1E89E140015 X-Stat-Signature: 8mk6u7aiug1xkqwojijgjm3jsxjdwgf7 X-Rspam-User: X-HE-Tag: 1673027691-299972 X-HE-Meta: U2FsdGVkX18lT0LGQ3peeP/SwIqvP4Xi3Wp8SNxAO3EIsqsX/dsNglcV54vf0pkyP0X+vx9CD4DcWiD/psn3V9nGx41y4KoDfJBBcvtNf0xE7hCrsLrBvhpA7+O0XXUfvdD4X+bQhM4oeM6jOTEvWKVSFN05eFmJdmb0FZ+OD0sMD5dZ2XCLSxGPIPv5Qc9N4bqQgeiByhBbhknLdzDWkAdiPBQ9EP1Sq4p0WTJFViRvSJ+LshiITZLQlDN+LHNBDeOMPtZrHJubgClb47qmy9+QYCvBhVq2xp6hXFgvP8j8yaX5x44MMOX3Ev3uuzr1cCOcA+25Voc92wqI6qJ0Va9uTGJbtRHhHCei9w/IatceyorlzbqYIQImJDgnzDQjZHUf5gupTTTwTvtgyk8W7vnoh5ZDeKrUWP3MKRd70a11Ogm3azcAIzy9hW8eBiIkO57A5yEM6504Jn2Xvbakme47WtDTVJlp+pw3WXMvMOIsnc1UpKFmugyygn8t/QQuHMaDK6OaqUk9thad3d8SHAVoCTboBCgwWeWmb2rkt4h6AUs0oNH3xb87DjfgWH+txcYDjsFEB5tKa7EO4puHrC44/qQVjRYxzP4t1vtXT8Qj1Q1cFw0QZ/HS554QHzDrTXS/S9Km76E6K2XjlXqYQSxndmRUTzHrrwDcXsMS5OuDTCtPQzgrtPOFlX1v3TjERgORYvqy/LmbxSKp/VO2XHxxpUFCo9R3BuxJP0OXe9oBZN8xQEkThxvKzutbv4ju0KPEztQEeNYVXDL7lIpQ5YqMEHNMU5jJiDmGw2vz3B0Gqu7JupbO4Jyp+EWB1U9a9jzCmOoTwz3Znrq8yvFi5rr9X/oQeobOneimdMFDpZY3+H9ZNh0/rrpyyuOi3nRSkxzKnLiImCywqoEDYYIvMHbVtKVOi+CRY9r4/9hY3u/jLTOX8/BR6uo94oo03TqOhzsqnlYlP1d1d+g2dFv zLUCwlI3 ZAQxhSHl+KI+YLkI= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hi Anshuman, thanks for looking at this. On Thu, Jan 5, 2023 at 8:24 PM Anshuman Khandual wrote: > > Hi Frank, > > Thanks for the patch, in principle this LGTM. Did a quick run on arm64, > did not find anything problematic. Although I have some comments below. > [...] > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > > index c631ade3f1d2..e9b52600904a 100644 > > --- a/mm/debug_vm_pgtable.c > > +++ b/mm/debug_vm_pgtable.c > > @@ -15,6 +15,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -80,6 +81,8 @@ struct pgtable_debug_args { > > unsigned long pmd_pfn; > > unsigned long pte_pfn; > > > > + phys_addr_t fixed_alignment; > > + > > This should not be a 'phys_addr_t', as it does not really contain a > physical address. Alignment value can be captured in 'unsigned long' > like other elements. True, yep. > > > unsigned long fixed_pgd_pfn; > > unsigned long fixed_p4d_pfn; > > unsigned long fixed_pud_pfn; > > @@ -430,7 +433,8 @@ static void __init pmd_huge_tests(struct pgtable_debug_args *args) > > { > > pmd_t pmd; > > > > - if (!arch_vmap_pmd_supported(args->page_prot)) > > + if (!arch_vmap_pmd_supported(args->page_prot) || > > + args->fixed_alignment < PMD_SIZE) > > return; > > Small nit. Additional line not need for the conditional statement. > You mean the line break in the condition? Not breaking it would push it to 90 characters (if tab=8). Most of this file, except for a few lines, does stick to 80. I don't feel particularly strongly about this either way, though :) > > > > > pr_debug("Validating PMD huge\n"); > > @@ -449,7 +453,8 @@ static void __init pud_huge_tests(struct pgtable_debug_args *args) > > { > > pud_t pud; > > > > - if (!arch_vmap_pud_supported(args->page_prot)) > > + if (!arch_vmap_pud_supported(args->page_prot) || > > + args->fixed_alignment < PUD_SIZE) > > return; > Small nit. Additional line not needed for the conditional statement. See above. > > > > > pr_debug("Validating PUD huge\n"); > > @@ -1077,11 +1082,41 @@ debug_vm_pgtable_alloc_huge_page(struct pgtable_debug_args *args, int order) > > return page; > > } > > > > +/* > > + * Check if a physical memory range described by contains > > + * an area that is of size psize, and aligned to the same. > > + * > > + * Don't use address 0, and check for overflow. > > + */ > > +static int __init phys_align_check(phys_addr_t pstart, > > + phys_addr_t pend, phys_addr_t psize, phys_addr_t *physp, > > + phys_addr_t *alignp) > > +{ > > + phys_addr_t aligned_start, aligned_end; > > + > > + if (pstart == 0) > > + pstart = PAGE_SIZE; > > Why ? Since the physical address will be used for page table tests, I think that avoiding 0 is probably a good idea. If e.g. a masking mistake crept into the code somewhere, using physical address 0 might not find it. Also, physical address 0 isn't used on x86. > > > + > > + aligned_start = ALIGN(pstart, psize); > > + aligned_end = aligned_start + psize; > > + > > + if (aligned_end > aligned_start && aligned_end <= pend) { > > + *alignp = psize; > > + *physp = aligned_start; > > + return 1; > > + } > > + > > + return 0; > > +} > > To be more clear, this function should return a 'bool' instead That would be better, yes. > > > + > > + > > static int __init init_args(struct pgtable_debug_args *args) > > { > > struct page *page = NULL; > > phys_addr_t phys; > > int ret = 0; > > + u64 idx; > > + phys_addr_t pstart, pend; > > This declaration can be merged into the previous line containing 'phys'. Sure, yes. > > > > > /* > > * Initialize the debugging data. > > @@ -1161,15 +1196,32 @@ static int __init init_args(struct pgtable_debug_args *args) > > WARN_ON(!args->start_ptep); > > > > /* > > - * PFN for mapping at PTE level is determined from a standard kernel > > - * text symbol. But pfns for higher page table levels are derived by > > - * masking lower bits of this real pfn. These derived pfns might not > > - * exist on the platform but that does not really matter as pfn_pxx() > > - * helpers will still create appropriate entries for the test. This > > - * helps avoid large memory block allocations to be used for mapping > > - * at higher page table levels in some of the tests. > > + * Find a valid physical range, preferably aligned to PUD_SIZE. > > + * Return the address and the alignment. It doesn't need to be > > + * allocated, it just needs to exist as usable memory. The memory > > + * won't be touched. > > + * > > + * The alignment is recorded, and can be checked to see if we > > + * can run the tests that require and actual valid physical > > s/and/an ? Indeed, that's a typo. > > > + * address range on some architectures ({pmd,pud}_huge_test > > + * on x86). > > */ > > + > > phys = __pa_symbol(&start_kernel); > > This original 'phys' will still be used as fallback, in case the below attempt > does not find a physical address with required alignments i.e [PUD|PMD]_SIZE ? Right, the original value (as it is done now) is there as a fallback. > > > + args->fixed_alignment = PAGE_SIZE; > > + > > + for_each_mem_range(idx, &pstart, &pend) { > > + if (phys_align_check(pstart, pend, PUD_SIZE, &phys, > > + &args->fixed_alignment)) > > + break; > > + > > + if (args->fixed_alignment >= PMD_SIZE) > > + continue; > > + > > + (void)phys_align_check(pstart, pend, PMD_SIZE, &phys, > > + &args->fixed_alignment); > > (void) ? Why not check the return value here ? If you get to that function call, you know that no aligned area has been found so far, so checking the return value won't change what you're going to do: you're going to keep going, since even if you get a PMD_SIZE aligned area, you still want to try to get a PUD_SIZE aligned area. So there's no point in checking it. > > > + } > > + > > args->fixed_pgd_pfn = __phys_to_pfn(phys & PGDIR_MASK); > > args->fixed_p4d_pfn = __phys_to_pfn(phys & P4D_MASK); > > args->fixed_pud_pfn = __phys_to_pfn(phys & PUD_MASK); > > This loops attempts to find a PUD_SIZE aligned address but breaks out in case it > atleast finds a PMD_SIZE aligned address, while looping through available memory > ranges. The entire process of finding 'phys' and 'args->fixed_alignment' should > be encapsulated inside a helper that also updates 'args->fixed_pxx_pfn' elements. The loop keeps going until it either runs out of physical memory ranges to check, or until it finds a PUD_SIZE-aligned area. It won't break out for a PMD_SIZE-aligned area. It could be made in to a separate function, yes, that might look a little cleaner. > > - Anshuman Thanks again for the comments. I see that this was added to mm-unstable by now. I can send an mm-unstable follow-up patch (though there won't be any functional changes). - Frank