Re: [External] Re: [PATCH v2 1/3] mm: sparsemem: split the huge PMD mapping of vmemmap pages

From: Muchun Song
Date: Tue Jun 15 2021 - 23:24:13 EST

Next message: Tzung-Bi Shih: "Re: [PATCH] drm/bridge: anx7625: Make hpd workqueue freezable"
Previous message: Andy Lutomirski: "[PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation"
In reply to: Mike Kravetz: "Re: [PATCH v2 1/3] mm: sparsemem: split the huge PMD mapping of vmemmap pages"
Next in thread: Andrew Morton: "Re: [PATCH v2 0/3] Split huge PMD mapping of vmemmap pages"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Jun 16, 2021 at 6:31 AM Mike Kravetz <mike.kravetz@xxxxxxxxxx> wrote:
>
> On 6/12/21 2:45 AM, Muchun Song wrote:
> > Currently, we disable huge PMD mapping of vmemmap pages when that feature
> > of "Free some vmemmap pages of HugeTLB pages" is enabled. If the vmemmap
> > is huge PMD mapped when we walk the vmemmap page tables, we split the
> > huge PMD firstly and then we move to PTE mappings. When HugeTLB pages are
> > freed from the pool we do not attempt coalasce and move back to a PMD
> > mapping because it is much more complex.
>
> Possible rewording of commit message:
>
> In [1], PMD mappings of vmemmap pages were disabled if the the feature
> hugetlb_free_vmemmap was enabled. This was done to simplify the initial
> implementation of vmmemap freeing for hugetlb pages. Now, remove this
> simplification by allowing PMD mapping and switching to PTE mappings as
> needed for allocated hugetlb pages.
>
> When a hugetlb page is allocated, the vmemmap page tables are walked to
> free vmemmap pages. During this walk, split huge PMD mappings to PTE
> mappings as required. In the unlikely case PTE pages can not be allocated,
> return error(ENOMEM) and do not optimize vmemmap of the hugetlb page.
>
> When HugeTLB pages are freed from the pool, we do not attempt to coalesce
> and move back to a PMD mapping because it is much more complex.
>
> [1] https://lkml.kernel.org/r/20210510030027.56044-8-songmuchun@xxxxxxxxxxxxx

Thanks, Mike, I'll reuse this.

>
> >
> > Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> > ---
> > include/linux/mm.h | 4 +-
> > mm/hugetlb_vmemmap.c | 5 +-
> > mm/sparse-vmemmap.c | 157 ++++++++++++++++++++++++++++++++++++++-------------
> > 3 files changed, 123 insertions(+), 43 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index cadc8cc2c715..8284e8ed30c9 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -3055,8 +3055,8 @@ static inline void print_vma_addr(char *prefix, unsigned long rip)
> > }
> > #endif
> >
> > -void vmemmap_remap_free(unsigned long start, unsigned long end,
> > - unsigned long reuse);
> > +int vmemmap_remap_free(unsigned long start, unsigned long end,
> > + unsigned long reuse);
> > int vmemmap_remap_alloc(unsigned long start, unsigned long end,
> > unsigned long reuse, gfp_t gfp_mask);
> >
> > diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c
> > index f9f9bb212319..06802056f296 100644
> > --- a/mm/hugetlb_vmemmap.c
> > +++ b/mm/hugetlb_vmemmap.c
> > @@ -258,9 +258,8 @@ void free_huge_page_vmemmap(struct hstate *h, struct page *head)
> > * to the page which @vmemmap_reuse is mapped to, then free the pages
> > * which the range [@vmemmap_addr, @vmemmap_end] is mapped to.
> > */
> > - vmemmap_remap_free(vmemmap_addr, vmemmap_end, vmemmap_reuse);
> > -
> > - SetHPageVmemmapOptimized(head);
> > + if (!vmemmap_remap_free(vmemmap_addr, vmemmap_end, vmemmap_reuse))
> > + SetHPageVmemmapOptimized(head);
> > }
> >
> > void __init hugetlb_vmemmap_init(struct hstate *h)
> > diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c
> > index 693de0aec7a8..7f73c37f742d 100644
> > --- a/mm/sparse-vmemmap.c
> > +++ b/mm/sparse-vmemmap.c
> > @@ -38,6 +38,7 @@
> > * vmemmap_remap_walk - walk vmemmap page table
> > *
> > * @remap_pte: called for each lowest-level entry (PTE).
> > + * @walked_pte: the number of walked pte.
>
> Suggest name change to 'nr_walked_pte' or just 'nr_walked'? walked_pte
> could be confused with a pointer to pte.

Make sense. Will update.

>
> > * @reuse_page: the page which is reused for the tail vmemmap pages.
> > * @reuse_addr: the virtual address of the @reuse_page page.
> > * @vmemmap_pages: the list head of the vmemmap pages that can be freed
> > @@ -46,11 +47,44 @@
> > struct vmemmap_remap_walk {
> > void (*remap_pte)(pte_t *pte, unsigned long addr,
> > struct vmemmap_remap_walk *walk);
> > + unsigned long walked_pte;
> > struct page *reuse_page;
> > unsigned long reuse_addr;
> > struct list_head *vmemmap_pages;
> > };
> >
> > +static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start,
> > + struct vmemmap_remap_walk *walk)
> > +{
> > + pmd_t __pmd;
> > + int i;
> > + unsigned long addr = start;
> > + struct page *page = pmd_page(*pmd);
> > + pte_t *pgtable = pte_alloc_one_kernel(&init_mm);
> > +
> > + if (!pgtable)
> > + return -ENOMEM;
> > +
> > + pmd_populate_kernel(&init_mm, &__pmd, pgtable);
> > +
> > + for (i = 0; i < PMD_SIZE / PAGE_SIZE; i++, addr += PAGE_SIZE) {
> > + pte_t entry, *pte;
> > + pgprot_t pgprot = PAGE_KERNEL;
> > +
> > + entry = mk_pte(page + i, pgprot);
> > + pte = pte_offset_kernel(&__pmd, addr);
> > + set_pte_at(&init_mm, addr, pte, entry);
> > + }
> > +
> > + /* Make pte visible before pmd. See comment in __pte_alloc(). */
> > + smp_wmb();
> > + pmd_populate_kernel(&init_mm, pmd, pgtable);
> > +
> > + flush_tlb_kernel_range(start, start + PMD_SIZE);
> > +
> > + return 0;
> > +}
> > +
> > static void vmemmap_pte_range(pmd_t *pmd, unsigned long addr,
> > unsigned long end,
> > struct vmemmap_remap_walk *walk)
> > @@ -68,59 +102,81 @@ static void vmemmap_pte_range(pmd_t *pmd, unsigned long addr,
> > * walking, skip the reuse address range.
> > */
> > addr += PAGE_SIZE;
> > + walk->walked_pte++;
> > pte++;
> > }
> >
> > - for (; addr != end; addr += PAGE_SIZE, pte++)
> > + for (; addr != end; addr += PAGE_SIZE, pte++) {
> > walk->remap_pte(pte, addr, walk);
> > + walk->walked_pte++;
> > + }
> > }
> >
> > -static void vmemmap_pmd_range(pud_t *pud, unsigned long addr,
> > - unsigned long end,
> > - struct vmemmap_remap_walk *walk)
> > +static int vmemmap_pmd_range(pud_t *pud, unsigned long addr,
> > + unsigned long end,
> > + struct vmemmap_remap_walk *walk)
> > {
> > pmd_t *pmd;
> > unsigned long next;
> >
> > pmd = pmd_offset(pud, addr);
> > do {
> > - BUG_ON(pmd_leaf(*pmd));
> > + if (pmd_leaf(*pmd)) {
> > + int ret;
> >
> > + ret = split_vmemmap_huge_pmd(pmd, addr & PMD_MASK, walk);
> > + if (ret)
> > + return ret;
> > + }
> > next = pmd_addr_end(addr, end);
> > vmemmap_pte_range(pmd, addr, next, walk);
> > } while (pmd++, addr = next, addr != end);
> > +
> > + return 0;
> > }
> >
> > -static void vmemmap_pud_range(p4d_t *p4d, unsigned long addr,
> > - unsigned long end,
> > - struct vmemmap_remap_walk *walk)
> > +static int vmemmap_pud_range(p4d_t *p4d, unsigned long addr,
> > + unsigned long end,
> > + struct vmemmap_remap_walk *walk)
> > {
> > pud_t *pud;
> > unsigned long next;
> >
> > pud = pud_offset(p4d, addr);
> > do {
> > + int ret;
> > +
> > next = pud_addr_end(addr, end);
> > - vmemmap_pmd_range(pud, addr, next, walk);
> > + ret = vmemmap_pmd_range(pud, addr, next, walk);
> > + if (ret)
> > + return ret;
> > } while (pud++, addr = next, addr != end);
> > +
> > + return 0;
> > }
> >
> > -static void vmemmap_p4d_range(pgd_t *pgd, unsigned long addr,
> > - unsigned long end,
> > - struct vmemmap_remap_walk *walk)
> > +static int vmemmap_p4d_range(pgd_t *pgd, unsigned long addr,
> > + unsigned long end,
> > + struct vmemmap_remap_walk *walk)
> > {
> > p4d_t *p4d;
> > unsigned long next;
> >
> > p4d = p4d_offset(pgd, addr);
> > do {
> > + int ret;
> > +
> > next = p4d_addr_end(addr, end);
> > - vmemmap_pud_range(p4d, addr, next, walk);
> > + ret = vmemmap_pud_range(p4d, addr, next, walk);
> > + if (ret)
> > + return ret;
> > } while (p4d++, addr = next, addr != end);
> > +
> > + return 0;
> > }
> >
> > -static void vmemmap_remap_range(unsigned long start, unsigned long end,
> > - struct vmemmap_remap_walk *walk)
> > +static int vmemmap_remap_range(unsigned long start, unsigned long end,
> > + struct vmemmap_remap_walk *walk)
> > {
> > unsigned long addr = start;
> > unsigned long next;
> > @@ -131,8 +187,12 @@ static void vmemmap_remap_range(unsigned long start, unsigned long end,
> >
> > pgd = pgd_offset_k(addr);
> > do {
> > + int ret;
> > +
> > next = pgd_addr_end(addr, end);
> > - vmemmap_p4d_range(pgd, addr, next, walk);
> > + ret = vmemmap_p4d_range(pgd, addr, next, walk);
> > + if (ret)
> > + return ret;
> > } while (pgd++, addr = next, addr != end);
> >
> > /*
> > @@ -141,6 +201,8 @@ static void vmemmap_remap_range(unsigned long start, unsigned long end,
> > * belongs to the range.
> > */
> > flush_tlb_kernel_range(start + PAGE_SIZE, end);
> > +
> > + return 0;
> > }
> >
> > /*
> > @@ -179,10 +241,27 @@ static void vmemmap_remap_pte(pte_t *pte, unsigned long addr,
> > pte_t entry = mk_pte(walk->reuse_page, pgprot);
> > struct page *page = pte_page(*pte);
> >
> > - list_add(&page->lru, walk->vmemmap_pages);
> > + list_add_tail(&page->lru, walk->vmemmap_pages);
> > set_pte_at(&init_mm, addr, pte, entry);
> > }
> >
> > +static void vmemmap_restore_pte(pte_t *pte, unsigned long addr,
> > + struct vmemmap_remap_walk *walk)
> > +{
> > + pgprot_t pgprot = PAGE_KERNEL;
> > + struct page *page;
> > + void *to;
> > +
> > + BUG_ON(pte_page(*pte) != walk->reuse_page);
> > +
> > + page = list_first_entry(walk->vmemmap_pages, struct page, lru);
> > + list_del(&page->lru);
> > + to = page_to_virt(page);
> > + copy_page(to, (void *)walk->reuse_addr);
> > +
> > + set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
> > +}
> > +
> > /**
> > * vmemmap_remap_free - remap the vmemmap virtual address range [@start, @end)
> > * to the page which @reuse is mapped to, then free vmemmap
> > @@ -193,12 +272,12 @@ static void vmemmap_remap_pte(pte_t *pte, unsigned long addr,
> > * remap.
> > * @reuse: reuse address.
> > *
> > - * Note: This function depends on vmemmap being base page mapped. Please make
> > - * sure that we disable PMD mapping of vmemmap pages when calling this function.
> > + * Return: %0 on success, negative error code otherwise.
> > */
> > -void vmemmap_remap_free(unsigned long start, unsigned long end,
> > - unsigned long reuse)
> > +int vmemmap_remap_free(unsigned long start, unsigned long end,
> > + unsigned long reuse)
> > {
> > + int ret;
> > LIST_HEAD(vmemmap_pages);
> > struct vmemmap_remap_walk walk = {
> > .remap_pte = vmemmap_remap_pte,
> > @@ -221,25 +300,25 @@ void vmemmap_remap_free(unsigned long start, unsigned long end,
> > */
> > BUG_ON(start - reuse != PAGE_SIZE);
> >
> > - vmemmap_remap_range(reuse, end, &walk);
> > - free_vmemmap_page_list(&vmemmap_pages);
> > -}
> > + mmap_write_lock(&init_mm);
> > + ret = vmemmap_remap_range(reuse, end, &walk);
> > + mmap_write_downgrade(&init_mm);
> >
> > -static void vmemmap_restore_pte(pte_t *pte, unsigned long addr,
> > - struct vmemmap_remap_walk *walk)
> > -{
> > - pgprot_t pgprot = PAGE_KERNEL;
> > - struct page *page;
> > - void *to;
> > + if (ret && walk.walked_pte) {
> > + end = reuse + walk.walked_pte * PAGE_SIZE;
>
> Might be good to have a comment saying:
>
> /*
> * vmemmap_pages contains pages from the previous
> * vmemmap_remap_range call which failed. These
> * are pages which were removed from the vmemmap.
> * They will be restored in the following call.
> */

More clear. Will do.

>
> Code looks good and I like that changes were mostly isolated to
> sparse-vmemmap.c.
>
> Reviewed-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>

Thanks Mike.

>
> It still would be good if someone else takes a look at these changes.
> --
> Mike Kravetz
>
> > + walk = (struct vmemmap_remap_walk) {
> > + .remap_pte = vmemmap_restore_pte,
> > + .reuse_addr = reuse,
> > + .vmemmap_pages = &vmemmap_pages,
> > + };
> >
> > - BUG_ON(pte_page(*pte) != walk->reuse_page);
> > + vmemmap_remap_range(reuse, end, &walk);
> > + }
> > + mmap_read_unlock(&init_mm);
> >
> > - page = list_first_entry(walk->vmemmap_pages, struct page, lru);
> > - list_del(&page->lru);
> > - to = page_to_virt(page);
> > - copy_page(to, (void *)walk->reuse_addr);
> > + free_vmemmap_page_list(&vmemmap_pages);
> >
> > - set_pte_at(&init_mm, addr, pte, mk_pte(page, pgprot));
> > + return ret;
> > }
> >
> > static int alloc_vmemmap_page_list(unsigned long start, unsigned long end,
> > @@ -273,6 +352,8 @@ static int alloc_vmemmap_page_list(unsigned long start, unsigned long end,
> > * remap.
> > * @reuse: reuse address.
> > * @gpf_mask: GFP flag for allocating vmemmap pages.
> > + *
> > + * Return: %0 on success, negative error code otherwise.
> > */
> > int vmemmap_remap_alloc(unsigned long start, unsigned long end,
> > unsigned long reuse, gfp_t gfp_mask)
> > @@ -287,12 +368,12 @@ int vmemmap_remap_alloc(unsigned long start, unsigned long end,
> > /* See the comment in the vmemmap_remap_free(). */
> > BUG_ON(start - reuse != PAGE_SIZE);
> >
> > - might_sleep_if(gfpflags_allow_blocking(gfp_mask));
> > -
> > if (alloc_vmemmap_page_list(start, end, gfp_mask, &vmemmap_pages))
> > return -ENOMEM;
> >
> > + mmap_read_lock(&init_mm);
> > vmemmap_remap_range(reuse, end, &walk);
> > + mmap_read_unlock(&init_mm);
> >
> > return 0;
> > }
> >

Next message: Tzung-Bi Shih: "Re: [PATCH] drm/bridge: anx7625: Make hpd workqueue freezable"
Previous message: Andy Lutomirski: "[PATCH 8/8] membarrier: Rewrite sync_core_before_usermode() and improve documentation"
In reply to: Mike Kravetz: "Re: [PATCH v2 1/3] mm: sparsemem: split the huge PMD mapping of vmemmap pages"
Next in thread: Andrew Morton: "Re: [PATCH v2 0/3] Split huge PMD mapping of vmemmap pages"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]