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 7F7DDC54EBD for ; Mon, 9 Jan 2023 08:48:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CF35D8E0002; Mon, 9 Jan 2023 03:48:28 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id CA35D8E0001; Mon, 9 Jan 2023 03:48:28 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B6A7B8E0002; Mon, 9 Jan 2023 03:48:28 -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 A10D08E0001 for ; Mon, 9 Jan 2023 03:48:28 -0500 (EST) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 6C7AA40535 for ; Mon, 9 Jan 2023 08:48:28 +0000 (UTC) X-FDA: 80334634296.26.5502D9B Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf14.hostedemail.com (Postfix) with ESMTP id 93528100003 for ; Mon, 9 Jan 2023 08:48:26 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=none; spf=pass (imf14.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1673254106; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=iLKOWBFln5T//U3BXdQz5oax4cLHZrLaJkWHwXwfN5Y=; b=i4U8VLJWRZQIWCFKXS/L7htxZy6+sC5wEG15EKuu17pfc3TlEr9lJkyJUjxVxoJHlBsjLz zysHuXWggMwP+qejsuiYsMTYckL3FHTQc2FV/FNZBTTNJHZ/WU+jrSV+3Nlxy0yQvRsWVZ Bc5Hj22fHB1eA/9kJzoOBU2N/8ahcAM= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=none; spf=pass (imf14.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1673254106; a=rsa-sha256; cv=none; b=qROeVxQMqz62IgZr6rA6MKNSXl/NkS+2kYhHXqM85GZCS8G18AsfgNi6i2RIvN2H4mIpYF sH0vDNyUV/rCUChHWJL/ktZn5EQkvMRBD9ZqQ2VGzjbJYkXGMqwKiPrQNNfCe0+0KSuMgX 0y1Na7GB/qJgsNLwLmPmY1Qi/B4Pqc4= 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 7C2651042; Mon, 9 Jan 2023 00:49:07 -0800 (PST) Received: from [10.162.40.15] (unknown [10.162.40.15]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 55C773F67D; Mon, 9 Jan 2023 00:48:24 -0800 (PST) Message-ID: <57ccdc9a-4cdc-a138-0996-635250a626e1@arm.com> Date: Mon, 9 Jan 2023 14:18:21 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [PATCH] mm/debug: use valid physical memory for pmd/pud tests Content-Language: en-US To: Frank van der Linden Cc: Andrew Morton , linux-mm@kvack.org References: <20230105215025.422635-1-fvdl@google.com> <083aa5a9-9209-7e06-a00f-dc9657acf1e6@arm.com> From: Anshuman Khandual In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Stat-Signature: 3pebctpxhrtqex7hwetuc93zt6mjjzx7 X-Rspam-User: X-Rspamd-Queue-Id: 93528100003 X-Rspamd-Server: rspam06 X-HE-Tag: 1673254106-359423 X-HE-Meta: U2FsdGVkX18vAYQEfadx+rYnruPmf/aEMTp53ZlkIAZIZPN+M3yC3RiZD4ACj47MMahygSeZHRnL1IpKaqAy8nF5N9r7EsPJPY85WWyH3N1mHas2Yv+t7gnGMIGLubJIn+5Ymj9bWy9gN8v84q3LFqyIsSFjPkNf2ru1R2eBXaayz07mn923/H+KXw/ZMyxHgi25jlcFBKv6ZxVDsOxtIJQefCqUFSV50rBEv9F5Q66Uvc7IUysWPDQNHYHPmU4s99Wqnr0MRf8FIZA0xGE6nO8jX2gcmwVd1dH+rqM6suNAVqcq96RXtnr8mvAxX7p7qeu6xnJGB0RWDAfB8Rj6LIPCnHIbreh2shdVP1p9m3/1urRcFzN1LwPzQ5pWpeEl1QhPKZ6ctDge0mgk+IL8kti+d3gzTCNJRmGEG/OBkmBrYsr/933T9+N+UOp4XFTpyhRFe/aZHStWJps+lFaoeqpa9N+LoBNl9B9F9qzcO6p4uxKnnYm5LCPzfMrlpCorcXBv2kWi99X+m1DMUMHAPQpcpywOlVgKOZ0UlYdeV8RpTt9uW+mXAwiz+442oN/nJFGf3kslbS8zoOvtmrgXL7DUtDT1k5lweM9tO7kWgw5X/GBqOeBqIdLBu4Kf3Nb3PT+zhzzYSu//V/LnI04tmdO2n8cDzZZEeQHIP+jYkSmXEif4pO29Q3zTPFOUlB0GYO/izEFB1AhS3OUGJBJz+KwuuPYsfdpYcCAha66mzDNp2PkjiJWKXbLqqh2FY4jUpbdtWARtzjQ7bsZLuK74p3JYOuInjdCQXMaMjbP7mN2Ye2DL6FS6ubeS/IcRBCo1v1pSseiQ080= 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: 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.