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
<anshuman.khandual@xxxxxxx> 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
> > <anshuman.khandual@xxxxxxx> 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 <linux/hugetlb.h>
> >>> #include <linux/kernel.h>
> >>> #include <linux/kconfig.h>
> >>> +#include <linux/memblock.h>
> >>> #include <linux/mm.h>
> >>> #include <linux/mman.h>
> >>> #include <linux/mm_types.h>
> >>> @@ -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 <pstart, pend> 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.
