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 C03D0C61DB3 for ; Mon, 9 Jan 2023 17:47:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2D2E48E0006; Mon, 9 Jan 2023 12:47:42 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 25BD88E0001; Mon, 9 Jan 2023 12:47:42 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0D7B58E0006; Mon, 9 Jan 2023 12:47:42 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id EBA088E0001 for ; Mon, 9 Jan 2023 12:47:41 -0500 (EST) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id A3A6C1A09DE for ; Mon, 9 Jan 2023 17:47:41 +0000 (UTC) X-FDA: 80335993122.27.D47A6CF Received: from mail-ej1-f51.google.com (mail-ej1-f51.google.com [209.85.218.51]) by imf12.hostedemail.com (Postfix) with ESMTP id CF81D40012 for ; Mon, 9 Jan 2023 17:47:39 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b="W/j+b4Zy"; spf=pass (imf12.hostedemail.com: domain of fvdl@google.com designates 209.85.218.51 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=1673286459; a=rsa-sha256; cv=none; b=g/qctuwrKkFg0uKH8tNsFuzKgarLPEuNF382FW5yPgRHAQAMf1fOzzpxjs6qoXMrEev9rf Klfn6ZJtj0yNkiTirNax849Trjzv/OkDFOvR8xWpGXHyQoEmEz4AmOZE2E9ZUrtXEQ2AVh Q03HTaoCuxvoWqijolq+4QzizpNsi6c= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b="W/j+b4Zy"; spf=pass (imf12.hostedemail.com: domain of fvdl@google.com designates 209.85.218.51 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=1673286459; 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=PzLZi58vmn1o7SydIT0mRtxlmY+LbB4xWlyv/P7+qRw=; b=owmPvEGTsvpZ1hZVzdGc0ohF5r4aNFa98C27hvw/O6szXXZLESZbJLuiED+MNjDlLb7tuT /SrhHUv8z40O6FM+4uFYhTycZymyhbALhm1DGti1WcrvzKnUcgPl5cbrwNRweqOzq8jv1t 3ViHb4h77RAbIdhZ7R75BSdXRiQ/970= Received: by mail-ej1-f51.google.com with SMTP id ud5so22103916ejc.4 for ; Mon, 09 Jan 2023 09:47:39 -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=PzLZi58vmn1o7SydIT0mRtxlmY+LbB4xWlyv/P7+qRw=; b=W/j+b4Zyy+SfZZ9eHr+vXYfPvynEvmmrAEReZeIAT7mZNSWzi9zOJy9rmCgm8zwSKO 6FTZi/xN5TVEoC8qjV/oUfXzRBsbipnS+TsrpX1HGYPVi9cDPJGjTBuxR6B3vAoHgOiB lMdTkhw9onR9NRGWxRAMjoZ4D21gB9oU7GQ3SGGg0qmEdKA9TbGblalAjdFctxtlAq+3 IpD1aaRRKlf74/QF7xUBlwJn2uplycljqCWn9AS9APYDGx7crB68AensnYOSy52SVv/O Krl0wdIu0fDJFo6m3DGxEX+YGEFkNeN0WgjlMOF+IKkybbPNjdJCwVyH1YjgjtPIukcX 1L9w== 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=PzLZi58vmn1o7SydIT0mRtxlmY+LbB4xWlyv/P7+qRw=; b=2XlSNkpkVSpCvHyGk5cuuin18i0wWtBjS4VzJQzc2FwhGQr62FcaIWuxTKE4mD4Oo9 8vXeVJoUOWk4VJvvpU5cvHtkfqE4E3RBWg8BEgm+9QZ9+6K6o/B0U0E0l6hUp2p7FPou iPLLHO0Gz4OvjPVd7RokfDK8qLf/92vlzNtgsDAvs24lfRKKLOAgVDGyBLFNTyaBu6hO qtF1cr08TD06SNcEL6SKS8QQNJvmrIV9givm7O0NW++QWcBber/7BN3sBT73KQp2z5LT k55MHC/H3sg+0O3q+umKO6hq6d45YrcWnK6R3K3lRJBEXdkCJKS2cYAxW5QQSPaocpUO N+Lg== X-Gm-Message-State: AFqh2kpw0fpD8Nhj+Lr2bz7wc01b88TJhh9ApjEL+HUMsR2r01DORZDI JtkkSmXXjw4pEsiIPBVkH0P3B+rVQZ/VGvZNypCURuDjQe8sjVdF X-Google-Smtp-Source: AMrXdXt/2HuNjYZN+fZW9dZlzW4fwymShWZKDLNdBivWwqdkja6PfQCJXpHkmE2wdUlVQgnYTCj8QPtvWqiTXvS+yoc= X-Received: by 2002:a17:906:b217:b0:84d:300a:cbfe with SMTP id p23-20020a170906b21700b0084d300acbfemr1275535ejz.332.1673286458279; Mon, 09 Jan 2023 09:47:38 -0800 (PST) MIME-Version: 1.0 References: <20230105215025.422635-1-fvdl@google.com> <083aa5a9-9209-7e06-a00f-dc9657acf1e6@arm.com> <57ccdc9a-4cdc-a138-0996-635250a626e1@arm.com> In-Reply-To: <57ccdc9a-4cdc-a138-0996-635250a626e1@arm.com> From: Frank van der Linden Date: Mon, 9 Jan 2023 09:47:26 -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-Rspam-User: X-Rspamd-Queue-Id: CF81D40012 X-Rspamd-Server: rspam01 X-Stat-Signature: d6rnt7f11o8h3a3r855tds8uxcf5itm5 X-HE-Tag: 1673286459-960966 X-HE-Meta: U2FsdGVkX1/d1AY3y7I0129JQZQlxJ8wwAL0MbMueTRpsd9lKgFBZ8JNKlLj/7dqCqYEcJbUN2TnJ5Xzea24l4J8/zrQ1Zqox+zRyVTT3gdOcw6UpN0XFWvE75z2Sc7Zc0X30szNbP79KGTjYe5q8oOJSVqMjX9IiXY+/4xS2lI50LtVimA/P+vxETrSibtTXNnIY9QQQzRElAnccS94JDOU22MRDaGakaIQ5aBQJlYONEVflAH3y50Ng9KJpW862NOIzOI4d4Ohm89frAlWmN6cjI7Ara/A9Bw2xI5WBtG5ZfHoWG+WbQI06W6bsUDJxtv4qBwomh/ffHYO8n7VbWYc8Iv64obC3EPVm3t52oLayZVwp1qEkjV7o5KI1jer2ENJ27MvoRO1UMkSWSuvrZfOZ2D5oOH8dpEfCvYPrMHdu95uyUaTGFgJR5YDLne2vAPLz+EmTD5noQTRS1wYPTSRltU8/E7XzG80/NOTGcxiGbnQOpJ/H9Y3hW33UUJBUHRen4TwJ1W/3l/clCekG2LlxZW3q/204dbBA4rf2X9tWngVazwZV0f3DQk3MRlNnWzUYyqRmcqK9nGuFEezrXcXAigpIoa83UnGAIA3VLShhSfBuecCwRDp4d4nhFABgYZeI+rRjpErGYFGVShbTfrfRoM8NXOWzlxL2riJRmm+VEodKj+HdxF6Ccji5zSyY8b3ukHElavQ0ka7AWU6dFzePKXk+uw/h1UP9zVJvqNcW0UW0O/kiieHuAUS+acUUCj6/O8SHG7ajdfcE5Ni3DdDFhtjcUf5dHME8sBK4NieaJnkVYw9KlQvktBHSFw9nWzqGDh8lfQWZqiXsUyrTb8qj6HiTnBvIcssZILAufIVkMKgmDjdfS78/iLlf5j00jn6EyBIFprRJNIlJzR6FjDUvBBhZyGvYSLTStI3LjJ0xarDI39v2y5n3A6DKXEmlBCFQXzTz10snu8fwCg bmq8fnSd SfAQPAljsnU68+HA= 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: Sure, v2 sent, addressing your comments. I got rid of the return value of phys_align_check() entirely, instead just checking the recorded alignment value. It's more consistent. Added more comments, made types consistent, split off things into a function. Thanks, - Frank On Mon, Jan 9, 2023 at 12:48 AM Anshuman Khandual wrote: > > > On 1/6/23 23:24, Frank van der Linden wrote: > > 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 :) > > I guess currently the lines could extend up to 100 instead. > > > > >> > >>> > >>> 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. > > Make sense, but will need a small comment explaining the same. > > >> > >>> + > >>> + 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. > > Okay but does a void is really necessary here even if the return value > is not checked ? > > > > >> > >>> + } > >>> + > >>> 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. > > Indeed. > > >> > >> - 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). > > I think you could still send an updated version with the suggested changes, > which can be pulled again for mm-unstable. These changes should be part of > a single commit being merged, for future clarity while reading these code.