| Message ID | 20210917034815.80264-4-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Free the 2nd vmemmap page associated with each HugeTLB page | expand |
On Sat, Sep 18, 2021 at 12:09 AM Muchun Song <songmuchun@bytedance.com> wrote: > > The init_mm.page_table_lock is used to protect kernel page tables, we > can use it to serialize splitting vmemmap PMD mappings instead of mmap > write lock, which can increase the concurrency of vmemmap_remap_free(). > Curious what is the actual benefit we get in user scenarios from this patch, 1. we set bootargs to reserve hugetlb statically 2. we "echo" some figures to sys or proc. In other words, Who is going to care about this concurrency? Can we have some details on this to put in the commit log? > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > mm/ptdump.c | 16 ++++++++++++---- > mm/sparse-vmemmap.c | 49 ++++++++++++++++++++++++++++++++++--------------- > 2 files changed, 46 insertions(+), 19 deletions(-) > > diff --git a/mm/ptdump.c b/mm/ptdump.c > index da751448d0e4..eea3d28d173c 100644 > --- a/mm/ptdump.c > +++ b/mm/ptdump.c > @@ -40,8 +40,10 @@ static int ptdump_pgd_entry(pgd_t *pgd, unsigned long addr, > if (st->effective_prot) > st->effective_prot(st, 0, pgd_val(val)); > > - if (pgd_leaf(val)) > + if (pgd_leaf(val)) { > st->note_page(st, addr, 0, pgd_val(val)); > + walk->action = ACTION_CONTINUE; > + } > > return 0; > } > @@ -61,8 +63,10 @@ static int ptdump_p4d_entry(p4d_t *p4d, unsigned long addr, > if (st->effective_prot) > st->effective_prot(st, 1, p4d_val(val)); > > - if (p4d_leaf(val)) > + if (p4d_leaf(val)) { > st->note_page(st, addr, 1, p4d_val(val)); > + walk->action = ACTION_CONTINUE; > + } > > return 0; > } > @@ -82,8 +86,10 @@ static int ptdump_pud_entry(pud_t *pud, unsigned long addr, > if (st->effective_prot) > st->effective_prot(st, 2, pud_val(val)); > > - if (pud_leaf(val)) > + if (pud_leaf(val)) { > st->note_page(st, addr, 2, pud_val(val)); > + walk->action = ACTION_CONTINUE; > + } > > return 0; > } > @@ -101,8 +107,10 @@ static int ptdump_pmd_entry(pmd_t *pmd, unsigned long addr, > > if (st->effective_prot) > st->effective_prot(st, 3, pmd_val(val)); > - if (pmd_leaf(val)) > + if (pmd_leaf(val)) { > st->note_page(st, addr, 3, pmd_val(val)); > + walk->action = ACTION_CONTINUE; > + } > > return 0; > } > diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c > index 62e3d20648ce..e636943ccfc4 100644 > --- a/mm/sparse-vmemmap.c > +++ b/mm/sparse-vmemmap.c > @@ -64,8 +64,8 @@ struct vmemmap_remap_walk { > */ > #define NR_RESET_STRUCT_PAGE 3 > > -static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, > - struct vmemmap_remap_walk *walk) > +static int __split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, > + struct vmemmap_remap_walk *walk) > { > pmd_t __pmd; > int i; > @@ -87,15 +87,37 @@ static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, > 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); > + spin_lock(&init_mm.page_table_lock); > + if (likely(pmd_leaf(*pmd))) { > + /* 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); > + spin_unlock(&init_mm.page_table_lock); > > - flush_tlb_kernel_range(start, start + PMD_SIZE); > + return 0; > + } > + spin_unlock(&init_mm.page_table_lock); > + pte_free_kernel(&init_mm, pgtable); > > return 0; > } > > +static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, > + struct vmemmap_remap_walk *walk) > +{ > + int ret; > + > + spin_lock(&init_mm.page_table_lock); > + ret = pmd_leaf(*pmd); > + spin_unlock(&init_mm.page_table_lock); > + > + if (ret) > + ret = __split_vmemmap_huge_pmd(pmd, start, walk); > + > + return ret; > +} > + > static void vmemmap_pte_range(pmd_t *pmd, unsigned long addr, > unsigned long end, > struct vmemmap_remap_walk *walk) > @@ -132,13 +154,12 @@ static int vmemmap_pmd_range(pud_t *pud, unsigned long addr, > > pmd = pmd_offset(pud, addr); > do { > - if (pmd_leaf(*pmd)) { > - int ret; > + int ret; > + > + ret = split_vmemmap_huge_pmd(pmd, addr & PMD_MASK, walk); > + if (ret) > + return 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); > @@ -321,10 +342,8 @@ int vmemmap_remap_free(unsigned long start, unsigned long end, > */ > BUG_ON(start - reuse != PAGE_SIZE); > > - mmap_write_lock(&init_mm); > + mmap_read_lock(&init_mm); > ret = vmemmap_remap_range(reuse, end, &walk); > - mmap_write_downgrade(&init_mm); > - > if (ret && walk.nr_walked) { > end = reuse + walk.nr_walked * PAGE_SIZE; > /* > -- > 2.11.0 > Thanks barry
On Sat, Sep 18, 2021 at 1:07 PM Barry Song <21cnbao@gmail.com> wrote: > > On Sat, Sep 18, 2021 at 12:09 AM Muchun Song <songmuchun@bytedance.com> wrote: > > > > The init_mm.page_table_lock is used to protect kernel page tables, we > > can use it to serialize splitting vmemmap PMD mappings instead of mmap > > write lock, which can increase the concurrency of vmemmap_remap_free(). > > > > Curious what is the actual benefit we get in user scenarios from this patch, > 1. we set bootargs to reserve hugetlb statically > 2. we "echo" some figures to sys or proc. > > In other words, Who is going to care about this concurrency? Actually, It increase the concurrency between allocations of HugeTLB pages. But it is not my first consideration. There are a lot of users of mmap read lock of init_mm. The mmap write lock is holding through vmemmap_remap_free(), I want to make it does not affect other users of mmap read lock. I suppose a lot of developers are trying to avoid using mmap write lock. I am also one of them. > Can we have some details on this to put in the commit log? For sure. Those judgments above should be placed in the commit log. Thanks.
On Sat, Sep 18, 2021 at 10:51 PM Muchun Song <songmuchun@bytedance.com> wrote: > > On Sat, Sep 18, 2021 at 1:07 PM Barry Song <21cnbao@gmail.com> wrote: > > > > On Sat, Sep 18, 2021 at 12:09 AM Muchun Song <songmuchun@bytedance.com> wrote: > > > > > > The init_mm.page_table_lock is used to protect kernel page tables, we > > > can use it to serialize splitting vmemmap PMD mappings instead of mmap > > > write lock, which can increase the concurrency of vmemmap_remap_free(). > > > > > > > Curious what is the actual benefit we get in user scenarios from this patch, > > 1. we set bootargs to reserve hugetlb statically > > 2. we "echo" some figures to sys or proc. > > > > In other words, Who is going to care about this concurrency? > > Actually, It increase the concurrency between allocations of > HugeTLB pages. But it is not my first consideration. There are > a lot of users of mmap read lock of init_mm. The mmap write > lock is holding through vmemmap_remap_free(), I want to make > it does not affect other users of mmap read lock. generically makes sense. I guess it wouldn't be critical at all for hugetlb allocation as practically we are not going to reserve and release hugtlb often as they are not THP. anyway, it is not making anything worse and always a win to move. > > I suppose a lot of developers are trying to avoid using mmap write > lock. I am also one of them. > > > Can we have some details on this to put in the commit log? > > For sure. Those judgments above should be placed in the > commit log. > > Thanks. Thanks barry
diff --git a/mm/ptdump.c b/mm/ptdump.c index da751448d0e4..eea3d28d173c 100644 --- a/mm/ptdump.c +++ b/mm/ptdump.c @@ -40,8 +40,10 @@ static int ptdump_pgd_entry(pgd_t *pgd, unsigned long addr, if (st->effective_prot) st->effective_prot(st, 0, pgd_val(val)); - if (pgd_leaf(val)) + if (pgd_leaf(val)) { st->note_page(st, addr, 0, pgd_val(val)); + walk->action = ACTION_CONTINUE; + } return 0; } @@ -61,8 +63,10 @@ static int ptdump_p4d_entry(p4d_t *p4d, unsigned long addr, if (st->effective_prot) st->effective_prot(st, 1, p4d_val(val)); - if (p4d_leaf(val)) + if (p4d_leaf(val)) { st->note_page(st, addr, 1, p4d_val(val)); + walk->action = ACTION_CONTINUE; + } return 0; } @@ -82,8 +86,10 @@ static int ptdump_pud_entry(pud_t *pud, unsigned long addr, if (st->effective_prot) st->effective_prot(st, 2, pud_val(val)); - if (pud_leaf(val)) + if (pud_leaf(val)) { st->note_page(st, addr, 2, pud_val(val)); + walk->action = ACTION_CONTINUE; + } return 0; } @@ -101,8 +107,10 @@ static int ptdump_pmd_entry(pmd_t *pmd, unsigned long addr, if (st->effective_prot) st->effective_prot(st, 3, pmd_val(val)); - if (pmd_leaf(val)) + if (pmd_leaf(val)) { st->note_page(st, addr, 3, pmd_val(val)); + walk->action = ACTION_CONTINUE; + } return 0; } diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c index 62e3d20648ce..e636943ccfc4 100644 --- a/mm/sparse-vmemmap.c +++ b/mm/sparse-vmemmap.c @@ -64,8 +64,8 @@ struct vmemmap_remap_walk { */ #define NR_RESET_STRUCT_PAGE 3 -static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, - struct vmemmap_remap_walk *walk) +static int __split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, + struct vmemmap_remap_walk *walk) { pmd_t __pmd; int i; @@ -87,15 +87,37 @@ static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, 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); + spin_lock(&init_mm.page_table_lock); + if (likely(pmd_leaf(*pmd))) { + /* 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); + spin_unlock(&init_mm.page_table_lock); - flush_tlb_kernel_range(start, start + PMD_SIZE); + return 0; + } + spin_unlock(&init_mm.page_table_lock); + pte_free_kernel(&init_mm, pgtable); return 0; } +static int split_vmemmap_huge_pmd(pmd_t *pmd, unsigned long start, + struct vmemmap_remap_walk *walk) +{ + int ret; + + spin_lock(&init_mm.page_table_lock); + ret = pmd_leaf(*pmd); + spin_unlock(&init_mm.page_table_lock); + + if (ret) + ret = __split_vmemmap_huge_pmd(pmd, start, walk); + + return ret; +} + static void vmemmap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end, struct vmemmap_remap_walk *walk) @@ -132,13 +154,12 @@ static int vmemmap_pmd_range(pud_t *pud, unsigned long addr, pmd = pmd_offset(pud, addr); do { - if (pmd_leaf(*pmd)) { - int ret; + int ret; + + ret = split_vmemmap_huge_pmd(pmd, addr & PMD_MASK, walk); + if (ret) + return 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); @@ -321,10 +342,8 @@ int vmemmap_remap_free(unsigned long start, unsigned long end, */ BUG_ON(start - reuse != PAGE_SIZE); - mmap_write_lock(&init_mm); + mmap_read_lock(&init_mm); ret = vmemmap_remap_range(reuse, end, &walk); - mmap_write_downgrade(&init_mm); - if (ret && walk.nr_walked) { end = reuse + walk.nr_walked * PAGE_SIZE; /*
The init_mm.page_table_lock is used to protect kernel page tables, we can use it to serialize splitting vmemmap PMD mappings instead of mmap write lock, which can increase the concurrency of vmemmap_remap_free(). Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- mm/ptdump.c | 16 ++++++++++++---- mm/sparse-vmemmap.c | 49 ++++++++++++++++++++++++++++++++++--------------- 2 files changed, 46 insertions(+), 19 deletions(-)