[v2,1/1] mm/madvise: enhance lazyfreeing with mTHP in madvise_free

Commit Message

Lance Yang March 7, 2024, 6:14 a.m. UTC

This patch optimizes lazyfreeing with PTE-mapped mTHP[1]
(Inspired by David Hildenbrand[2]). We aim to avoid unnecessary
folio splitting if the large folio is entirely within the given
range.

On an Intel I5 CPU, lazyfreeing a 1GiB VMA backed by
PTE-mapped folios of the same size results in the following
runtimes for madvise(MADV_FREE) in seconds (shorter is better):

Folio Size |   Old    |   New    | Change
------------------------------------------
      4KiB | 0.590251 | 0.590259 |    0%
     16KiB | 2.990447 | 0.185655 |  -94%
     32KiB | 2.547831 | 0.104870 |  -95%
     64KiB | 2.457796 | 0.052812 |  -97%
    128KiB | 2.281034 | 0.032777 |  -99%
    256KiB | 2.230387 | 0.017496 |  -99%
    512KiB | 2.189106 | 0.010781 |  -99%
   1024KiB | 2.183949 | 0.007753 |  -99%
   2048KiB | 0.002799 | 0.002804 |    0%

[1] https://lkml.kernel.org/r/[e-mail]
[2] https://lore.kernel.org/linux-mm/[e-mail]/

Signed-off-by: Lance Yang <[e-mail]>
---
v1 -> v2:
 * Update the performance numbers
 * Update the changelog, suggested by Ryan Roberts
 * Check the COW folio, suggested by Yin Fengwei
 * Check if we are mapping all subpages, suggested by Barry Song,
 David Hildenbrand, Ryan Roberts
 * https://lore.kernel.org/linux-mm/[e-mail]/

 mm/madvise.c | 85 +++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 11 deletions(-)

Comments

Barry Song March 7, 2024, 7 a.m. UTC | #1

On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>
> This patch optimizes lazyfreeing with PTE-mapped mTHP[1]
> (Inspired by David Hildenbrand[2]). We aim to avoid unnecessary
> folio splitting if the large folio is entirely within the given
> range.
>
> On an Intel I5 CPU, lazyfreeing a 1GiB VMA backed by
> PTE-mapped folios of the same size results in the following
> runtimes for madvise(MADV_FREE) in seconds (shorter is better):
>
> Folio Size |   Old    |   New    | Change
> ------------------------------------------
>       4KiB | 0.590251 | 0.590259 |    0%
>      16KiB | 2.990447 | 0.185655 |  -94%
>      32KiB | 2.547831 | 0.104870 |  -95%
>      64KiB | 2.457796 | 0.052812 |  -97%
>     128KiB | 2.281034 | 0.032777 |  -99%
>     256KiB | 2.230387 | 0.017496 |  -99%
>     512KiB | 2.189106 | 0.010781 |  -99%
>    1024KiB | 2.183949 | 0.007753 |  -99%
>    2048KiB | 0.002799 | 0.002804 |    0%
>
> [1] https://lkml.kernel.org/r/[e-mail]
> [2] https://lore.kernel.org/linux-mm/[e-mail]/
>
> Signed-off-by: Lance Yang <[e-mail]>
> ---
> v1 -> v2:
>  * Update the performance numbers
>  * Update the changelog, suggested by Ryan Roberts
>  * Check the COW folio, suggested by Yin Fengwei
>  * Check if we are mapping all subpages, suggested by Barry Song,
>  David Hildenbrand, Ryan Roberts
>  * https://lore.kernel.org/linux-mm/[e-mail]/
>
>  mm/madvise.c | 85 +++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 74 insertions(+), 11 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 44a498c94158..1437ac6eb25e 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -616,6 +616,20 @@ static long madvise_pageout(struct vm_area_struct *vma,
>         return 0;
>  }
>
> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> +                                                struct folio *folio, pte_t *start_pte)
> +{
> +       int nr_pages = folio_nr_pages(folio);
> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> +
> +       for (int i = 0; i < nr_pages; i++)
> +               if (page_mapcount(folio_page(folio, i)) != 1)
> +                       return false;

we have moved to folio_estimated_sharers though it is not precise, so
we don't do
this check with lots of loops and depending on the subpage's mapcount.
BTW, do we need to rebase our work against David's changes[1]?
[1] https://lore.kernel.org/linux-mm/[e-mail]/

> +
> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
> +                                        ptep_get(start_pte), nr_pages, flags, NULL);
> +}
> +
>  static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>                                 unsigned long end, struct mm_walk *walk)
>
> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>                  */
>                 if (folio_test_large(folio)) {
>                         int err;
> +                       unsigned long next_addr, align;
>
> -                       if (folio_estimated_sharers(folio) != 1)
> -                               break;
> -                       if (!folio_trylock(folio))
> -                               break;
> +                       if (folio_estimated_sharers(folio) != 1 ||
> +                           !folio_trylock(folio))
> +                               goto skip_large_folio;


I don't think we can skip all the PTEs for nr_pages, as some of them might be
pointing to other folios.

for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
and PTE16 will point to two different small folios. We can only skip when we
are sure nr_pages == folio_pte_batch() is sure.

> +
> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
> +                       next_addr = ALIGN_DOWN(addr + align, align);
> +
> +                       /*
> +                        * If we mark only the subpages as lazyfree, or
> +                        * cannot mark the entire large folio as lazyfree,
> +                        * then just split it.
> +                        */
> +                       if (next_addr > end || next_addr - addr != align ||
> +                           !can_mark_large_folio_lazyfree(addr, folio, pte))
> +                               goto split_large_folio;
> +
> +                       /*
> +                        * Avoid unnecessary folio splitting if the large
> +                        * folio is entirely within the given range.
> +                        */
> +                       folio_clear_dirty(folio);
> +                       folio_unlock(folio);
> +                       for (; addr != next_addr; pte++, addr += PAGE_SIZE) {
> +                               ptent = ptep_get(pte);
> +                               if (pte_young(ptent) || pte_dirty(ptent)) {
> +                                       ptent = ptep_get_and_clear_full(
> +                                               mm, addr, pte, tlb->fullmm);
> +                                       ptent = pte_mkold(ptent);
> +                                       ptent = pte_mkclean(ptent);
> +                                       set_pte_at(mm, addr, pte, ptent);
> +                                       tlb_remove_tlb_entry(tlb, pte, addr);
> +                               }

Can we do this in batches? for a CONT-PTE mapped large folio, you are unfolding
and folding again. It seems quite expensive.

> +                       }
> +                       folio_mark_lazyfree(folio);
> +                       goto next_folio;
> +
> +split_large_folio:
>                         folio_get(folio);
>                         arch_leave_lazy_mmu_mode();
>                         pte_unmap_unlock(start_pte, ptl);
> @@ -688,13 +736,28 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>                         err = split_folio(folio);
>                         folio_unlock(folio);
>                         folio_put(folio);
> -                       if (err)
> -                               break;
> -                       start_pte = pte =
> -                               pte_offset_map_lock(mm, pmd, addr, &ptl);
> -                       if (!start_pte)
> -                               break;
> -                       arch_enter_lazy_mmu_mode();
> +
> +                       /*
> +                        * If the large folio is locked or cannot be split,
> +                        * we just skip it.
> +                        */
> +                       if (err) {
> +skip_large_folio:
> +                               if (next_addr >= end)
> +                                       break;
> +                               pte += (next_addr - addr) / PAGE_SIZE;
> +                               addr = next_addr;
> +                       }
> +
> +                       if (!start_pte) {
> +                               start_pte = pte = pte_offset_map_lock(
> +                                       mm, pmd, addr, &ptl);
> +                               if (!start_pte)
> +                                       break;
> +                               arch_enter_lazy_mmu_mode();
> +                       }
> +
> +next_folio:
>                         pte--;
>                         addr -= PAGE_SIZE;
>                         continue;
> --
> 2.33.1
>

Thanks
Barry

Lance Yang March 7, 2024, 8 a.m. UTC | #2

Hey Barry,

Thanks for taking time to review!

On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>
> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
> >
[...]
> > +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> > +                                                struct folio *folio, pte_t *start_pte)
> > +{
> > +       int nr_pages = folio_nr_pages(folio);
> > +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> > +
> > +       for (int i = 0; i < nr_pages; i++)
> > +               if (page_mapcount(folio_page(folio, i)) != 1)
> > +                       return false;
>
> we have moved to folio_estimated_sharers though it is not precise, so
> we don't do
> this check with lots of loops and depending on the subpage's mapcount.

If we don't check the subpage’s mapcount, and there is a cow folio associated
with this folio and the cow folio has smaller size than this folio,
should we still
mark this folio as lazyfree?

> BTW, do we need to rebase our work against David's changes[1]?
> [1] https://lore.kernel.org/linux-mm/[e-mail]/

Yes, we should rebase our work against David’s changes.

>
> > +
> > +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
> > +                                        ptep_get(start_pte), nr_pages, flags, NULL);
> > +}
> > +
> >  static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >                                 unsigned long end, struct mm_walk *walk)
> >
> > @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >                  */
> >                 if (folio_test_large(folio)) {
> >                         int err;
> > +                       unsigned long next_addr, align;
> >
> > -                       if (folio_estimated_sharers(folio) != 1)
> > -                               break;
> > -                       if (!folio_trylock(folio))
> > -                               break;
> > +                       if (folio_estimated_sharers(folio) != 1 ||
> > +                           !folio_trylock(folio))
> > +                               goto skip_large_folio;
>
>
> I don't think we can skip all the PTEs for nr_pages, as some of them might be
> pointing to other folios.
>
> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
> and PTE16 will point to two different small folios. We can only skip when we
> are sure nr_pages == folio_pte_batch() is sure.

Agreed. Thanks for pointing that out.

>
> > +
> > +                       align = folio_nr_pages(folio) * PAGE_SIZE;
> > +                       next_addr = ALIGN_DOWN(addr + align, align);
> > +
> > +                       /*
> > +                        * If we mark only the subpages as lazyfree, or
> > +                        * cannot mark the entire large folio as lazyfree,
> > +                        * then just split it.
> > +                        */
> > +                       if (next_addr > end || next_addr - addr != align ||
> > +                           !can_mark_large_folio_lazyfree(addr, folio, pte))
> > +                               goto split_large_folio;
> > +
> > +                       /*
> > +                        * Avoid unnecessary folio splitting if the large
> > +                        * folio is entirely within the given range.
> > +                        */
> > +                       folio_clear_dirty(folio);
> > +                       folio_unlock(folio);
> > +                       for (; addr != next_addr; pte++, addr += PAGE_SIZE) {
> > +                               ptent = ptep_get(pte);
> > +                               if (pte_young(ptent) || pte_dirty(ptent)) {
> > +                                       ptent = ptep_get_and_clear_full(
> > +                                               mm, addr, pte, tlb->fullmm);
> > +                                       ptent = pte_mkold(ptent);
> > +                                       ptent = pte_mkclean(ptent);
> > +                                       set_pte_at(mm, addr, pte, ptent);
> > +                                       tlb_remove_tlb_entry(tlb, pte, addr);
> > +                               }
>
> Can we do this in batches? for a CONT-PTE mapped large folio, you are unfolding
> and folding again. It seems quite expensive.

Thanks for your suggestion. I'll do this in batches in v3.

Thanks again for your time!

Best,
Lance

>
> > +                       }
> > +                       folio_mark_lazyfree(folio);
> > +                       goto next_folio;
> > +
> > +split_large_folio:
> >                         folio_get(folio);
> >                         arch_leave_lazy_mmu_mode();
> >                         pte_unmap_unlock(start_pte, ptl);
> > @@ -688,13 +736,28 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >                         err = split_folio(folio);
> >                         folio_unlock(folio);
> >                         folio_put(folio);
> > -                       if (err)
> > -                               break;
> > -                       start_pte = pte =
> > -                               pte_offset_map_lock(mm, pmd, addr, &ptl);
> > -                       if (!start_pte)
> > -                               break;
> > -                       arch_enter_lazy_mmu_mode();
> > +
> > +                       /*
> > +                        * If the large folio is locked or cannot be split,
> > +                        * we just skip it.
> > +                        */
> > +                       if (err) {
> > +skip_large_folio:
> > +                               if (next_addr >= end)
> > +                                       break;
> > +                               pte += (next_addr - addr) / PAGE_SIZE;
> > +                               addr = next_addr;
> > +                       }
> > +
> > +                       if (!start_pte) {
> > +                               start_pte = pte = pte_offset_map_lock(
> > +                                       mm, pmd, addr, &ptl);
> > +                               if (!start_pte)
> > +                                       break;
> > +                               arch_enter_lazy_mmu_mode();
> > +                       }
> > +
> > +next_folio:
> >                         pte--;
> >                         addr -= PAGE_SIZE;
> >                         continue;
> > --
> > 2.33.1
> >
>
> Thanks
> Barry

Barry Song March 7, 2024, 8:10 a.m. UTC | #3

On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>
> Hey Barry,
>
> Thanks for taking time to review!
>
> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
> >
> > On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
> > >
> [...]
> > > +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> > > +                                                struct folio *folio, pte_t *start_pte)
> > > +{
> > > +       int nr_pages = folio_nr_pages(folio);
> > > +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> > > +
> > > +       for (int i = 0; i < nr_pages; i++)
> > > +               if (page_mapcount(folio_page(folio, i)) != 1)
> > > +                       return false;
> >
> > we have moved to folio_estimated_sharers though it is not precise, so
> > we don't do
> > this check with lots of loops and depending on the subpage's mapcount.
>
> If we don't check the subpage’s mapcount, and there is a cow folio associated
> with this folio and the cow folio has smaller size than this folio,
> should we still
> mark this folio as lazyfree?

I agree, this is true. However, we've somehow accepted the fact that
folio_likely_mapped_shared
can result in false negatives or false positives to balance the
overhead.  So I really don't know :-)

Maybe David and Vishal can give some comments here.

>
> > BTW, do we need to rebase our work against David's changes[1]?
> > [1] https://lore.kernel.org/linux-mm/[e-mail]/
>
> Yes, we should rebase our work against David’s changes.
>
> >
> > > +
> > > +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
> > > +                                        ptep_get(start_pte), nr_pages, flags, NULL);
> > > +}
> > > +
> > >  static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> > >                                 unsigned long end, struct mm_walk *walk)
> > >
> > > @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> > >                  */
> > >                 if (folio_test_large(folio)) {
> > >                         int err;
> > > +                       unsigned long next_addr, align;
> > >
> > > -                       if (folio_estimated_sharers(folio) != 1)
> > > -                               break;
> > > -                       if (!folio_trylock(folio))
> > > -                               break;
> > > +                       if (folio_estimated_sharers(folio) != 1 ||
> > > +                           !folio_trylock(folio))
> > > +                               goto skip_large_folio;
> >
> >
> > I don't think we can skip all the PTEs for nr_pages, as some of them might be
> > pointing to other folios.
> >
> > for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
> > and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
> > and PTE16 will point to two different small folios. We can only skip when we
> > are sure nr_pages == folio_pte_batch() is sure.
>
> Agreed. Thanks for pointing that out.
>
> >
> > > +
> > > +                       align = folio_nr_pages(folio) * PAGE_SIZE;
> > > +                       next_addr = ALIGN_DOWN(addr + align, align);
> > > +
> > > +                       /*
> > > +                        * If we mark only the subpages as lazyfree, or
> > > +                        * cannot mark the entire large folio as lazyfree,
> > > +                        * then just split it.
> > > +                        */
> > > +                       if (next_addr > end || next_addr - addr != align ||
> > > +                           !can_mark_large_folio_lazyfree(addr, folio, pte))
> > > +                               goto split_large_folio;
> > > +
> > > +                       /*
> > > +                        * Avoid unnecessary folio splitting if the large
> > > +                        * folio is entirely within the given range.
> > > +                        */
> > > +                       folio_clear_dirty(folio);
> > > +                       folio_unlock(folio);
> > > +                       for (; addr != next_addr; pte++, addr += PAGE_SIZE) {
> > > +                               ptent = ptep_get(pte);
> > > +                               if (pte_young(ptent) || pte_dirty(ptent)) {
> > > +                                       ptent = ptep_get_and_clear_full(
> > > +                                               mm, addr, pte, tlb->fullmm);
> > > +                                       ptent = pte_mkold(ptent);
> > > +                                       ptent = pte_mkclean(ptent);
> > > +                                       set_pte_at(mm, addr, pte, ptent);
> > > +                                       tlb_remove_tlb_entry(tlb, pte, addr);
> > > +                               }
> >
> > Can we do this in batches? for a CONT-PTE mapped large folio, you are unfolding
> > and folding again. It seems quite expensive.
>
> Thanks for your suggestion. I'll do this in batches in v3.
>
> Thanks again for your time!
>
> Best,
> Lance
>
> >
> > > +                       }
> > > +                       folio_mark_lazyfree(folio);
> > > +                       goto next_folio;
> > > +
> > > +split_large_folio:
> > >                         folio_get(folio);
> > >                         arch_leave_lazy_mmu_mode();
> > >                         pte_unmap_unlock(start_pte, ptl);
> > > @@ -688,13 +736,28 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> > >                         err = split_folio(folio);
> > >                         folio_unlock(folio);
> > >                         folio_put(folio);
> > > -                       if (err)
> > > -                               break;
> > > -                       start_pte = pte =
> > > -                               pte_offset_map_lock(mm, pmd, addr, &ptl);
> > > -                       if (!start_pte)
> > > -                               break;
> > > -                       arch_enter_lazy_mmu_mode();
> > > +
> > > +                       /*
> > > +                        * If the large folio is locked or cannot be split,
> > > +                        * we just skip it.
> > > +                        */
> > > +                       if (err) {
> > > +skip_large_folio:
> > > +                               if (next_addr >= end)
> > > +                                       break;
> > > +                               pte += (next_addr - addr) / PAGE_SIZE;
> > > +                               addr = next_addr;
> > > +                       }
> > > +
> > > +                       if (!start_pte) {
> > > +                               start_pte = pte = pte_offset_map_lock(
> > > +                                       mm, pmd, addr, &ptl);
> > > +                               if (!start_pte)
> > > +                                       break;
> > > +                               arch_enter_lazy_mmu_mode();
> > > +                       }
> > > +
> > > +next_folio:
> > >                         pte--;
> > >                         addr -= PAGE_SIZE;
> > >                         continue;
> > > --
> > > 2.33.1
> > >

Thanks
Barry

Ryan Roberts March 7, 2024, 9:07 a.m. UTC | #4

On 07/03/2024 08:10, Barry Song wrote:
> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>
>> Hey Barry,
>>
>> Thanks for taking time to review!
>>
>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>
>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>
>> [...]
>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>> +                                                struct folio *folio, pte_t *start_pte)
>>>> +{
>>>> +       int nr_pages = folio_nr_pages(folio);
>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>> +
>>>> +       for (int i = 0; i < nr_pages; i++)
>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>> +                       return false;
>>>
>>> we have moved to folio_estimated_sharers though it is not precise, so
>>> we don't do
>>> this check with lots of loops and depending on the subpage's mapcount.
>>
>> If we don't check the subpage’s mapcount, and there is a cow folio associated
>> with this folio and the cow folio has smaller size than this folio,
>> should we still
>> mark this folio as lazyfree?
> 
> I agree, this is true. However, we've somehow accepted the fact that
> folio_likely_mapped_shared
> can result in false negatives or false positives to balance the
> overhead.  So I really don't know :-)
> 
> Maybe David and Vishal can give some comments here.
> 
>>
>>> BTW, do we need to rebase our work against David's changes[1]?
>>> [1] https://lore.kernel.org/linux-mm/[e-mail]/
>>
>> Yes, we should rebase our work against David’s changes.
>>
>>>
>>>> +
>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>> +                                        ptep_get(start_pte), nr_pages, flags, NULL);
>>>> +}
>>>> +
>>>>  static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>                                 unsigned long end, struct mm_walk *walk)
>>>>
>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>                  */
>>>>                 if (folio_test_large(folio)) {
>>>>                         int err;
>>>> +                       unsigned long next_addr, align;
>>>>
>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>> -                               break;
>>>> -                       if (!folio_trylock(folio))
>>>> -                               break;
>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>> +                           !folio_trylock(folio))
>>>> +                               goto skip_large_folio;
>>>
>>>
>>> I don't think we can skip all the PTEs for nr_pages, as some of them might be
>>> pointing to other folios.
>>>
>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>> and PTE16 will point to two different small folios. We can only skip when we
>>> are sure nr_pages == folio_pte_batch() is sure.
>>
>> Agreed. Thanks for pointing that out.
>>
>>>
>>>> +
>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>> +
>>>> +                       /*
>>>> +                        * If we mark only the subpages as lazyfree, or
>>>> +                        * cannot mark the entire large folio as lazyfree,
>>>> +                        * then just split it.
>>>> +                        */
>>>> +                       if (next_addr > end || next_addr - addr != align ||
>>>> +                           !can_mark_large_folio_lazyfree(addr, folio, pte))
>>>> +                               goto split_large_folio;
>>>> +
>>>> +                       /*
>>>> +                        * Avoid unnecessary folio splitting if the large
>>>> +                        * folio is entirely within the given range.
>>>> +                        */
>>>> +                       folio_clear_dirty(folio);
>>>> +                       folio_unlock(folio);
>>>> +                       for (; addr != next_addr; pte++, addr += PAGE_SIZE) {
>>>> +                               ptent = ptep_get(pte);
>>>> +                               if (pte_young(ptent) || pte_dirty(ptent)) {
>>>> +                                       ptent = ptep_get_and_clear_full(
>>>> +                                               mm, addr, pte, tlb->fullmm);
>>>> +                                       ptent = pte_mkold(ptent);
>>>> +                                       ptent = pte_mkclean(ptent);
>>>> +                                       set_pte_at(mm, addr, pte, ptent);
>>>> +                                       tlb_remove_tlb_entry(tlb, pte, addr);
>>>> +                               }
>>>
>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are unfolding
>>> and folding again. It seems quite expensive.

I'm not convinced we should be doing this in batches. We want the initial
folio_pte_batch() to be as loose as possible regarding permissions so that we
reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
soft dirty, etc). I think it might be possible that some PTEs are RO and other
RW too (e.g. due to cow - although with the current cow impl, probably not. But
its fragile to assume that). Anyway, if we do an initial batch that ignores all
that then do this bit as a batch, you will end up smeering all the ptes with
whatever properties were set on the first pte, which probably isn't right.

I've done a similar conversion for madvise_cold_or_pageout_pte_range() as part
of my swap-out series v4 (hoping to post imminently, but still working out a
latent bug that it triggers). I use ptep_test_and_clear_young() in that, which
arm64 can apply per-pte but avoid doing a contpte unfold/fold. I know you have
to clear dirty here too, but I think this pattern is preferable.

FYI, my swap-out series also halfway-batches madvise_free_pte_range() so that I
can batch free_swap_and_cache() for the swap entry case. Ideally the work you
are doing here would be rebased on top of that and plug-in to the approach
implemented there. (subject to others' views of course).

I'll cc you when I post it.

>>
>> Thanks for your suggestion. I'll do this in batches in v3.
>>
>> Thanks again for your time!
>>
>> Best,
>> Lance
>>
>>>
>>>> +                       }
>>>> +                       folio_mark_lazyfree(folio);
>>>> +                       goto next_folio;
>>>> +
>>>> +split_large_folio:
>>>>                         folio_get(folio);
>>>>                         arch_leave_lazy_mmu_mode();
>>>>                         pte_unmap_unlock(start_pte, ptl);
>>>> @@ -688,13 +736,28 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>                         err = split_folio(folio);
>>>>                         folio_unlock(folio);
>>>>                         folio_put(folio);
>>>> -                       if (err)
>>>> -                               break;
>>>> -                       start_pte = pte =
>>>> -                               pte_offset_map_lock(mm, pmd, addr, &ptl);
>>>> -                       if (!start_pte)
>>>> -                               break;
>>>> -                       arch_enter_lazy_mmu_mode();
>>>> +
>>>> +                       /*
>>>> +                        * If the large folio is locked or cannot be split,
>>>> +                        * we just skip it.
>>>> +                        */
>>>> +                       if (err) {
>>>> +skip_large_folio:
>>>> +                               if (next_addr >= end)
>>>> +                                       break;
>>>> +                               pte += (next_addr - addr) / PAGE_SIZE;
>>>> +                               addr = next_addr;
>>>> +                       }
>>>> +
>>>> +                       if (!start_pte) {
>>>> +                               start_pte = pte = pte_offset_map_lock(
>>>> +                                       mm, pmd, addr, &ptl);
>>>> +                               if (!start_pte)
>>>> +                                       break;
>>>> +                               arch_enter_lazy_mmu_mode();
>>>> +                       }
>>>> +
>>>> +next_folio:
>>>>                         pte--;
>>>>                         addr -= PAGE_SIZE;
>>>>                         continue;
>>>> --
>>>> 2.33.1
>>>>
> 
> Thanks
> Barry

Barry Song March 7, 2024, 9:33 a.m. UTC | #5

On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>
> On 07/03/2024 08:10, Barry Song wrote:
> > On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
> >>
> >> Hey Barry,
> >>
> >> Thanks for taking time to review!
> >>
> >> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
> >>>
> >>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
> >>>>
> >> [...]
> >>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> >>>> +                                                struct folio *folio, pte_t *start_pte)
> >>>> +{
> >>>> +       int nr_pages = folio_nr_pages(folio);
> >>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> >>>> +
> >>>> +       for (int i = 0; i < nr_pages; i++)
> >>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
> >>>> +                       return false;
> >>>
> >>> we have moved to folio_estimated_sharers though it is not precise, so
> >>> we don't do
> >>> this check with lots of loops and depending on the subpage's mapcount.
> >>
> >> If we don't check the subpage’s mapcount, and there is a cow folio associated
> >> with this folio and the cow folio has smaller size than this folio,
> >> should we still
> >> mark this folio as lazyfree?
> >
> > I agree, this is true. However, we've somehow accepted the fact that
> > folio_likely_mapped_shared
> > can result in false negatives or false positives to balance the
> > overhead.  So I really don't know :-)
> >
> > Maybe David and Vishal can give some comments here.
> >
> >>
> >>> BTW, do we need to rebase our work against David's changes[1]?
> >>> [1] https://lore.kernel.org/linux-mm/[e-mail]/
> >>
> >> Yes, we should rebase our work against David’s changes.
> >>
> >>>
> >>>> +
> >>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
> >>>> +                                        ptep_get(start_pte), nr_pages, flags, NULL);
> >>>> +}
> >>>> +
> >>>>  static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>                                 unsigned long end, struct mm_walk *walk)
> >>>>
> >>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>                  */
> >>>>                 if (folio_test_large(folio)) {
> >>>>                         int err;
> >>>> +                       unsigned long next_addr, align;
> >>>>
> >>>> -                       if (folio_estimated_sharers(folio) != 1)
> >>>> -                               break;
> >>>> -                       if (!folio_trylock(folio))
> >>>> -                               break;
> >>>> +                       if (folio_estimated_sharers(folio) != 1 ||
> >>>> +                           !folio_trylock(folio))
> >>>> +                               goto skip_large_folio;
> >>>
> >>>
> >>> I don't think we can skip all the PTEs for nr_pages, as some of them might be
> >>> pointing to other folios.
> >>>
> >>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
> >>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
> >>> and PTE16 will point to two different small folios. We can only skip when we
> >>> are sure nr_pages == folio_pte_batch() is sure.
> >>
> >> Agreed. Thanks for pointing that out.
> >>
> >>>
> >>>> +
> >>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
> >>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
> >>>> +
> >>>> +                       /*
> >>>> +                        * If we mark only the subpages as lazyfree, or
> >>>> +                        * cannot mark the entire large folio as lazyfree,
> >>>> +                        * then just split it.
> >>>> +                        */
> >>>> +                       if (next_addr > end || next_addr - addr != align ||
> >>>> +                           !can_mark_large_folio_lazyfree(addr, folio, pte))
> >>>> +                               goto split_large_folio;
> >>>> +
> >>>> +                       /*
> >>>> +                        * Avoid unnecessary folio splitting if the large
> >>>> +                        * folio is entirely within the given range.
> >>>> +                        */
> >>>> +                       folio_clear_dirty(folio);
> >>>> +                       folio_unlock(folio);
> >>>> +                       for (; addr != next_addr; pte++, addr += PAGE_SIZE) {
> >>>> +                               ptent = ptep_get(pte);
> >>>> +                               if (pte_young(ptent) || pte_dirty(ptent)) {
> >>>> +                                       ptent = ptep_get_and_clear_full(
> >>>> +                                               mm, addr, pte, tlb->fullmm);
> >>>> +                                       ptent = pte_mkold(ptent);
> >>>> +                                       ptent = pte_mkclean(ptent);
> >>>> +                                       set_pte_at(mm, addr, pte, ptent);
> >>>> +                                       tlb_remove_tlb_entry(tlb, pte, addr);
> >>>> +                               }
> >>>
> >>> Can we do this in batches? for a CONT-PTE mapped large folio, you are unfolding
> >>> and folding again. It seems quite expensive.
>
> I'm not convinced we should be doing this in batches. We want the initial
> folio_pte_batch() to be as loose as possible regarding permissions so that we
> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
> soft dirty, etc). I think it might be possible that some PTEs are RO and other
> RW too (e.g. due to cow - although with the current cow impl, probably not. But
> its fragile to assume that). Anyway, if we do an initial batch that ignores all

You are correct. I believe this scenario could indeed occur. For instance,
if process A forks process B and then unmaps itself, leaving B as the
sole process owning the large folio.  The current wp_page_reuse() function
will reuse PTE one by one while the specific subpage is written. This can
make a part of PTE writable while the others are read-only.

> that then do this bit as a batch, you will end up smeering all the ptes with
> whatever properties were set on the first pte, which probably isn't right.
>
> I've done a similar conversion for madvise_cold_or_pageout_pte_range() as part
> of my swap-out series v4 (hoping to post imminently, but still working out a
> latent bug that it triggers). I use ptep_test_and_clear_young() in that, which
> arm64 can apply per-pte but avoid doing a contpte unfold/fold. I know you have
> to clear dirty here too, but I think this pattern is preferable.

nice to know ptep_test_and_clear_young() won't unfold and fold CONT-PTE.
I probably have missed this part of your CONT-PTE series as I was quite busy
with others :-)

>
> FYI, my swap-out series also halfway-batches madvise_free_pte_range() so that I
> can batch free_swap_and_cache() for the swap entry case. Ideally the work you
> are doing here would be rebased on top of that and plug-in to the approach
> implemented there. (subject to others' views of course).
>
> I'll cc you when I post it.
>
> >>
> >> Thanks for your suggestion. I'll do this in batches in v3.
> >>
> >> Thanks again for your time!
> >>
> >> Best,
> >> Lance
> >>
> >>>
> >>>> +                       }
> >>>> +                       folio_mark_lazyfree(folio);
> >>>> +                       goto next_folio;
> >>>> +
> >>>> +split_large_folio:
> >>>>                         folio_get(folio);
> >>>>                         arch_leave_lazy_mmu_mode();
> >>>>                         pte_unmap_unlock(start_pte, ptl);
> >>>> @@ -688,13 +736,28 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>                         err = split_folio(folio);
> >>>>                         folio_unlock(folio);
> >>>>                         folio_put(folio);
> >>>> -                       if (err)
> >>>> -                               break;
> >>>> -                       start_pte = pte =
> >>>> -                               pte_offset_map_lock(mm, pmd, addr, &ptl);
> >>>> -                       if (!start_pte)
> >>>> -                               break;
> >>>> -                       arch_enter_lazy_mmu_mode();
> >>>> +
> >>>> +                       /*
> >>>> +                        * If the large folio is locked or cannot be split,
> >>>> +                        * we just skip it.
> >>>> +                        */
> >>>> +                       if (err) {
> >>>> +skip_large_folio:
> >>>> +                               if (next_addr >= end)
> >>>> +                                       break;
> >>>> +                               pte += (next_addr - addr) / PAGE_SIZE;
> >>>> +                               addr = next_addr;
> >>>> +                       }
> >>>> +
> >>>> +                       if (!start_pte) {
> >>>> +                               start_pte = pte = pte_offset_map_lock(
> >>>> +                                       mm, pmd, addr, &ptl);
> >>>> +                               if (!start_pte)
> >>>> +                                       break;
> >>>> +                               arch_enter_lazy_mmu_mode();
> >>>> +                       }
> >>>> +
> >>>> +next_folio:
> >>>>                         pte--;
> >>>>                         addr -= PAGE_SIZE;
> >>>>                         continue;
> >>>> --
> >>>> 2.33.1
> >>>>
> >

Thanks
 Barry

Ryan Roberts March 7, 2024, 10:50 a.m. UTC | #6

On 07/03/2024 09:33, Barry Song wrote:
> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>
>> On 07/03/2024 08:10, Barry Song wrote:
>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>
>>>> Hey Barry,
>>>>
>>>> Thanks for taking time to review!
>>>>
>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>
>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>
>>>> [...]
>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>> +                                                struct folio *folio, pte_t *start_pte)
>>>>>> +{
>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>> +
>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>> +                       return false;
>>>>>
>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>> we don't do
>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>
>>>> If we don't check the subpage’s mapcount, and there is a cow folio associated
>>>> with this folio and the cow folio has smaller size than this folio,
>>>> should we still
>>>> mark this folio as lazyfree?
>>>
>>> I agree, this is true. However, we've somehow accepted the fact that
>>> folio_likely_mapped_shared
>>> can result in false negatives or false positives to balance the
>>> overhead.  So I really don't know :-)
>>>
>>> Maybe David and Vishal can give some comments here.
>>>
>>>>
>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>> [1] https://lore.kernel.org/linux-mm/[e-mail]/
>>>>
>>>> Yes, we should rebase our work against David’s changes.
>>>>
>>>>>
>>>>>> +
>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>> +                                        ptep_get(start_pte), nr_pages, flags, NULL);
>>>>>> +}
>>>>>> +
>>>>>>  static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>                                 unsigned long end, struct mm_walk *walk)
>>>>>>
>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>                  */
>>>>>>                 if (folio_test_large(folio)) {
>>>>>>                         int err;
>>>>>> +                       unsigned long next_addr, align;
>>>>>>
>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>> -                               break;
>>>>>> -                       if (!folio_trylock(folio))
>>>>>> -                               break;
>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>> +                           !folio_trylock(folio))
>>>>>> +                               goto skip_large_folio;
>>>>>
>>>>>
>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them might be
>>>>> pointing to other folios.
>>>>>
>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>> and PTE16 will point to two different small folios. We can only skip when we
>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>
>>>> Agreed. Thanks for pointing that out.
>>>>
>>>>>
>>>>>> +
>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>> +
>>>>>> +                       /*
>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>> +                        * cannot mark the entire large folio as lazyfree,
>>>>>> +                        * then just split it.
>>>>>> +                        */
>>>>>> +                       if (next_addr > end || next_addr - addr != align ||
>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio, pte))
>>>>>> +                               goto split_large_folio;
>>>>>> +
>>>>>> +                       /*
>>>>>> +                        * Avoid unnecessary folio splitting if the large
>>>>>> +                        * folio is entirely within the given range.
>>>>>> +                        */
>>>>>> +                       folio_clear_dirty(folio);
>>>>>> +                       folio_unlock(folio);
>>>>>> +                       for (; addr != next_addr; pte++, addr += PAGE_SIZE) {
>>>>>> +                               ptent = ptep_get(pte);
>>>>>> +                               if (pte_young(ptent) || pte_dirty(ptent)) {
>>>>>> +                                       ptent = ptep_get_and_clear_full(
>>>>>> +                                               mm, addr, pte, tlb->fullmm);
>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>> +                                       set_pte_at(mm, addr, pte, ptent);
>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte, addr);
>>>>>> +                               }
>>>>>
>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are unfolding
>>>>> and folding again. It seems quite expensive.
>>
>> I'm not convinced we should be doing this in batches. We want the initial
>> folio_pte_batch() to be as loose as possible regarding permissions so that we
>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
>> soft dirty, etc). I think it might be possible that some PTEs are RO and other
>> RW too (e.g. due to cow - although with the current cow impl, probably not. But
>> its fragile to assume that). Anyway, if we do an initial batch that ignores all
> 
> You are correct. I believe this scenario could indeed occur. For instance,
> if process A forks process B and then unmaps itself, leaving B as the
> sole process owning the large folio.  The current wp_page_reuse() function
> will reuse PTE one by one while the specific subpage is written. 

Hmm - I thought it would only reuse if the total mapcount for the folio was 1.
And since it is a large folio with each page mapped once in proc B, I thought
every subpage write would cause a copy except the last one? I haven't looked at
the code for a while. But I had it in my head that this is an area we need to
improve for mTHP.

> This can
> make a part of PTE writable while the others are read-only.
> 
>> that then do this bit as a batch, you will end up smeering all the ptes with
>> whatever properties were set on the first pte, which probably isn't right.
>>
>> I've done a similar conversion for madvise_cold_or_pageout_pte_range() as part
>> of my swap-out series v4 (hoping to post imminently, but still working out a
>> latent bug that it triggers). I use ptep_test_and_clear_young() in that, which
>> arm64 can apply per-pte but avoid doing a contpte unfold/fold. I know you have
>> to clear dirty here too, but I think this pattern is preferable.
> 
> nice to know ptep_test_and_clear_young() won't unfold and fold CONT-PTE.
> I probably have missed this part of your CONT-PTE series as I was quite busy
> with others :-)
> 
>>
>> FYI, my swap-out series also halfway-batches madvise_free_pte_range() so that I
>> can batch free_swap_and_cache() for the swap entry case. Ideally the work you
>> are doing here would be rebased on top of that and plug-in to the approach
>> implemented there. (subject to others' views of course).
>>
>> I'll cc you when I post it.
>>
>>>>
>>>> Thanks for your suggestion. I'll do this in batches in v3.
>>>>
>>>> Thanks again for your time!
>>>>
>>>> Best,
>>>> Lance
>>>>
>>>>>
>>>>>> +                       }
>>>>>> +                       folio_mark_lazyfree(folio);
>>>>>> +                       goto next_folio;
>>>>>> +
>>>>>> +split_large_folio:
>>>>>>                         folio_get(folio);
>>>>>>                         arch_leave_lazy_mmu_mode();
>>>>>>                         pte_unmap_unlock(start_pte, ptl);
>>>>>> @@ -688,13 +736,28 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>                         err = split_folio(folio);
>>>>>>                         folio_unlock(folio);
>>>>>>                         folio_put(folio);
>>>>>> -                       if (err)
>>>>>> -                               break;
>>>>>> -                       start_pte = pte =
>>>>>> -                               pte_offset_map_lock(mm, pmd, addr, &ptl);
>>>>>> -                       if (!start_pte)
>>>>>> -                               break;
>>>>>> -                       arch_enter_lazy_mmu_mode();
>>>>>> +
>>>>>> +                       /*
>>>>>> +                        * If the large folio is locked or cannot be split,
>>>>>> +                        * we just skip it.
>>>>>> +                        */
>>>>>> +                       if (err) {
>>>>>> +skip_large_folio:
>>>>>> +                               if (next_addr >= end)
>>>>>> +                                       break;
>>>>>> +                               pte += (next_addr - addr) / PAGE_SIZE;
>>>>>> +                               addr = next_addr;
>>>>>> +                       }
>>>>>> +
>>>>>> +                       if (!start_pte) {
>>>>>> +                               start_pte = pte = pte_offset_map_lock(
>>>>>> +                                       mm, pmd, addr, &ptl);
>>>>>> +                               if (!start_pte)
>>>>>> +                                       break;
>>>>>> +                               arch_enter_lazy_mmu_mode();
>>>>>> +                       }
>>>>>> +
>>>>>> +next_folio:
>>>>>>                         pte--;
>>>>>>                         addr -= PAGE_SIZE;
>>>>>>                         continue;
>>>>>> --
>>>>>> 2.33.1
>>>>>>
>>>
> 
> Thanks
>  Barry

David Hildenbrand March 7, 2024, 10:54 a.m. UTC | #7

On 07.03.24 11:50, Ryan Roberts wrote:
> On 07/03/2024 09:33, Barry Song wrote:
>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>
>>> On 07/03/2024 08:10, Barry Song wrote:
>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>
>>>>> Hey Barry,
>>>>>
>>>>> Thanks for taking time to review!
>>>>>
>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>
>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>
>>>>> [...]
>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>> +                                                struct folio *folio, pte_t *start_pte)
>>>>>>> +{
>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>> +
>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>> +                       return false;
>>>>>>
>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>> we don't do
>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>
>>>>> If we don't check the subpage’s mapcount, and there is a cow folio associated
>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>> should we still
>>>>> mark this folio as lazyfree?
>>>>
>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>> folio_likely_mapped_shared
>>>> can result in false negatives or false positives to balance the
>>>> overhead.  So I really don't know :-)
>>>>
>>>> Maybe David and Vishal can give some comments here.
>>>>
>>>>>
>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>> [1] https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>
>>>>> Yes, we should rebase our work against David’s changes.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>> +                                        ptep_get(start_pte), nr_pages, flags, NULL);
>>>>>>> +}
>>>>>>> +
>>>>>>>   static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>                                  unsigned long end, struct mm_walk *walk)
>>>>>>>
>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>                   */
>>>>>>>                  if (folio_test_large(folio)) {
>>>>>>>                          int err;
>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>
>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>> -                               break;
>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>> -                               break;
>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>> +                           !folio_trylock(folio))
>>>>>>> +                               goto skip_large_folio;
>>>>>>
>>>>>>
>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them might be
>>>>>> pointing to other folios.
>>>>>>
>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>> and PTE16 will point to two different small folios. We can only skip when we
>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>
>>>>> Agreed. Thanks for pointing that out.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>> +
>>>>>>> +                       /*
>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>> +                        * cannot mark the entire large folio as lazyfree,
>>>>>>> +                        * then just split it.
>>>>>>> +                        */
>>>>>>> +                       if (next_addr > end || next_addr - addr != align ||
>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio, pte))
>>>>>>> +                               goto split_large_folio;
>>>>>>> +
>>>>>>> +                       /*
>>>>>>> +                        * Avoid unnecessary folio splitting if the large
>>>>>>> +                        * folio is entirely within the given range.
>>>>>>> +                        */
>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>> +                       folio_unlock(folio);
>>>>>>> +                       for (; addr != next_addr; pte++, addr += PAGE_SIZE) {
>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>> +                               if (pte_young(ptent) || pte_dirty(ptent)) {
>>>>>>> +                                       ptent = ptep_get_and_clear_full(
>>>>>>> +                                               mm, addr, pte, tlb->fullmm);
>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>> +                                       set_pte_at(mm, addr, pte, ptent);
>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte, addr);
>>>>>>> +                               }
>>>>>>
>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are unfolding
>>>>>> and folding again. It seems quite expensive.
>>>
>>> I'm not convinced we should be doing this in batches. We want the initial
>>> folio_pte_batch() to be as loose as possible regarding permissions so that we
>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
>>> soft dirty, etc). I think it might be possible that some PTEs are RO and other
>>> RW too (e.g. due to cow - although with the current cow impl, probably not. But
>>> its fragile to assume that). Anyway, if we do an initial batch that ignores all
>>
>> You are correct. I believe this scenario could indeed occur. For instance,
>> if process A forks process B and then unmaps itself, leaving B as the
>> sole process owning the large folio.  The current wp_page_reuse() function
>> will reuse PTE one by one while the specific subpage is written.
> 
> Hmm - I thought it would only reuse if the total mapcount for the folio was 1.
> And since it is a large folio with each page mapped once in proc B, I thought
> every subpage write would cause a copy except the last one? I haven't looked at
> the code for a while. But I had it in my head that this is an area we need to
> improve for mTHP.

wp_page_reuse() will currently reuse a PTE part of a large folio only if 
a single PTE remains mapped (refcount == 0).

David Hildenbrand March 7, 2024, 10:54 a.m. UTC | #8

On 07.03.24 11:54, David Hildenbrand wrote:
> On 07.03.24 11:50, Ryan Roberts wrote:
>> On 07/03/2024 09:33, Barry Song wrote:
>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>
>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>
>>>>>> Hey Barry,
>>>>>>
>>>>>> Thanks for taking time to review!
>>>>>>
>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>
>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>
>>>>>> [...]
>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>> +                                                struct folio *folio, pte_t *start_pte)
>>>>>>>> +{
>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>> +
>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>> +                       return false;
>>>>>>>
>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>> we don't do
>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>
>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio associated
>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>> should we still
>>>>>> mark this folio as lazyfree?
>>>>>
>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>> folio_likely_mapped_shared
>>>>> can result in false negatives or false positives to balance the
>>>>> overhead.  So I really don't know :-)
>>>>>
>>>>> Maybe David and Vishal can give some comments here.
>>>>>
>>>>>>
>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>> [1] https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>
>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>> +                                        ptep_get(start_pte), nr_pages, flags, NULL);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>    static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>                                   unsigned long end, struct mm_walk *walk)
>>>>>>>>
>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>                    */
>>>>>>>>                   if (folio_test_large(folio)) {
>>>>>>>>                           int err;
>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>
>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>> -                               break;
>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>> -                               break;
>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>> +                               goto skip_large_folio;
>>>>>>>
>>>>>>>
>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them might be
>>>>>>> pointing to other folios.
>>>>>>>
>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>> and PTE16 will point to two different small folios. We can only skip when we
>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>
>>>>>> Agreed. Thanks for pointing that out.
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>> +
>>>>>>>> +                       /*
>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>> +                        * cannot mark the entire large folio as lazyfree,
>>>>>>>> +                        * then just split it.
>>>>>>>> +                        */
>>>>>>>> +                       if (next_addr > end || next_addr - addr != align ||
>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio, pte))
>>>>>>>> +                               goto split_large_folio;
>>>>>>>> +
>>>>>>>> +                       /*
>>>>>>>> +                        * Avoid unnecessary folio splitting if the large
>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>> +                        */
>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>> +                       folio_unlock(folio);
>>>>>>>> +                       for (; addr != next_addr; pte++, addr += PAGE_SIZE) {
>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>> +                               if (pte_young(ptent) || pte_dirty(ptent)) {
>>>>>>>> +                                       ptent = ptep_get_and_clear_full(
>>>>>>>> +                                               mm, addr, pte, tlb->fullmm);
>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>> +                                       set_pte_at(mm, addr, pte, ptent);
>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte, addr);
>>>>>>>> +                               }
>>>>>>>
>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are unfolding
>>>>>>> and folding again. It seems quite expensive.
>>>>
>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>> folio_pte_batch() to be as loose as possible regarding permissions so that we
>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and other
>>>> RW too (e.g. due to cow - although with the current cow impl, probably not. But
>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores all
>>>
>>> You are correct. I believe this scenario could indeed occur. For instance,
>>> if process A forks process B and then unmaps itself, leaving B as the
>>> sole process owning the large folio.  The current wp_page_reuse() function
>>> will reuse PTE one by one while the specific subpage is written.
>>
>> Hmm - I thought it would only reuse if the total mapcount for the folio was 1.
>> And since it is a large folio with each page mapped once in proc B, I thought
>> every subpage write would cause a copy except the last one? I haven't looked at
>> the code for a while. But I had it in my head that this is an area we need to
>> improve for mTHP.
> 
> wp_page_reuse() will currently reuse a PTE part of a large folio only if
> a single PTE remains mapped (refcount == 0).

^ == 1

Ryan Roberts March 7, 2024, 11:13 a.m. UTC | #9

On 07/03/2024 10:54, David Hildenbrand wrote:
> On 07.03.24 11:54, David Hildenbrand wrote:
>> On 07.03.24 11:50, Ryan Roberts wrote:
>>> On 07/03/2024 09:33, Barry Song wrote:
>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>>
>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>>
>>>>>>> Hey Barry,
>>>>>>>
>>>>>>> Thanks for taking time to review!
>>>>>>>
>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>
>>>>>>> [...]
>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>>> +                                                struct folio *folio,
>>>>>>>>> pte_t *start_pte)
>>>>>>>>> +{
>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>> +
>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>> +                       return false;
>>>>>>>>
>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>>> we don't do
>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>>
>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>> associated
>>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>>> should we still
>>>>>>> mark this folio as lazyfree?
>>>>>>
>>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>>> folio_likely_mapped_shared
>>>>>> can result in false negatives or false positives to balance the
>>>>>> overhead.  So I really don't know :-)
>>>>>>
>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>
>>>>>>>
>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>> [1]
>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>
>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
>>>>>>>>> flags, NULL);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>    static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>>                                   unsigned long end, struct mm_walk *walk)
>>>>>>>>>
>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
>>>>>>>>> unsigned long addr,
>>>>>>>>>                    */
>>>>>>>>>                   if (folio_test_large(folio)) {
>>>>>>>>>                           int err;
>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>
>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>> -                               break;
>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>> -                               break;
>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>
>>>>>>>>
>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
>>>>>>>> might be
>>>>>>>> pointing to other folios.
>>>>>>>>
>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>>> and PTE16 will point to two different small folios. We can only skip
>>>>>>>> when we
>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>
>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>>> +
>>>>>>>>> +                       /*
>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>>> +                        * cannot mark the entire large folio as lazyfree,
>>>>>>>>> +                        * then just split it.
>>>>>>>>> +                        */
>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
>>>>>>>>> align ||
>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>> pte))
>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>> +
>>>>>>>>> +                       /*
>>>>>>>>> +                        * Avoid unnecessary folio splitting if the large
>>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>>> +                        */
>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>> PAGE_SIZE) {
>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>> +                                       ptent = ptep_get_and_clear_full(
>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>> tlb->fullmm);
>>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>>> +                                       set_pte_at(mm, addr, pte, ptent);
>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>> addr);
>>>>>>>>> +                               }
>>>>>>>>
>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
>>>>>>>> unfolding
>>>>>>>> and folding again. It seems quite expensive.
>>>>>
>>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>>> folio_pte_batch() to be as loose as possible regarding permissions so that we
>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and other
>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
>>>>> But
>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
>>>>> all
>>>>
>>>> You are correct. I believe this scenario could indeed occur. For instance,
>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>> sole process owning the large folio.  The current wp_page_reuse() function
>>>> will reuse PTE one by one while the specific subpage is written.
>>>
>>> Hmm - I thought it would only reuse if the total mapcount for the folio was 1.
>>> And since it is a large folio with each page mapped once in proc B, I thought
>>> every subpage write would cause a copy except the last one? I haven't looked at
>>> the code for a while. But I had it in my head that this is an area we need to
>>> improve for mTHP.
>>
>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
>> a single PTE remains mapped (refcount == 0).
> 
> ^ == 1

Ahh yes. That's what I meant. I got the behacviour vagulely right though.

Anyway, regardless, I'm not sure we want to batch here. Or if we do, we want to
batch function that will only clear access and dirty.

David Hildenbrand March 7, 2024, 11:17 a.m. UTC | #10

On 07.03.24 12:13, Ryan Roberts wrote:
> On 07/03/2024 10:54, David Hildenbrand wrote:
>> On 07.03.24 11:54, David Hildenbrand wrote:
>>> On 07.03.24 11:50, Ryan Roberts wrote:
>>>> On 07/03/2024 09:33, Barry Song wrote:
>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>
>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> Hey Barry,
>>>>>>>>
>>>>>>>> Thanks for taking time to review!
>>>>>>>>
>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>
>>>>>>>> [...]
>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>>>> +                                                struct folio *folio,
>>>>>>>>>> pte_t *start_pte)
>>>>>>>>>> +{
>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>>> +
>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>>> +                       return false;
>>>>>>>>>
>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>>>> we don't do
>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>>>
>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>>> associated
>>>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>>>> should we still
>>>>>>>> mark this folio as lazyfree?
>>>>>>>
>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>>>> folio_likely_mapped_shared
>>>>>>> can result in false negatives or false positives to balance the
>>>>>>> overhead.  So I really don't know :-)
>>>>>>>
>>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>>
>>>>>>>>
>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>>> [1]
>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>>
>>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
>>>>>>>>>> flags, NULL);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>     static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>>>                                    unsigned long end, struct mm_walk *walk)
>>>>>>>>>>
>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
>>>>>>>>>> unsigned long addr,
>>>>>>>>>>                     */
>>>>>>>>>>                    if (folio_test_large(folio)) {
>>>>>>>>>>                            int err;
>>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>>
>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>>> -                               break;
>>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>>> -                               break;
>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
>>>>>>>>> might be
>>>>>>>>> pointing to other folios.
>>>>>>>>>
>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
>>>>>>>>> when we
>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>>
>>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>>>> +
>>>>>>>>>> +                       /*
>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>>>> +                        * cannot mark the entire large folio as lazyfree,
>>>>>>>>>> +                        * then just split it.
>>>>>>>>>> +                        */
>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
>>>>>>>>>> align ||
>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>>> pte))
>>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>>> +
>>>>>>>>>> +                       /*
>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the large
>>>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>>>> +                        */
>>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>>> PAGE_SIZE) {
>>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>>> +                                       ptent = ptep_get_and_clear_full(
>>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>>> tlb->fullmm);
>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>>>> +                                       set_pte_at(mm, addr, pte, ptent);
>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>>> addr);
>>>>>>>>>> +                               }
>>>>>>>>>
>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
>>>>>>>>> unfolding
>>>>>>>>> and folding again. It seems quite expensive.
>>>>>>
>>>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so that we
>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and other
>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
>>>>>> But
>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
>>>>>> all
>>>>>
>>>>> You are correct. I believe this scenario could indeed occur. For instance,
>>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>>> sole process owning the large folio.  The current wp_page_reuse() function
>>>>> will reuse PTE one by one while the specific subpage is written.
>>>>
>>>> Hmm - I thought it would only reuse if the total mapcount for the folio was 1.
>>>> And since it is a large folio with each page mapped once in proc B, I thought
>>>> every subpage write would cause a copy except the last one? I haven't looked at
>>>> the code for a while. But I had it in my head that this is an area we need to
>>>> improve for mTHP.
>>>
>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
>>> a single PTE remains mapped (refcount == 0).
>>
>> ^ == 1
> 
> Ahh yes. That's what I meant. I got the behacviour vagulely right though.
> 
> Anyway, regardless, I'm not sure we want to batch here. Or if we do, we want to
> batch function that will only clear access and dirty.

We likely want to detect a folio batch the "usual" way (as relaxed as 
possible), then do all the checks (#pte == folio_mapcount() under folio 
lock), and finally batch-update the access and dirty only.

Barry Song March 7, 2024, 11:26 a.m. UTC | #11

On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
>
> On 07/03/2024 10:54, David Hildenbrand wrote:
> > On 07.03.24 11:54, David Hildenbrand wrote:
> >> On 07.03.24 11:50, Ryan Roberts wrote:
> >>> On 07/03/2024 09:33, Barry Song wrote:
> >>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
> >>>>>
> >>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>
> >>>>>>> Hey Barry,
> >>>>>>>
> >>>>>>> Thanks for taking time to review!
> >>>>>>>
> >>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
> >>>>>>>>
> >>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>>>
> >>>>>>> [...]
> >>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> >>>>>>>>> +                                                struct folio *folio,
> >>>>>>>>> pte_t *start_pte)
> >>>>>>>>> +{
> >>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
> >>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> >>>>>>>>> +
> >>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
> >>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
> >>>>>>>>> +                       return false;
> >>>>>>>>
> >>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
> >>>>>>>> we don't do
> >>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
> >>>>>>>
> >>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
> >>>>>>> associated
> >>>>>>> with this folio and the cow folio has smaller size than this folio,
> >>>>>>> should we still
> >>>>>>> mark this folio as lazyfree?
> >>>>>>
> >>>>>> I agree, this is true. However, we've somehow accepted the fact that
> >>>>>> folio_likely_mapped_shared
> >>>>>> can result in false negatives or false positives to balance the
> >>>>>> overhead.  So I really don't know :-)
> >>>>>>
> >>>>>> Maybe David and Vishal can give some comments here.
> >>>>>>
> >>>>>>>
> >>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
> >>>>>>>> [1]
> >>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
> >>>>>>>
> >>>>>>> Yes, we should rebase our work against David’s changes.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> +
> >>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
> >>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
> >>>>>>>>> flags, NULL);
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>>    static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>>>>>>                                   unsigned long end, struct mm_walk *walk)
> >>>>>>>>>
> >>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
> >>>>>>>>> unsigned long addr,
> >>>>>>>>>                    */
> >>>>>>>>>                   if (folio_test_large(folio)) {
> >>>>>>>>>                           int err;
> >>>>>>>>> +                       unsigned long next_addr, align;
> >>>>>>>>>
> >>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
> >>>>>>>>> -                               break;
> >>>>>>>>> -                       if (!folio_trylock(folio))
> >>>>>>>>> -                               break;
> >>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
> >>>>>>>>> +                           !folio_trylock(folio))
> >>>>>>>>> +                               goto skip_large_folio;
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
> >>>>>>>> might be
> >>>>>>>> pointing to other folios.
> >>>>>>>>
> >>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
> >>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
> >>>>>>>> and PTE16 will point to two different small folios. We can only skip
> >>>>>>>> when we
> >>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
> >>>>>>>
> >>>>>>> Agreed. Thanks for pointing that out.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> +
> >>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
> >>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
> >>>>>>>>> +
> >>>>>>>>> +                       /*
> >>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
> >>>>>>>>> +                        * cannot mark the entire large folio as lazyfree,
> >>>>>>>>> +                        * then just split it.
> >>>>>>>>> +                        */
> >>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
> >>>>>>>>> align ||
> >>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
> >>>>>>>>> pte))
> >>>>>>>>> +                               goto split_large_folio;
> >>>>>>>>> +
> >>>>>>>>> +                       /*
> >>>>>>>>> +                        * Avoid unnecessary folio splitting if the large
> >>>>>>>>> +                        * folio is entirely within the given range.
> >>>>>>>>> +                        */
> >>>>>>>>> +                       folio_clear_dirty(folio);
> >>>>>>>>> +                       folio_unlock(folio);
> >>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
> >>>>>>>>> PAGE_SIZE) {
> >>>>>>>>> +                               ptent = ptep_get(pte);
> >>>>>>>>> +                               if (pte_young(ptent) ||
> >>>>>>>>> pte_dirty(ptent)) {
> >>>>>>>>> +                                       ptent = ptep_get_and_clear_full(
> >>>>>>>>> +                                               mm, addr, pte,
> >>>>>>>>> tlb->fullmm);
> >>>>>>>>> +                                       ptent = pte_mkold(ptent);
> >>>>>>>>> +                                       ptent = pte_mkclean(ptent);
> >>>>>>>>> +                                       set_pte_at(mm, addr, pte, ptent);
> >>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
> >>>>>>>>> addr);
> >>>>>>>>> +                               }
> >>>>>>>>
> >>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
> >>>>>>>> unfolding
> >>>>>>>> and folding again. It seems quite expensive.
> >>>>>
> >>>>> I'm not convinced we should be doing this in batches. We want the initial
> >>>>> folio_pte_batch() to be as loose as possible regarding permissions so that we
> >>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
> >>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and other
> >>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
> >>>>> But
> >>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
> >>>>> all
> >>>>
> >>>> You are correct. I believe this scenario could indeed occur. For instance,
> >>>> if process A forks process B and then unmaps itself, leaving B as the
> >>>> sole process owning the large folio.  The current wp_page_reuse() function
> >>>> will reuse PTE one by one while the specific subpage is written.
> >>>
> >>> Hmm - I thought it would only reuse if the total mapcount for the folio was 1.
> >>> And since it is a large folio with each page mapped once in proc B, I thought
> >>> every subpage write would cause a copy except the last one? I haven't looked at
> >>> the code for a while. But I had it in my head that this is an area we need to
> >>> improve for mTHP.

So sad I am wrong again 

David Hildenbrand March 7, 2024, 11:31 a.m. UTC | #12

On 07.03.24 12:26, Barry Song wrote:
> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
>>
>> On 07/03/2024 10:54, David Hildenbrand wrote:
>>> On 07.03.24 11:54, David Hildenbrand wrote:
>>>> On 07.03.24 11:50, Ryan Roberts wrote:
>>>>> On 07/03/2024 09:33, Barry Song wrote:
>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>
>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> Hey Barry,
>>>>>>>>>
>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>
>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>>>>
>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>>>>> +                                                struct folio *folio,
>>>>>>>>>>> pte_t *start_pte)
>>>>>>>>>>> +{
>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>>>> +
>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>>>> +                       return false;
>>>>>>>>>>
>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>>>>> we don't do
>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>>>>
>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>>>> associated
>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>>>>> should we still
>>>>>>>>> mark this folio as lazyfree?
>>>>>>>>
>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>>>>> folio_likely_mapped_shared
>>>>>>>> can result in false negatives or false positives to balance the
>>>>>>>> overhead.  So I really don't know :-)
>>>>>>>>
>>>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>>>> [1]
>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>>>
>>>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
>>>>>>>>>>> flags, NULL);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>>     static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>>>>                                    unsigned long end, struct mm_walk *walk)
>>>>>>>>>>>
>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
>>>>>>>>>>> unsigned long addr,
>>>>>>>>>>>                     */
>>>>>>>>>>>                    if (folio_test_large(folio)) {
>>>>>>>>>>>                            int err;
>>>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>>>
>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>>>> -                               break;
>>>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>>>> -                               break;
>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
>>>>>>>>>> might be
>>>>>>>>>> pointing to other folios.
>>>>>>>>>>
>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
>>>>>>>>>> when we
>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>>>
>>>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>>>>> +
>>>>>>>>>>> +                       /*
>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>>>>> +                        * cannot mark the entire large folio as lazyfree,
>>>>>>>>>>> +                        * then just split it.
>>>>>>>>>>> +                        */
>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
>>>>>>>>>>> align ||
>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>>>> pte))
>>>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>>>> +
>>>>>>>>>>> +                       /*
>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the large
>>>>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>>>>> +                        */
>>>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>>>> PAGE_SIZE) {
>>>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>>>> +                                       ptent = ptep_get_and_clear_full(
>>>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>>>> tlb->fullmm);
>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte, ptent);
>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>>>> addr);
>>>>>>>>>>> +                               }
>>>>>>>>>>
>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
>>>>>>>>>> unfolding
>>>>>>>>>> and folding again. It seems quite expensive.
>>>>>>>
>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so that we
>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and other
>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
>>>>>>> But
>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
>>>>>>> all
>>>>>>
>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
>>>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
>>>>>> will reuse PTE one by one while the specific subpage is written.
>>>>>
>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio was 1.
>>>>> And since it is a large folio with each page mapped once in proc B, I thought
>>>>> every subpage write would cause a copy except the last one? I haven't looked at
>>>>> the code for a while. But I had it in my head that this is an area we need to
>>>>> improve for mTHP.
> 
> So sad I am wrong again 

Ryan Roberts March 7, 2024, 11:42 a.m. UTC | #13

On 07/03/2024 11:31, David Hildenbrand wrote:
> On 07.03.24 12:26, Barry Song wrote:
>> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
>>>
>>> On 07/03/2024 10:54, David Hildenbrand wrote:
>>>> On 07.03.24 11:54, David Hildenbrand wrote:
>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
>>>>>> On 07/03/2024 09:33, Barry Song wrote:
>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>
>>>>>>>>>> Hey Barry,
>>>>>>>>>>
>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>
>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>
>>>>>>>>>> [...]
>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>>>>>> +                                                struct folio *folio,
>>>>>>>>>>>> pte_t *start_pte)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>>>>> +
>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>>>>> +                       return false;
>>>>>>>>>>>
>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>>>>>> we don't do
>>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>>>>>
>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>>>>> associated
>>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>>>>>> should we still
>>>>>>>>>> mark this folio as lazyfree?
>>>>>>>>>
>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>>>>>> folio_likely_mapped_shared
>>>>>>>>> can result in false negatives or false positives to balance the
>>>>>>>>> overhead.  So I really don't know :-)
>>>>>>>>>
>>>>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>>>>> [1]
>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>>>>
>>>>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> +
>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
>>>>>>>>>>>> flags, NULL);
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>>     static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>>>>>                                    unsigned long end, struct mm_walk
>>>>>>>>>>>> *walk)
>>>>>>>>>>>>
>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
>>>>>>>>>>>> unsigned long addr,
>>>>>>>>>>>>                     */
>>>>>>>>>>>>                    if (folio_test_large(folio)) {
>>>>>>>>>>>>                            int err;
>>>>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>>>>
>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>>>>> -                               break;
>>>>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>>>>> -                               break;
>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
>>>>>>>>>>> might be
>>>>>>>>>>> pointing to other folios.
>>>>>>>>>>>
>>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
>>>>>>>>>>> when we
>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>>>>
>>>>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> +
>>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>>>>>> +
>>>>>>>>>>>> +                       /*
>>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>>>>>> +                        * cannot mark the entire large folio as
>>>>>>>>>>>> lazyfree,
>>>>>>>>>>>> +                        * then just split it.
>>>>>>>>>>>> +                        */
>>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
>>>>>>>>>>>> align ||
>>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>>>>> pte))
>>>>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>>>>> +
>>>>>>>>>>>> +                       /*
>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the
>>>>>>>>>>>> large
>>>>>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>>>>>> +                        */
>>>>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>>>>> PAGE_SIZE) {
>>>>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>>>>> +                                       ptent =
>>>>>>>>>>>> ptep_get_and_clear_full(
>>>>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>>>>> tlb->fullmm);
>>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte,
>>>>>>>>>>>> ptent);
>>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>>>>> addr);
>>>>>>>>>>>> +                               }
>>>>>>>>>>>
>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
>>>>>>>>>>> unfolding
>>>>>>>>>>> and folding again. It seems quite expensive.
>>>>>>>>
>>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so
>>>>>>>> that we
>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits
>>>>>>>> like
>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and
>>>>>>>> other
>>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
>>>>>>>> But
>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
>>>>>>>> all
>>>>>>>
>>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
>>>>>>> will reuse PTE one by one while the specific subpage is written.
>>>>>>
>>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio
>>>>>> was 1.
>>>>>> And since it is a large folio with each page mapped once in proc B, I thought
>>>>>> every subpage write would cause a copy except the last one? I haven't
>>>>>> looked at
>>>>>> the code for a while. But I had it in my head that this is an area we need to
>>>>>> improve for mTHP.
>>
>> So sad I am wrong again 

David Hildenbrand March 7, 2024, 11:45 a.m. UTC | #14

On 07.03.24 12:42, Ryan Roberts wrote:
> On 07/03/2024 11:31, David Hildenbrand wrote:
>> On 07.03.24 12:26, Barry Song wrote:
>>> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
>>>>
>>>> On 07/03/2024 10:54, David Hildenbrand wrote:
>>>>> On 07.03.24 11:54, David Hildenbrand wrote:
>>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
>>>>>>> On 07/03/2024 09:33, Barry Song wrote:
>>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>
>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>> [...]
>>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>>>>>>> +                                                struct folio *folio,
>>>>>>>>>>>>> pte_t *start_pte)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>>>>>> +                       return false;
>>>>>>>>>>>>
>>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>>>>>>> we don't do
>>>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>>>>>>
>>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>>>>>> associated
>>>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>>>>>>> should we still
>>>>>>>>>>> mark this folio as lazyfree?
>>>>>>>>>>
>>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>>>>>>> folio_likely_mapped_shared
>>>>>>>>>> can result in false negatives or false positives to balance the
>>>>>>>>>> overhead.  So I really don't know :-)
>>>>>>>>>>
>>>>>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>>>>>> [1]
>>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>>>>>
>>>>>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
>>>>>>>>>>>>> flags, NULL);
>>>>>>>>>>>>> +}
>>>>>>>>>>>>> +
>>>>>>>>>>>>>      static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>>>>>>                                     unsigned long end, struct mm_walk
>>>>>>>>>>>>> *walk)
>>>>>>>>>>>>>
>>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
>>>>>>>>>>>>> unsigned long addr,
>>>>>>>>>>>>>                      */
>>>>>>>>>>>>>                     if (folio_test_large(folio)) {
>>>>>>>>>>>>>                             int err;
>>>>>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>>>>>
>>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
>>>>>>>>>>>> might be
>>>>>>>>>>>> pointing to other folios.
>>>>>>>>>>>>
>>>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
>>>>>>>>>>>> when we
>>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>>>>>
>>>>>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>>>>>>> +                        * cannot mark the entire large folio as
>>>>>>>>>>>>> lazyfree,
>>>>>>>>>>>>> +                        * then just split it.
>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
>>>>>>>>>>>>> align ||
>>>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>>>>>> pte))
>>>>>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the
>>>>>>>>>>>>> large
>>>>>>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>>>>>> PAGE_SIZE) {
>>>>>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>>>>>> +                                       ptent =
>>>>>>>>>>>>> ptep_get_and_clear_full(
>>>>>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>>>>>> tlb->fullmm);
>>>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte,
>>>>>>>>>>>>> ptent);
>>>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>>>>>> addr);
>>>>>>>>>>>>> +                               }
>>>>>>>>>>>>
>>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
>>>>>>>>>>>> unfolding
>>>>>>>>>>>> and folding again. It seems quite expensive.
>>>>>>>>>
>>>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so
>>>>>>>>> that we
>>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits
>>>>>>>>> like
>>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and
>>>>>>>>> other
>>>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
>>>>>>>>> But
>>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
>>>>>>>>> all
>>>>>>>>
>>>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
>>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
>>>>>>>> will reuse PTE one by one while the specific subpage is written.
>>>>>>>
>>>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio
>>>>>>> was 1.
>>>>>>> And since it is a large folio with each page mapped once in proc B, I thought
>>>>>>> every subpage write would cause a copy except the last one? I haven't
>>>>>>> looked at
>>>>>>> the code for a while. But I had it in my head that this is an area we need to
>>>>>>> improve for mTHP.
>>>
>>> So sad I am wrong again 

Barry Song March 7, 2024, 12:01 p.m. UTC | #15

On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[e-mail]> wrote:
>
> On 07.03.24 12:42, Ryan Roberts wrote:
> > On 07/03/2024 11:31, David Hildenbrand wrote:
> >> On 07.03.24 12:26, Barry Song wrote:
> >>> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
> >>>>
> >>>> On 07/03/2024 10:54, David Hildenbrand wrote:
> >>>>> On 07.03.24 11:54, David Hildenbrand wrote:
> >>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
> >>>>>>> On 07/03/2024 09:33, Barry Song wrote:
> >>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
> >>>>>>>>>
> >>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> Hey Barry,
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks for taking time to review!
> >>>>>>>>>>>
> >>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>> [...]
> >>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> >>>>>>>>>>>>> +                                                struct folio *folio,
> >>>>>>>>>>>>> pte_t *start_pte)
> >>>>>>>>>>>>> +{
> >>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
> >>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
> >>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
> >>>>>>>>>>>>> +                       return false;
> >>>>>>>>>>>>
> >>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
> >>>>>>>>>>>> we don't do
> >>>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
> >>>>>>>>>>>
> >>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
> >>>>>>>>>>> associated
> >>>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
> >>>>>>>>>>> should we still
> >>>>>>>>>>> mark this folio as lazyfree?
> >>>>>>>>>>
> >>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
> >>>>>>>>>> folio_likely_mapped_shared
> >>>>>>>>>> can result in false negatives or false positives to balance the
> >>>>>>>>>> overhead.  So I really don't know :-)
> >>>>>>>>>>
> >>>>>>>>>> Maybe David and Vishal can give some comments here.
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
> >>>>>>>>>>>> [1]
> >>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
> >>>>>>>>>>>
> >>>>>>>>>>> Yes, we should rebase our work against David’s changes.
> >>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
> >>>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
> >>>>>>>>>>>>> flags, NULL);
> >>>>>>>>>>>>> +}
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>>      static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>>>>>>>>>>                                     unsigned long end, struct mm_walk
> >>>>>>>>>>>>> *walk)
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
> >>>>>>>>>>>>> unsigned long addr,
> >>>>>>>>>>>>>                      */
> >>>>>>>>>>>>>                     if (folio_test_large(folio)) {
> >>>>>>>>>>>>>                             int err;
> >>>>>>>>>>>>> +                       unsigned long next_addr, align;
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
> >>>>>>>>>>>>> -                               break;
> >>>>>>>>>>>>> -                       if (!folio_trylock(folio))
> >>>>>>>>>>>>> -                               break;
> >>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
> >>>>>>>>>>>>> +                           !folio_trylock(folio))
> >>>>>>>>>>>>> +                               goto skip_large_folio;
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
> >>>>>>>>>>>> might be
> >>>>>>>>>>>> pointing to other folios.
> >>>>>>>>>>>>
> >>>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
> >>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
> >>>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
> >>>>>>>>>>>> when we
> >>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
> >>>>>>>>>>>
> >>>>>>>>>>> Agreed. Thanks for pointing that out.
> >>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
> >>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +                       /*
> >>>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
> >>>>>>>>>>>>> +                        * cannot mark the entire large folio as
> >>>>>>>>>>>>> lazyfree,
> >>>>>>>>>>>>> +                        * then just split it.
> >>>>>>>>>>>>> +                        */
> >>>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
> >>>>>>>>>>>>> align ||
> >>>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
> >>>>>>>>>>>>> pte))
> >>>>>>>>>>>>> +                               goto split_large_folio;
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +                       /*
> >>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the
> >>>>>>>>>>>>> large
> >>>>>>>>>>>>> +                        * folio is entirely within the given range.
> >>>>>>>>>>>>> +                        */
> >>>>>>>>>>>>> +                       folio_clear_dirty(folio);
> >>>>>>>>>>>>> +                       folio_unlock(folio);
> >>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
> >>>>>>>>>>>>> PAGE_SIZE) {
> >>>>>>>>>>>>> +                               ptent = ptep_get(pte);
> >>>>>>>>>>>>> +                               if (pte_young(ptent) ||
> >>>>>>>>>>>>> pte_dirty(ptent)) {
> >>>>>>>>>>>>> +                                       ptent =
> >>>>>>>>>>>>> ptep_get_and_clear_full(
> >>>>>>>>>>>>> +                                               mm, addr, pte,
> >>>>>>>>>>>>> tlb->fullmm);
> >>>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
> >>>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
> >>>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte,
> >>>>>>>>>>>>> ptent);
> >>>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
> >>>>>>>>>>>>> addr);
> >>>>>>>>>>>>> +                               }
> >>>>>>>>>>>>
> >>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
> >>>>>>>>>>>> unfolding
> >>>>>>>>>>>> and folding again. It seems quite expensive.
> >>>>>>>>>
> >>>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
> >>>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so
> >>>>>>>>> that we
> >>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits
> >>>>>>>>> like
> >>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and
> >>>>>>>>> other
> >>>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
> >>>>>>>>> But
> >>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
> >>>>>>>>> all
> >>>>>>>>
> >>>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
> >>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
> >>>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
> >>>>>>>> will reuse PTE one by one while the specific subpage is written.
> >>>>>>>
> >>>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio
> >>>>>>> was 1.
> >>>>>>> And since it is a large folio with each page mapped once in proc B, I thought
> >>>>>>> every subpage write would cause a copy except the last one? I haven't
> >>>>>>> looked at
> >>>>>>> the code for a while. But I had it in my head that this is an area we need to
> >>>>>>> improve for mTHP.
> >>>
> >>> So sad I am wrong again 

David Hildenbrand March 7, 2024, 12:04 p.m. UTC | #16

On 07.03.24 13:01, Barry Song wrote:
> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[e-mail]> wrote:
>>
>> On 07.03.24 12:42, Ryan Roberts wrote:
>>> On 07/03/2024 11:31, David Hildenbrand wrote:
>>>> On 07.03.24 12:26, Barry Song wrote:
>>>>> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>
>>>>>> On 07/03/2024 10:54, David Hildenbrand wrote:
>>>>>>> On 07.03.24 11:54, David Hildenbrand wrote:
>>>>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
>>>>>>>>> On 07/03/2024 09:33, Barry Song wrote:
>>>>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>>>>>>>>> +                                                struct folio *folio,
>>>>>>>>>>>>>>> pte_t *start_pte)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>>>>>>>> +                       return false;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>>>>>>>>> we don't do
>>>>>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>>>>>>>>
>>>>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>>>>>>>> associated
>>>>>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>>>>>>>>> should we still
>>>>>>>>>>>>> mark this folio as lazyfree?
>>>>>>>>>>>>
>>>>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>>>>>>>>> folio_likely_mapped_shared
>>>>>>>>>>>> can result in false negatives or false positives to balance the
>>>>>>>>>>>> overhead.  So I really don't know :-)
>>>>>>>>>>>>
>>>>>>>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
>>>>>>>>>>>>>>> flags, NULL);
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>       static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>>>>>>>>                                      unsigned long end, struct mm_walk
>>>>>>>>>>>>>>> *walk)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
>>>>>>>>>>>>>>> unsigned long addr,
>>>>>>>>>>>>>>>                       */
>>>>>>>>>>>>>>>                      if (folio_test_large(folio)) {
>>>>>>>>>>>>>>>                              int err;
>>>>>>>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
>>>>>>>>>>>>>> might be
>>>>>>>>>>>>>> pointing to other folios.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
>>>>>>>>>>>>>> when we
>>>>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>>>>>>>>> +                        * cannot mark the entire large folio as
>>>>>>>>>>>>>>> lazyfree,
>>>>>>>>>>>>>>> +                        * then just split it.
>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
>>>>>>>>>>>>>>> align ||
>>>>>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>>>>>>>> pte))
>>>>>>>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the
>>>>>>>>>>>>>>> large
>>>>>>>>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>>>>>>>> PAGE_SIZE) {
>>>>>>>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>>>>>>>> +                                       ptent =
>>>>>>>>>>>>>>> ptep_get_and_clear_full(
>>>>>>>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>>>>>>>> tlb->fullmm);
>>>>>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte,
>>>>>>>>>>>>>>> ptent);
>>>>>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>>>>>>>> addr);
>>>>>>>>>>>>>>> +                               }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
>>>>>>>>>>>>>> unfolding
>>>>>>>>>>>>>> and folding again. It seems quite expensive.
>>>>>>>>>>>
>>>>>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>>>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so
>>>>>>>>>>> that we
>>>>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits
>>>>>>>>>>> like
>>>>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and
>>>>>>>>>>> other
>>>>>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
>>>>>>>>>>> But
>>>>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
>>>>>>>>>>> all
>>>>>>>>>>
>>>>>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
>>>>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>>>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
>>>>>>>>>> will reuse PTE one by one while the specific subpage is written.
>>>>>>>>>
>>>>>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio
>>>>>>>>> was 1.
>>>>>>>>> And since it is a large folio with each page mapped once in proc B, I thought
>>>>>>>>> every subpage write would cause a copy except the last one? I haven't
>>>>>>>>> looked at
>>>>>>>>> the code for a while. But I had it in my head that this is an area we need to
>>>>>>>>> improve for mTHP.
>>>>>
>>>>> So sad I am wrong again 

Lance Yang March 7, 2024, 2:41 p.m. UTC | #17

Hey Barry, Ryan, David,

Thanks a lot for taking the time to explain and provide suggestions!
I really appreciate your time!

IIUC, here's what we need to do for v3:

If folio_likely_mapped_shared() is true, or if we cannot acquire
the folio lock, we simply skip the batched PTEs. Then, we compare
the number of batched PTEs against folio_mapcount(). Finally,
batch-update the access and dirty only.

I'm not sure if I've understood correctly, could you please confirm?

Thanks,
Lance

On Thu, Mar 7, 2024 at 7:17 PM David Hildenbrand <[e-mail]> wrote:
>
> On 07.03.24 12:13, Ryan Roberts wrote:
> > On 07/03/2024 10:54, David Hildenbrand wrote:
> >> On 07.03.24 11:54, David Hildenbrand wrote:
> >>> On 07.03.24 11:50, Ryan Roberts wrote:
> >>>> On 07/03/2024 09:33, Barry Song wrote:
> >>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
> >>>>>>
> >>>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>>
> >>>>>>>> Hey Barry,
> >>>>>>>>
> >>>>>>>> Thanks for taking time to review!
> >>>>>>>>
> >>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
> >>>>>>>>>
> >>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>>>>
> >>>>>>>> [...]
> >>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> >>>>>>>>>> +                                                struct folio *folio,
> >>>>>>>>>> pte_t *start_pte)
> >>>>>>>>>> +{
> >>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
> >>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> >>>>>>>>>> +
> >>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
> >>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
> >>>>>>>>>> +                       return false;
> >>>>>>>>>
> >>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
> >>>>>>>>> we don't do
> >>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
> >>>>>>>>
> >>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
> >>>>>>>> associated
> >>>>>>>> with this folio and the cow folio has smaller size than this folio,
> >>>>>>>> should we still
> >>>>>>>> mark this folio as lazyfree?
> >>>>>>>
> >>>>>>> I agree, this is true. However, we've somehow accepted the fact that
> >>>>>>> folio_likely_mapped_shared
> >>>>>>> can result in false negatives or false positives to balance the
> >>>>>>> overhead.  So I really don't know :-)
> >>>>>>>
> >>>>>>> Maybe David and Vishal can give some comments here.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
> >>>>>>>>> [1]
> >>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
> >>>>>>>>
> >>>>>>>> Yes, we should rebase our work against David’s changes.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
> >>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
> >>>>>>>>>> flags, NULL);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>>     static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>>>>>>>                                    unsigned long end, struct mm_walk *walk)
> >>>>>>>>>>
> >>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
> >>>>>>>>>> unsigned long addr,
> >>>>>>>>>>                     */
> >>>>>>>>>>                    if (folio_test_large(folio)) {
> >>>>>>>>>>                            int err;
> >>>>>>>>>> +                       unsigned long next_addr, align;
> >>>>>>>>>>
> >>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
> >>>>>>>>>> -                               break;
> >>>>>>>>>> -                       if (!folio_trylock(folio))
> >>>>>>>>>> -                               break;
> >>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
> >>>>>>>>>> +                           !folio_trylock(folio))
> >>>>>>>>>> +                               goto skip_large_folio;
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
> >>>>>>>>> might be
> >>>>>>>>> pointing to other folios.
> >>>>>>>>>
> >>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
> >>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
> >>>>>>>>> and PTE16 will point to two different small folios. We can only skip
> >>>>>>>>> when we
> >>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
> >>>>>>>>
> >>>>>>>> Agreed. Thanks for pointing that out.
> >>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
> >>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
> >>>>>>>>>> +
> >>>>>>>>>> +                       /*
> >>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
> >>>>>>>>>> +                        * cannot mark the entire large folio as lazyfree,
> >>>>>>>>>> +                        * then just split it.
> >>>>>>>>>> +                        */
> >>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
> >>>>>>>>>> align ||
> >>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
> >>>>>>>>>> pte))
> >>>>>>>>>> +                               goto split_large_folio;
> >>>>>>>>>> +
> >>>>>>>>>> +                       /*
> >>>>>>>>>> +                        * Avoid unnecessary folio splitting if the large
> >>>>>>>>>> +                        * folio is entirely within the given range.
> >>>>>>>>>> +                        */
> >>>>>>>>>> +                       folio_clear_dirty(folio);
> >>>>>>>>>> +                       folio_unlock(folio);
> >>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
> >>>>>>>>>> PAGE_SIZE) {
> >>>>>>>>>> +                               ptent = ptep_get(pte);
> >>>>>>>>>> +                               if (pte_young(ptent) ||
> >>>>>>>>>> pte_dirty(ptent)) {
> >>>>>>>>>> +                                       ptent = ptep_get_and_clear_full(
> >>>>>>>>>> +                                               mm, addr, pte,
> >>>>>>>>>> tlb->fullmm);
> >>>>>>>>>> +                                       ptent = pte_mkold(ptent);
> >>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
> >>>>>>>>>> +                                       set_pte_at(mm, addr, pte, ptent);
> >>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
> >>>>>>>>>> addr);
> >>>>>>>>>> +                               }
> >>>>>>>>>
> >>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
> >>>>>>>>> unfolding
> >>>>>>>>> and folding again. It seems quite expensive.
> >>>>>>
> >>>>>> I'm not convinced we should be doing this in batches. We want the initial
> >>>>>> folio_pte_batch() to be as loose as possible regarding permissions so that we
> >>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
> >>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and other
> >>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
> >>>>>> But
> >>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
> >>>>>> all
> >>>>>
> >>>>> You are correct. I believe this scenario could indeed occur. For instance,
> >>>>> if process A forks process B and then unmaps itself, leaving B as the
> >>>>> sole process owning the large folio.  The current wp_page_reuse() function
> >>>>> will reuse PTE one by one while the specific subpage is written.
> >>>>
> >>>> Hmm - I thought it would only reuse if the total mapcount for the folio was 1.
> >>>> And since it is a large folio with each page mapped once in proc B, I thought
> >>>> every subpage write would cause a copy except the last one? I haven't looked at
> >>>> the code for a while. But I had it in my head that this is an area we need to
> >>>> improve for mTHP.
> >>>
> >>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
> >>> a single PTE remains mapped (refcount == 0).
> >>
> >> ^ == 1
> >
> > Ahh yes. That's what I meant. I got the behacviour vagulely right though.
> >
> > Anyway, regardless, I'm not sure we want to batch here. Or if we do, we want to
> > batch function that will only clear access and dirty.
>
> We likely want to detect a folio batch the "usual" way (as relaxed as
> possible), then do all the checks (#pte == folio_mapcount() under folio
> lock), and finally batch-update the access and dirty only.
>
> --
> Cheers,
>
> David / dhildenb
>

David Hildenbrand March 7, 2024, 2:58 p.m. UTC | #18

On 07.03.24 15:41, Lance Yang wrote:
> Hey Barry, Ryan, David,
> 
> Thanks a lot for taking the time to explain and provide suggestions!
> I really appreciate your time!
> 
> IIUC, here's what we need to do for v3:
> 
> If folio_likely_mapped_shared() is true, or if we cannot acquire
> the folio lock, we simply skip the batched PTEs. Then, we compare
> the number of batched PTEs against folio_mapcount(). Finally,
> batch-update the access and dirty only.
> 
> I'm not sure if I've understood correctly, could you please confirm?
> 
> Thanks,
> Lance
> 
> On Thu, Mar 7, 2024 at 7:17 PM David Hildenbrand <[e-mail]> wrote:
>>
>> On 07.03.24 12:13, Ryan Roberts wrote:
>>> On 07/03/2024 10:54, David Hildenbrand wrote:
>>>> On 07.03.24 11:54, David Hildenbrand wrote:
>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
>>>>>> On 07/03/2024 09:33, Barry Song wrote:
>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>
>>>>>>>>>> Hey Barry,
>>>>>>>>>>
>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>
>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>
>>>>>>>>>> [...]
>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>>>>>> +                                                struct folio *folio,
>>>>>>>>>>>> pte_t *start_pte)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>>>>> +
>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>>>>> +                       return false;
>>>>>>>>>>>
>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>>>>>> we don't do
>>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>>>>>
>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>>>>> associated
>>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>>>>>> should we still
>>>>>>>>>> mark this folio as lazyfree?
>>>>>>>>>
>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>>>>>> folio_likely_mapped_shared
>>>>>>>>> can result in false negatives or false positives to balance the
>>>>>>>>> overhead.  So I really don't know :-)
>>>>>>>>>
>>>>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>>>>> [1]
>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>>>>
>>>>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> +
>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
>>>>>>>>>>>> flags, NULL);
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>>      static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>>>>>                                     unsigned long end, struct mm_walk *walk)
>>>>>>>>>>>>
>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
>>>>>>>>>>>> unsigned long addr,
>>>>>>>>>>>>                      */
>>>>>>>>>>>>                     if (folio_test_large(folio)) {
>>>>>>>>>>>>                             int err;
>>>>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>>>>
>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>>>>> -                               break;
>>>>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>>>>> -                               break;
>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
>>>>>>>>>>> might be
>>>>>>>>>>> pointing to other folios.
>>>>>>>>>>>
>>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
>>>>>>>>>>> when we
>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>>>>
>>>>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> +
>>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>>>>>> +
>>>>>>>>>>>> +                       /*
>>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>>>>>> +                        * cannot mark the entire large folio as lazyfree,
>>>>>>>>>>>> +                        * then just split it.
>>>>>>>>>>>> +                        */
>>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
>>>>>>>>>>>> align ||
>>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>>>>> pte))
>>>>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>>>>> +
>>>>>>>>>>>> +                       /*
>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the large
>>>>>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>>>>>> +                        */
>>>>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>>>>> PAGE_SIZE) {
>>>>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>>>>> +                                       ptent = ptep_get_and_clear_full(
>>>>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>>>>> tlb->fullmm);
>>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte, ptent);
>>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>>>>> addr);
>>>>>>>>>>>> +                               }
>>>>>>>>>>>
>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
>>>>>>>>>>> unfolding
>>>>>>>>>>> and folding again. It seems quite expensive.
>>>>>>>>
>>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so that we
>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and other
>>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
>>>>>>>> But
>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
>>>>>>>> all
>>>>>>>
>>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
>>>>>>> will reuse PTE one by one while the specific subpage is written.
>>>>>>
>>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio was 1.
>>>>>> And since it is a large folio with each page mapped once in proc B, I thought
>>>>>> every subpage write would cause a copy except the last one? I haven't looked at
>>>>>> the code for a while. But I had it in my head that this is an area we need to
>>>>>> improve for mTHP.
>>>>>
>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
>>>>> a single PTE remains mapped (refcount == 0).
>>>>
>>>> ^ == 1
>>>
>>> Ahh yes. That's what I meant. I got the behacviour vagulely right though.
>>>
>>> Anyway, regardless, I'm not sure we want to batch here. Or if we do, we want to
>>> batch function that will only clear access and dirty.
>>
>> We likely want to detect a folio batch the "usual" way (as relaxed as
>> possible), then do all the checks (#pte == folio_mapcount() under folio
>> lock), and finally batch-update the access and dirty only.

Something like:

1) We might want to factor out the existing single-pte case and extend 
it to handle multiple PTEs (nr_pages). For the existing code, we would 
pass in "nr_pages".

For example, instead of "folio_mapcount(folio) != 1" you'd check 
"folio_mapcount(folio) != nr_pages" in there. And we'd need functions to 
abstract working on multiple ptes.

2) We'd add something like wrprotect_ptes(), that does the mkold+clean 
on multiple PTEs.

Naming suggestion for such a function requested :)

3) Then, we might want to extend folio_pte_batch() by an *any_young and 
*any_dirty parameter that will get optimized out for other users. So you 
get that information right when scanning.


Just a rough idea, devil is in the detail. But likely trying to abstrct 
the code to handle "multiple pages of the same folio" should come just 
naturally like we used to do for fork() and munmap() so far.

Lance Yang March 7, 2024, 3:08 p.m. UTC | #19

Thanks a lot, David!

Got it. I'll do my best.

Thanks,
Lance

On Thu, Mar 7, 2024 at 10:58 PM David Hildenbrand <[e-mail]> wrote:
>
> On 07.03.24 15:41, Lance Yang wrote:
> > Hey Barry, Ryan, David,
> >
> > Thanks a lot for taking the time to explain and provide suggestions!
> > I really appreciate your time!
> >
> > IIUC, here's what we need to do for v3:
> >
> > If folio_likely_mapped_shared() is true, or if we cannot acquire
> > the folio lock, we simply skip the batched PTEs. Then, we compare
> > the number of batched PTEs against folio_mapcount(). Finally,
> > batch-update the access and dirty only.
> >
> > I'm not sure if I've understood correctly, could you please confirm?
> >
> > Thanks,
> > Lance
> >
> > On Thu, Mar 7, 2024 at 7:17 PM David Hildenbrand <[e-mail]> wrote:
> >>
> >> On 07.03.24 12:13, Ryan Roberts wrote:
> >>> On 07/03/2024 10:54, David Hildenbrand wrote:
> >>>> On 07.03.24 11:54, David Hildenbrand wrote:
> >>>>> On 07.03.24 11:50, Ryan Roberts wrote:
> >>>>>> On 07/03/2024 09:33, Barry Song wrote:
> >>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
> >>>>>>>>
> >>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Hey Barry,
> >>>>>>>>>>
> >>>>>>>>>> Thanks for taking time to review!
> >>>>>>>>>>
> >>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>> [...]
> >>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> >>>>>>>>>>>> +                                                struct folio *folio,
> >>>>>>>>>>>> pte_t *start_pte)
> >>>>>>>>>>>> +{
> >>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
> >>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
> >>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
> >>>>>>>>>>>> +                       return false;
> >>>>>>>>>>>
> >>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
> >>>>>>>>>>> we don't do
> >>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
> >>>>>>>>>>
> >>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
> >>>>>>>>>> associated
> >>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
> >>>>>>>>>> should we still
> >>>>>>>>>> mark this folio as lazyfree?
> >>>>>>>>>
> >>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
> >>>>>>>>> folio_likely_mapped_shared
> >>>>>>>>> can result in false negatives or false positives to balance the
> >>>>>>>>> overhead.  So I really don't know :-)
> >>>>>>>>>
> >>>>>>>>> Maybe David and Vishal can give some comments here.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
> >>>>>>>>>>> [1]
> >>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
> >>>>>>>>>>
> >>>>>>>>>> Yes, we should rebase our work against David’s changes.
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
> >>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
> >>>>>>>>>>>> flags, NULL);
> >>>>>>>>>>>> +}
> >>>>>>>>>>>> +
> >>>>>>>>>>>>      static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>>>>>>>>>                                     unsigned long end, struct mm_walk *walk)
> >>>>>>>>>>>>
> >>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
> >>>>>>>>>>>> unsigned long addr,
> >>>>>>>>>>>>                      */
> >>>>>>>>>>>>                     if (folio_test_large(folio)) {
> >>>>>>>>>>>>                             int err;
> >>>>>>>>>>>> +                       unsigned long next_addr, align;
> >>>>>>>>>>>>
> >>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
> >>>>>>>>>>>> -                               break;
> >>>>>>>>>>>> -                       if (!folio_trylock(folio))
> >>>>>>>>>>>> -                               break;
> >>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
> >>>>>>>>>>>> +                           !folio_trylock(folio))
> >>>>>>>>>>>> +                               goto skip_large_folio;
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
> >>>>>>>>>>> might be
> >>>>>>>>>>> pointing to other folios.
> >>>>>>>>>>>
> >>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
> >>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
> >>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
> >>>>>>>>>>> when we
> >>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
> >>>>>>>>>>
> >>>>>>>>>> Agreed. Thanks for pointing that out.
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
> >>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +                       /*
> >>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
> >>>>>>>>>>>> +                        * cannot mark the entire large folio as lazyfree,
> >>>>>>>>>>>> +                        * then just split it.
> >>>>>>>>>>>> +                        */
> >>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
> >>>>>>>>>>>> align ||
> >>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
> >>>>>>>>>>>> pte))
> >>>>>>>>>>>> +                               goto split_large_folio;
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +                       /*
> >>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the large
> >>>>>>>>>>>> +                        * folio is entirely within the given range.
> >>>>>>>>>>>> +                        */
> >>>>>>>>>>>> +                       folio_clear_dirty(folio);
> >>>>>>>>>>>> +                       folio_unlock(folio);
> >>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
> >>>>>>>>>>>> PAGE_SIZE) {
> >>>>>>>>>>>> +                               ptent = ptep_get(pte);
> >>>>>>>>>>>> +                               if (pte_young(ptent) ||
> >>>>>>>>>>>> pte_dirty(ptent)) {
> >>>>>>>>>>>> +                                       ptent = ptep_get_and_clear_full(
> >>>>>>>>>>>> +                                               mm, addr, pte,
> >>>>>>>>>>>> tlb->fullmm);
> >>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
> >>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
> >>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte, ptent);
> >>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
> >>>>>>>>>>>> addr);
> >>>>>>>>>>>> +                               }
> >>>>>>>>>>>
> >>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
> >>>>>>>>>>> unfolding
> >>>>>>>>>>> and folding again. It seems quite expensive.
> >>>>>>>>
> >>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
> >>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so that we
> >>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
> >>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and other
> >>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
> >>>>>>>> But
> >>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
> >>>>>>>> all
> >>>>>>>
> >>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
> >>>>>>> if process A forks process B and then unmaps itself, leaving B as the
> >>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
> >>>>>>> will reuse PTE one by one while the specific subpage is written.
> >>>>>>
> >>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio was 1.
> >>>>>> And since it is a large folio with each page mapped once in proc B, I thought
> >>>>>> every subpage write would cause a copy except the last one? I haven't looked at
> >>>>>> the code for a while. But I had it in my head that this is an area we need to
> >>>>>> improve for mTHP.
> >>>>>
> >>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
> >>>>> a single PTE remains mapped (refcount == 0).
> >>>>
> >>>> ^ == 1
> >>>
> >>> Ahh yes. That's what I meant. I got the behacviour vagulely right though.
> >>>
> >>> Anyway, regardless, I'm not sure we want to batch here. Or if we do, we want to
> >>> batch function that will only clear access and dirty.
> >>
> >> We likely want to detect a folio batch the "usual" way (as relaxed as
> >> possible), then do all the checks (#pte == folio_mapcount() under folio
> >> lock), and finally batch-update the access and dirty only.
>
> Something like:
>
> 1) We might want to factor out the existing single-pte case and extend
> it to handle multiple PTEs (nr_pages). For the existing code, we would
> pass in "nr_pages".
>
> For example, instead of "folio_mapcount(folio) != 1" you'd check
> "folio_mapcount(folio) != nr_pages" in there. And we'd need functions to
> abstract working on multiple ptes.
>
> 2) We'd add something like wrprotect_ptes(), that does the mkold+clean
> on multiple PTEs.
>
> Naming suggestion for such a function requested :)
>
> 3) Then, we might want to extend folio_pte_batch() by an *any_young and
> *any_dirty parameter that will get optimized out for other users. So you
> get that information right when scanning.
>
>
> Just a rough idea, devil is in the detail. But likely trying to abstrct
> the code to handle "multiple pages of the same folio" should come just
> naturally like we used to do for fork() and munmap() so far.
>
> --
> Cheers,
>
> David / dhildenb
>

Ryan Roberts March 7, 2024, 4:31 p.m. UTC | #20

On 07/03/2024 12:01, Barry Song wrote:
> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[e-mail]> wrote:
>>
>> On 07.03.24 12:42, Ryan Roberts wrote:
>>> On 07/03/2024 11:31, David Hildenbrand wrote:
>>>> On 07.03.24 12:26, Barry Song wrote:
>>>>> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>
>>>>>> On 07/03/2024 10:54, David Hildenbrand wrote:
>>>>>>> On 07.03.24 11:54, David Hildenbrand wrote:
>>>>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
>>>>>>>>> On 07/03/2024 09:33, Barry Song wrote:
>>>>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>>>>>>>>> +                                                struct folio *folio,
>>>>>>>>>>>>>>> pte_t *start_pte)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>>>>>>>> +                       return false;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>>>>>>>>> we don't do
>>>>>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>>>>>>>>
>>>>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>>>>>>>> associated
>>>>>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>>>>>>>>> should we still
>>>>>>>>>>>>> mark this folio as lazyfree?
>>>>>>>>>>>>
>>>>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>>>>>>>>> folio_likely_mapped_shared
>>>>>>>>>>>> can result in false negatives or false positives to balance the
>>>>>>>>>>>> overhead.  So I really don't know :-)
>>>>>>>>>>>>
>>>>>>>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
>>>>>>>>>>>>>>> flags, NULL);
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>      static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>>>>>>>>                                     unsigned long end, struct mm_walk
>>>>>>>>>>>>>>> *walk)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
>>>>>>>>>>>>>>> unsigned long addr,
>>>>>>>>>>>>>>>                      */
>>>>>>>>>>>>>>>                     if (folio_test_large(folio)) {
>>>>>>>>>>>>>>>                             int err;
>>>>>>>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
>>>>>>>>>>>>>> might be
>>>>>>>>>>>>>> pointing to other folios.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
>>>>>>>>>>>>>> when we
>>>>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>>>>>>>>> +                        * cannot mark the entire large folio as
>>>>>>>>>>>>>>> lazyfree,
>>>>>>>>>>>>>>> +                        * then just split it.
>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
>>>>>>>>>>>>>>> align ||
>>>>>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>>>>>>>> pte))
>>>>>>>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the
>>>>>>>>>>>>>>> large
>>>>>>>>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>>>>>>>> PAGE_SIZE) {
>>>>>>>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>>>>>>>> +                                       ptent =
>>>>>>>>>>>>>>> ptep_get_and_clear_full(
>>>>>>>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>>>>>>>> tlb->fullmm);
>>>>>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte,
>>>>>>>>>>>>>>> ptent);
>>>>>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>>>>>>>> addr);
>>>>>>>>>>>>>>> +                               }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
>>>>>>>>>>>>>> unfolding
>>>>>>>>>>>>>> and folding again. It seems quite expensive.
>>>>>>>>>>>
>>>>>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>>>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so
>>>>>>>>>>> that we
>>>>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits
>>>>>>>>>>> like
>>>>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and
>>>>>>>>>>> other
>>>>>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
>>>>>>>>>>> But
>>>>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
>>>>>>>>>>> all
>>>>>>>>>>
>>>>>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
>>>>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>>>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
>>>>>>>>>> will reuse PTE one by one while the specific subpage is written.
>>>>>>>>>
>>>>>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio
>>>>>>>>> was 1.
>>>>>>>>> And since it is a large folio with each page mapped once in proc B, I thought
>>>>>>>>> every subpage write would cause a copy except the last one? I haven't
>>>>>>>>> looked at
>>>>>>>>> the code for a while. But I had it in my head that this is an area we need to
>>>>>>>>> improve for mTHP.
>>>>>
>>>>> So sad I am wrong again 

Barry Song March 7, 2024, 6:54 p.m. UTC | #21

On Fri, Mar 8, 2024 at 12:31 AM Ryan Roberts <[e-mail]> wrote:
>
> On 07/03/2024 12:01, Barry Song wrote:
> > On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[e-mail]> wrote:
> >>
> >> On 07.03.24 12:42, Ryan Roberts wrote:
> >>> On 07/03/2024 11:31, David Hildenbrand wrote:
> >>>> On 07.03.24 12:26, Barry Song wrote:
> >>>>> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
> >>>>>>
> >>>>>> On 07/03/2024 10:54, David Hildenbrand wrote:
> >>>>>>> On 07.03.24 11:54, David Hildenbrand wrote:
> >>>>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
> >>>>>>>>> On 07/03/2024 09:33, Barry Song wrote:
> >>>>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Hey Barry,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Thanks for taking time to review!
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>> [...]
> >>>>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> >>>>>>>>>>>>>>> +                                                struct folio *folio,
> >>>>>>>>>>>>>>> pte_t *start_pte)
> >>>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
> >>>>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
> >>>>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
> >>>>>>>>>>>>>>> +                       return false;
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
> >>>>>>>>>>>>>> we don't do
> >>>>>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
> >>>>>>>>>>>>> associated
> >>>>>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
> >>>>>>>>>>>>> should we still
> >>>>>>>>>>>>> mark this folio as lazyfree?
> >>>>>>>>>>>>
> >>>>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
> >>>>>>>>>>>> folio_likely_mapped_shared
> >>>>>>>>>>>> can result in false negatives or false positives to balance the
> >>>>>>>>>>>> overhead.  So I really don't know :-)
> >>>>>>>>>>>>
> >>>>>>>>>>>> Maybe David and Vishal can give some comments here.
> >>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
> >>>>>>>>>>>>>> [1]
> >>>>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Yes, we should rebase our work against David’s changes.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
> >>>>>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
> >>>>>>>>>>>>>>> flags, NULL);
> >>>>>>>>>>>>>>> +}
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>      static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>>>>>>>>>>>>                                     unsigned long end, struct mm_walk
> >>>>>>>>>>>>>>> *walk)
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
> >>>>>>>>>>>>>>> unsigned long addr,
> >>>>>>>>>>>>>>>                      */
> >>>>>>>>>>>>>>>                     if (folio_test_large(folio)) {
> >>>>>>>>>>>>>>>                             int err;
> >>>>>>>>>>>>>>> +                       unsigned long next_addr, align;
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
> >>>>>>>>>>>>>>> -                               break;
> >>>>>>>>>>>>>>> -                       if (!folio_trylock(folio))
> >>>>>>>>>>>>>>> -                               break;
> >>>>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
> >>>>>>>>>>>>>>> +                           !folio_trylock(folio))
> >>>>>>>>>>>>>>> +                               goto skip_large_folio;
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
> >>>>>>>>>>>>>> might be
> >>>>>>>>>>>>>> pointing to other folios.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
> >>>>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
> >>>>>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
> >>>>>>>>>>>>>> when we
> >>>>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Agreed. Thanks for pointing that out.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
> >>>>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> +                       /*
> >>>>>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
> >>>>>>>>>>>>>>> +                        * cannot mark the entire large folio as
> >>>>>>>>>>>>>>> lazyfree,
> >>>>>>>>>>>>>>> +                        * then just split it.
> >>>>>>>>>>>>>>> +                        */
> >>>>>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
> >>>>>>>>>>>>>>> align ||
> >>>>>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
> >>>>>>>>>>>>>>> pte))
> >>>>>>>>>>>>>>> +                               goto split_large_folio;
> >>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>> +                       /*
> >>>>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the
> >>>>>>>>>>>>>>> large
> >>>>>>>>>>>>>>> +                        * folio is entirely within the given range.
> >>>>>>>>>>>>>>> +                        */
> >>>>>>>>>>>>>>> +                       folio_clear_dirty(folio);
> >>>>>>>>>>>>>>> +                       folio_unlock(folio);
> >>>>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
> >>>>>>>>>>>>>>> PAGE_SIZE) {
> >>>>>>>>>>>>>>> +                               ptent = ptep_get(pte);
> >>>>>>>>>>>>>>> +                               if (pte_young(ptent) ||
> >>>>>>>>>>>>>>> pte_dirty(ptent)) {
> >>>>>>>>>>>>>>> +                                       ptent =
> >>>>>>>>>>>>>>> ptep_get_and_clear_full(
> >>>>>>>>>>>>>>> +                                               mm, addr, pte,
> >>>>>>>>>>>>>>> tlb->fullmm);
> >>>>>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
> >>>>>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
> >>>>>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte,
> >>>>>>>>>>>>>>> ptent);
> >>>>>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
> >>>>>>>>>>>>>>> addr);
> >>>>>>>>>>>>>>> +                               }
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
> >>>>>>>>>>>>>> unfolding
> >>>>>>>>>>>>>> and folding again. It seems quite expensive.
> >>>>>>>>>>>
> >>>>>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
> >>>>>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so
> >>>>>>>>>>> that we
> >>>>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits
> >>>>>>>>>>> like
> >>>>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and
> >>>>>>>>>>> other
> >>>>>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
> >>>>>>>>>>> But
> >>>>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
> >>>>>>>>>>> all
> >>>>>>>>>>
> >>>>>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
> >>>>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
> >>>>>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
> >>>>>>>>>> will reuse PTE one by one while the specific subpage is written.
> >>>>>>>>>
> >>>>>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio
> >>>>>>>>> was 1.
> >>>>>>>>> And since it is a large folio with each page mapped once in proc B, I thought
> >>>>>>>>> every subpage write would cause a copy except the last one? I haven't
> >>>>>>>>> looked at
> >>>>>>>>> the code for a while. But I had it in my head that this is an area we need to
> >>>>>>>>> improve for mTHP.
> >>>>>
> >>>>> So sad I am wrong again 

David Hildenbrand March 7, 2024, 7:48 p.m. UTC | #22

On 07.03.24 19:54, Barry Song wrote:
> On Fri, Mar 8, 2024 at 12:31 AM Ryan Roberts <[e-mail]> wrote:
>>
>> On 07/03/2024 12:01, Barry Song wrote:
>>> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[e-mail]> wrote:
>>>>
>>>> On 07.03.24 12:42, Ryan Roberts wrote:
>>>>> On 07/03/2024 11:31, David Hildenbrand wrote:
>>>>>> On 07.03.24 12:26, Barry Song wrote:
>>>>>>> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On 07/03/2024 10:54, David Hildenbrand wrote:
>>>>>>>>> On 07.03.24 11:54, David Hildenbrand wrote:
>>>>>>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
>>>>>>>>>>> On 07/03/2024 09:33, Barry Song wrote:
>>>>>>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>>>>>>>>>>> +                                                struct folio *folio,
>>>>>>>>>>>>>>>>> pte_t *start_pte)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>>>>>>>>>> +                       return false;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>>>>>>>>>>> we don't do
>>>>>>>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>>>>>>>>>> associated
>>>>>>>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>>>>>>>>>>> should we still
>>>>>>>>>>>>>>> mark this folio as lazyfree?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>>>>>>>>>>> folio_likely_mapped_shared
>>>>>>>>>>>>>> can result in false negatives or false positives to balance the
>>>>>>>>>>>>>> overhead.  So I really don't know :-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
>>>>>>>>>>>>>>>>> flags, NULL);
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>       static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>>>>>>>>>>                                      unsigned long end, struct mm_walk
>>>>>>>>>>>>>>>>> *walk)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
>>>>>>>>>>>>>>>>> unsigned long addr,
>>>>>>>>>>>>>>>>>                       */
>>>>>>>>>>>>>>>>>                      if (folio_test_large(folio)) {
>>>>>>>>>>>>>>>>>                              int err;
>>>>>>>>>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>>>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
>>>>>>>>>>>>>>>> might be
>>>>>>>>>>>>>>>> pointing to other folios.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
>>>>>>>>>>>>>>>> when we
>>>>>>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>>>>>>>>>>> +                        * cannot mark the entire large folio as
>>>>>>>>>>>>>>>>> lazyfree,
>>>>>>>>>>>>>>>>> +                        * then just split it.
>>>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
>>>>>>>>>>>>>>>>> align ||
>>>>>>>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>>>>>>>>>> pte))
>>>>>>>>>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the
>>>>>>>>>>>>>>>>> large
>>>>>>>>>>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>>>>>>>>>> PAGE_SIZE) {
>>>>>>>>>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>>>>>>>>>> +                                       ptent =
>>>>>>>>>>>>>>>>> ptep_get_and_clear_full(
>>>>>>>>>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>>>>>>>>>> tlb->fullmm);
>>>>>>>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte,
>>>>>>>>>>>>>>>>> ptent);
>>>>>>>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>>>>>>>>>> addr);
>>>>>>>>>>>>>>>>> +                               }
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
>>>>>>>>>>>>>>>> unfolding
>>>>>>>>>>>>>>>> and folding again. It seems quite expensive.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>>>>>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so
>>>>>>>>>>>>> that we
>>>>>>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits
>>>>>>>>>>>>> like
>>>>>>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and
>>>>>>>>>>>>> other
>>>>>>>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
>>>>>>>>>>>>> But
>>>>>>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
>>>>>>>>>>>>> all
>>>>>>>>>>>>
>>>>>>>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
>>>>>>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>>>>>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
>>>>>>>>>>>> will reuse PTE one by one while the specific subpage is written.
>>>>>>>>>>>
>>>>>>>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio
>>>>>>>>>>> was 1.
>>>>>>>>>>> And since it is a large folio with each page mapped once in proc B, I thought
>>>>>>>>>>> every subpage write would cause a copy except the last one? I haven't
>>>>>>>>>>> looked at
>>>>>>>>>>> the code for a while. But I had it in my head that this is an area we need to
>>>>>>>>>>> improve for mTHP.
>>>>>>>
>>>>>>> So sad I am wrong again 

Ryan Roberts March 8, 2024, 1:05 p.m. UTC | #23

On 07/03/2024 18:54, Barry Song wrote:
> On Fri, Mar 8, 2024 at 12:31 AM Ryan Roberts <[e-mail]> wrote:
>>
>> On 07/03/2024 12:01, Barry Song wrote:
>>> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[e-mail]> wrote:
>>>>
>>>> On 07.03.24 12:42, Ryan Roberts wrote:
>>>>> On 07/03/2024 11:31, David Hildenbrand wrote:
>>>>>> On 07.03.24 12:26, Barry Song wrote:
>>>>>>> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On 07/03/2024 10:54, David Hildenbrand wrote:
>>>>>>>>> On 07.03.24 11:54, David Hildenbrand wrote:
>>>>>>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
>>>>>>>>>>> On 07/03/2024 09:33, Barry Song wrote:
>>>>>>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>>>>>>>>>>> +                                                struct folio *folio,
>>>>>>>>>>>>>>>>> pte_t *start_pte)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>>>>>>>>>> +                       return false;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>>>>>>>>>>> we don't do
>>>>>>>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>>>>>>>>>> associated
>>>>>>>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>>>>>>>>>>> should we still
>>>>>>>>>>>>>>> mark this folio as lazyfree?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>>>>>>>>>>> folio_likely_mapped_shared
>>>>>>>>>>>>>> can result in false negatives or false positives to balance the
>>>>>>>>>>>>>> overhead.  So I really don't know :-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
>>>>>>>>>>>>>>>>> flags, NULL);
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>      static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>>>>>>>>>>                                     unsigned long end, struct mm_walk
>>>>>>>>>>>>>>>>> *walk)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
>>>>>>>>>>>>>>>>> unsigned long addr,
>>>>>>>>>>>>>>>>>                      */
>>>>>>>>>>>>>>>>>                     if (folio_test_large(folio)) {
>>>>>>>>>>>>>>>>>                             int err;
>>>>>>>>>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>>>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
>>>>>>>>>>>>>>>> might be
>>>>>>>>>>>>>>>> pointing to other folios.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
>>>>>>>>>>>>>>>> when we
>>>>>>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>>>>>>>>>>> +                        * cannot mark the entire large folio as
>>>>>>>>>>>>>>>>> lazyfree,
>>>>>>>>>>>>>>>>> +                        * then just split it.
>>>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
>>>>>>>>>>>>>>>>> align ||
>>>>>>>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>>>>>>>>>> pte))
>>>>>>>>>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the
>>>>>>>>>>>>>>>>> large
>>>>>>>>>>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>>>>>>>>>> PAGE_SIZE) {
>>>>>>>>>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>>>>>>>>>> +                                       ptent =
>>>>>>>>>>>>>>>>> ptep_get_and_clear_full(
>>>>>>>>>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>>>>>>>>>> tlb->fullmm);
>>>>>>>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte,
>>>>>>>>>>>>>>>>> ptent);
>>>>>>>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>>>>>>>>>> addr);
>>>>>>>>>>>>>>>>> +                               }
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
>>>>>>>>>>>>>>>> unfolding
>>>>>>>>>>>>>>>> and folding again. It seems quite expensive.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>>>>>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so
>>>>>>>>>>>>> that we
>>>>>>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits
>>>>>>>>>>>>> like
>>>>>>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and
>>>>>>>>>>>>> other
>>>>>>>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
>>>>>>>>>>>>> But
>>>>>>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
>>>>>>>>>>>>> all
>>>>>>>>>>>>
>>>>>>>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
>>>>>>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>>>>>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
>>>>>>>>>>>> will reuse PTE one by one while the specific subpage is written.
>>>>>>>>>>>
>>>>>>>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio
>>>>>>>>>>> was 1.
>>>>>>>>>>> And since it is a large folio with each page mapped once in proc B, I thought
>>>>>>>>>>> every subpage write would cause a copy except the last one? I haven't
>>>>>>>>>>> looked at
>>>>>>>>>>> the code for a while. But I had it in my head that this is an area we need to
>>>>>>>>>>> improve for mTHP.
>>>>>>>
>>>>>>> So sad I am wrong again 

David Hildenbrand March 8, 2024, 1:27 p.m. UTC | #24

On 08.03.24 14:05, Ryan Roberts wrote:
> On 07/03/2024 18:54, Barry Song wrote:
>> On Fri, Mar 8, 2024 at 12:31 AM Ryan Roberts <[e-mail]> wrote:
>>>
>>> On 07/03/2024 12:01, Barry Song wrote:
>>>> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[e-mail]> wrote:
>>>>>
>>>>> On 07.03.24 12:42, Ryan Roberts wrote:
>>>>>> On 07/03/2024 11:31, David Hildenbrand wrote:
>>>>>>> On 07.03.24 12:26, Barry Song wrote:
>>>>>>>> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> On 07/03/2024 10:54, David Hildenbrand wrote:
>>>>>>>>>> On 07.03.24 11:54, David Hildenbrand wrote:
>>>>>>>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
>>>>>>>>>>>> On 07/03/2024 09:33, Barry Song wrote:
>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>>>>>>>>>>>>>>> +                                                struct folio *folio,
>>>>>>>>>>>>>>>>>> pte_t *start_pte)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>>>>>>>>>>> +                       return false;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>>>>>>>>>>>>>>> we don't do
>>>>>>>>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>>>>>>>>>>> associated
>>>>>>>>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
>>>>>>>>>>>>>>>> should we still
>>>>>>>>>>>>>>>> mark this folio as lazyfree?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
>>>>>>>>>>>>>>> folio_likely_mapped_shared
>>>>>>>>>>>>>>> can result in false negatives or false positives to balance the
>>>>>>>>>>>>>>> overhead.  So I really don't know :-)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>>>>>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
>>>>>>>>>>>>>>>>>> flags, NULL);
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>       static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>>>>>>>>>>>>>>                                      unsigned long end, struct mm_walk
>>>>>>>>>>>>>>>>>> *walk)
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
>>>>>>>>>>>>>>>>>> unsigned long addr,
>>>>>>>>>>>>>>>>>>                       */
>>>>>>>>>>>>>>>>>>                      if (folio_test_large(folio)) {
>>>>>>>>>>>>>>>>>>                              int err;
>>>>>>>>>>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>>>>>>>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>>>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
>>>>>>>>>>>>>>>>> might be
>>>>>>>>>>>>>>>>> pointing to other folios.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>>>>>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>>>>>>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
>>>>>>>>>>>>>>>>> when we
>>>>>>>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>>>>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>>>>>>>>>>>>>>> +                        * cannot mark the entire large folio as
>>>>>>>>>>>>>>>>>> lazyfree,
>>>>>>>>>>>>>>>>>> +                        * then just split it.
>>>>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
>>>>>>>>>>>>>>>>>> align ||
>>>>>>>>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>>>>>>>>>>> pte))
>>>>>>>>>>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the
>>>>>>>>>>>>>>>>>> large
>>>>>>>>>>>>>>>>>> +                        * folio is entirely within the given range.
>>>>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>>>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>>>>>>>>>>> PAGE_SIZE) {
>>>>>>>>>>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>>>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>>>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>>>>>>>>>>> +                                       ptent =
>>>>>>>>>>>>>>>>>> ptep_get_and_clear_full(
>>>>>>>>>>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>>>>>>>>>>> tlb->fullmm);
>>>>>>>>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
>>>>>>>>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>>>>>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte,
>>>>>>>>>>>>>>>>>> ptent);
>>>>>>>>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>>>>>>>>>>> addr);
>>>>>>>>>>>>>>>>>> +                               }
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
>>>>>>>>>>>>>>>>> unfolding
>>>>>>>>>>>>>>>>> and folding again. It seems quite expensive.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
>>>>>>>>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so
>>>>>>>>>>>>>> that we
>>>>>>>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits
>>>>>>>>>>>>>> like
>>>>>>>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and
>>>>>>>>>>>>>> other
>>>>>>>>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
>>>>>>>>>>>>>> But
>>>>>>>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
>>>>>>>>>>>>>> all
>>>>>>>>>>>>>
>>>>>>>>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
>>>>>>>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>>>>>>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
>>>>>>>>>>>>> will reuse PTE one by one while the specific subpage is written.
>>>>>>>>>>>>
>>>>>>>>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio
>>>>>>>>>>>> was 1.
>>>>>>>>>>>> And since it is a large folio with each page mapped once in proc B, I thought
>>>>>>>>>>>> every subpage write would cause a copy except the last one? I haven't
>>>>>>>>>>>> looked at
>>>>>>>>>>>> the code for a while. But I had it in my head that this is an area we need to
>>>>>>>>>>>> improve for mTHP.
>>>>>>>>
>>>>>>>> So sad I am wrong again 

Ryan Roberts March 8, 2024, 1:48 p.m. UTC | #25

On 08/03/2024 13:27, David Hildenbrand wrote:
> On 08.03.24 14:05, Ryan Roberts wrote:
>> On 07/03/2024 18:54, Barry Song wrote:
>>> On Fri, Mar 8, 2024 at 12:31 AM Ryan Roberts <[e-mail]> wrote:
>>>>
>>>> On 07/03/2024 12:01, Barry Song wrote:
>>>>> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[e-mail]> wrote:
>>>>>>
>>>>>> On 07.03.24 12:42, Ryan Roberts wrote:
>>>>>>> On 07/03/2024 11:31, David Hildenbrand wrote:
>>>>>>>> On 07.03.24 12:26, Barry Song wrote:
>>>>>>>>> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
>>>>>>>>>>
>>>>>>>>>> On 07/03/2024 10:54, David Hildenbrand wrote:
>>>>>>>>>>> On 07.03.24 11:54, David Hildenbrand wrote:
>>>>>>>>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
>>>>>>>>>>>>> On 07/03/2024 09:33, Barry Song wrote:
>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts
>>>>>>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]>
>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]>
>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang
>>>>>>>>>>>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> [...]
>>>>>>>>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned
>>>>>>>>>>>>>>>>>>> long addr,
>>>>>>>>>>>>>>>>>>> +                                                struct folio
>>>>>>>>>>>>>>>>>>> *folio,
>>>>>>>>>>>>>>>>>>> pte_t *start_pte)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>>>>>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>>>>>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>>>>>>>>>>>>>>>> +                       return false;
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not
>>>>>>>>>>>>>>>>>> precise, so
>>>>>>>>>>>>>>>>>> we don't do
>>>>>>>>>>>>>>>>>> this check with lots of loops and depending on the subpage's
>>>>>>>>>>>>>>>>>> mapcount.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
>>>>>>>>>>>>>>>>> associated
>>>>>>>>>>>>>>>>> with this folio and the cow folio has smaller size than this
>>>>>>>>>>>>>>>>> folio,
>>>>>>>>>>>>>>>>> should we still
>>>>>>>>>>>>>>>>> mark this folio as lazyfree?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact
>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>> folio_likely_mapped_shared
>>>>>>>>>>>>>>>> can result in false negatives or false positives to balance the
>>>>>>>>>>>>>>>> overhead.  So I really don't know :-)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Maybe David and Vishal can give some comments here.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yes, we should rebase our work against David’s changes.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr,
>>>>>>>>>>>>>>>>>>> start_pte,
>>>>>>>>>>>>>>>>>>> +                                        ptep_get(start_pte),
>>>>>>>>>>>>>>>>>>> nr_pages,
>>>>>>>>>>>>>>>>>>> flags, NULL);
>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>>       static int madvise_free_pte_range(pmd_t *pmd, unsigned
>>>>>>>>>>>>>>>>>>> long addr,
>>>>>>>>>>>>>>>>>>>                                      unsigned long end,
>>>>>>>>>>>>>>>>>>> struct mm_walk
>>>>>>>>>>>>>>>>>>> *walk)
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t
>>>>>>>>>>>>>>>>>>> *pmd,
>>>>>>>>>>>>>>>>>>> unsigned long addr,
>>>>>>>>>>>>>>>>>>>                       */
>>>>>>>>>>>>>>>>>>>                      if (folio_test_large(folio)) {
>>>>>>>>>>>>>>>>>>>                              int err;
>>>>>>>>>>>>>>>>>>> +                       unsigned long next_addr, align;
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>>>>>> -                       if (!folio_trylock(folio))
>>>>>>>>>>>>>>>>>>> -                               break;
>>>>>>>>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) !=
>>>>>>>>>>>>>>>>>>> 1 ||
>>>>>>>>>>>>>>>>>>> +                           !folio_trylock(folio))
>>>>>>>>>>>>>>>>>>> +                               goto skip_large_folio;
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some
>>>>>>>>>>>>>>>>>> of them
>>>>>>>>>>>>>>>>>> might be
>>>>>>>>>>>>>>>>>> pointing to other folios.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> for example, for a large folio with 16PTEs, you do
>>>>>>>>>>>>>>>>>> MADV_DONTNEED(15-16),
>>>>>>>>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults,
>>>>>>>>>>>>>>>>>> thus PTE15
>>>>>>>>>>>>>>>>>> and PTE16 will point to two different small folios. We can
>>>>>>>>>>>>>>>>>> only skip
>>>>>>>>>>>>>>>>>> when we
>>>>>>>>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Agreed. Thanks for pointing that out.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +                       align = folio_nr_pages(folio) *
>>>>>>>>>>>>>>>>>>> PAGE_SIZE;
>>>>>>>>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align,
>>>>>>>>>>>>>>>>>>> align);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>>>>>> +                        * If we mark only the subpages as
>>>>>>>>>>>>>>>>>>> lazyfree, or
>>>>>>>>>>>>>>>>>>> +                        * cannot mark the entire large folio as
>>>>>>>>>>>>>>>>>>> lazyfree,
>>>>>>>>>>>>>>>>>>> +                        * then just split it.
>>>>>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>>>>>> +                       if (next_addr > end || next_addr -
>>>>>>>>>>>>>>>>>>> addr !=
>>>>>>>>>>>>>>>>>>> align ||
>>>>>>>>>>>>>>>>>>> +                          
>>>>>>>>>>>>>>>>>>> !can_mark_large_folio_lazyfree(addr, folio,
>>>>>>>>>>>>>>>>>>> pte))
>>>>>>>>>>>>>>>>>>> +                               goto split_large_folio;
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +                       /*
>>>>>>>>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting
>>>>>>>>>>>>>>>>>>> if the
>>>>>>>>>>>>>>>>>>> large
>>>>>>>>>>>>>>>>>>> +                        * folio is entirely within the given
>>>>>>>>>>>>>>>>>>> range.
>>>>>>>>>>>>>>>>>>> +                        */
>>>>>>>>>>>>>>>>>>> +                       folio_clear_dirty(folio);
>>>>>>>>>>>>>>>>>>> +                       folio_unlock(folio);
>>>>>>>>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
>>>>>>>>>>>>>>>>>>> PAGE_SIZE) {
>>>>>>>>>>>>>>>>>>> +                               ptent = ptep_get(pte);
>>>>>>>>>>>>>>>>>>> +                               if (pte_young(ptent) ||
>>>>>>>>>>>>>>>>>>> pte_dirty(ptent)) {
>>>>>>>>>>>>>>>>>>> +                                       ptent =
>>>>>>>>>>>>>>>>>>> ptep_get_and_clear_full(
>>>>>>>>>>>>>>>>>>> +                                               mm, addr, pte,
>>>>>>>>>>>>>>>>>>> tlb->fullmm);
>>>>>>>>>>>>>>>>>>> +                                       ptent =
>>>>>>>>>>>>>>>>>>> pte_mkold(ptent);
>>>>>>>>>>>>>>>>>>> +                                       ptent =
>>>>>>>>>>>>>>>>>>> pte_mkclean(ptent);
>>>>>>>>>>>>>>>>>>> +                                       set_pte_at(mm, addr,
>>>>>>>>>>>>>>>>>>> pte,
>>>>>>>>>>>>>>>>>>> ptent);
>>>>>>>>>>>>>>>>>>> +                                      
>>>>>>>>>>>>>>>>>>> tlb_remove_tlb_entry(tlb, pte,
>>>>>>>>>>>>>>>>>>> addr);
>>>>>>>>>>>>>>>>>>> +                               }
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio,
>>>>>>>>>>>>>>>>>> you are
>>>>>>>>>>>>>>>>>> unfolding
>>>>>>>>>>>>>>>>>> and folding again. It seems quite expensive.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I'm not convinced we should be doing this in batches. We want the
>>>>>>>>>>>>>>> initial
>>>>>>>>>>>>>>> folio_pte_batch() to be as loose as possible regarding
>>>>>>>>>>>>>>> permissions so
>>>>>>>>>>>>>>> that we
>>>>>>>>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore
>>>>>>>>>>>>>>> SW bits
>>>>>>>>>>>>>>> like
>>>>>>>>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are
>>>>>>>>>>>>>>> RO and
>>>>>>>>>>>>>>> other
>>>>>>>>>>>>>>> RW too (e.g. due to cow - although with the current cow impl,
>>>>>>>>>>>>>>> probably not.
>>>>>>>>>>>>>>> But
>>>>>>>>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch
>>>>>>>>>>>>>>> that ignores
>>>>>>>>>>>>>>> all
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> You are correct. I believe this scenario could indeed occur. For
>>>>>>>>>>>>>> instance,
>>>>>>>>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
>>>>>>>>>>>>>> sole process owning the large folio.  The current wp_page_reuse()
>>>>>>>>>>>>>> function
>>>>>>>>>>>>>> will reuse PTE one by one while the specific subpage is written.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hmm - I thought it would only reuse if the total mapcount for the
>>>>>>>>>>>>> folio
>>>>>>>>>>>>> was 1.
>>>>>>>>>>>>> And since it is a large folio with each page mapped once in proc B,
>>>>>>>>>>>>> I thought
>>>>>>>>>>>>> every subpage write would cause a copy except the last one? I haven't
>>>>>>>>>>>>> looked at
>>>>>>>>>>>>> the code for a while. But I had it in my head that this is an area
>>>>>>>>>>>>> we need to
>>>>>>>>>>>>> improve for mTHP.
>>>>>>>>>
>>>>>>>>> So sad I am wrong again 

Barry Song March 8, 2024, 6:01 p.m. UTC | #26

On Fri, Mar 8, 2024 at 9:05 PM Ryan Roberts <[e-mail]> wrote:
>
> On 07/03/2024 18:54, Barry Song wrote:
> > On Fri, Mar 8, 2024 at 12:31 AM Ryan Roberts <[e-mail]> wrote:
> >>
> >> On 07/03/2024 12:01, Barry Song wrote:
> >>> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[e-mail]> wrote:
> >>>>
> >>>> On 07.03.24 12:42, Ryan Roberts wrote:
> >>>>> On 07/03/2024 11:31, David Hildenbrand wrote:
> >>>>>> On 07.03.24 12:26, Barry Song wrote:
> >>>>>>> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[e-mail]> wrote:
> >>>>>>>>
> >>>>>>>> On 07/03/2024 10:54, David Hildenbrand wrote:
> >>>>>>>>> On 07.03.24 11:54, David Hildenbrand wrote:
> >>>>>>>>>> On 07.03.24 11:50, Ryan Roberts wrote:
> >>>>>>>>>>> On 07/03/2024 09:33, Barry Song wrote:
> >>>>>>>>>>>> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[e-mail]> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Hey Barry,
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Thanks for taking time to review!
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> [...]
> >>>>>>>>>>>>>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> >>>>>>>>>>>>>>>>> +                                                struct folio *folio,
> >>>>>>>>>>>>>>>>> pte_t *start_pte)
> >>>>>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>>>>> +       int nr_pages = folio_nr_pages(folio);
> >>>>>>>>>>>>>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> >>>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>>> +       for (int i = 0; i < nr_pages; i++)
> >>>>>>>>>>>>>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
> >>>>>>>>>>>>>>>>> +                       return false;
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> we have moved to folio_estimated_sharers though it is not precise, so
> >>>>>>>>>>>>>>>> we don't do
> >>>>>>>>>>>>>>>> this check with lots of loops and depending on the subpage's mapcount.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> If we don't check the subpage’s mapcount, and there is a cow folio
> >>>>>>>>>>>>>>> associated
> >>>>>>>>>>>>>>> with this folio and the cow folio has smaller size than this folio,
> >>>>>>>>>>>>>>> should we still
> >>>>>>>>>>>>>>> mark this folio as lazyfree?
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> I agree, this is true. However, we've somehow accepted the fact that
> >>>>>>>>>>>>>> folio_likely_mapped_shared
> >>>>>>>>>>>>>> can result in false negatives or false positives to balance the
> >>>>>>>>>>>>>> overhead.  So I really don't know :-)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Maybe David and Vishal can give some comments here.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> BTW, do we need to rebase our work against David's changes[1]?
> >>>>>>>>>>>>>>>> [1]
> >>>>>>>>>>>>>>>> https://lore.kernel.org/linux-mm/[e-mail]/
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Yes, we should rebase our work against David’s changes.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
> >>>>>>>>>>>>>>>>> +                                        ptep_get(start_pte), nr_pages,
> >>>>>>>>>>>>>>>>> flags, NULL);
> >>>>>>>>>>>>>>>>> +}
> >>>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>>>      static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>>>>>>>>>>>>>>                                     unsigned long end, struct mm_walk
> >>>>>>>>>>>>>>>>> *walk)
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd,
> >>>>>>>>>>>>>>>>> unsigned long addr,
> >>>>>>>>>>>>>>>>>                      */
> >>>>>>>>>>>>>>>>>                     if (folio_test_large(folio)) {
> >>>>>>>>>>>>>>>>>                             int err;
> >>>>>>>>>>>>>>>>> +                       unsigned long next_addr, align;
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> -                       if (folio_estimated_sharers(folio) != 1)
> >>>>>>>>>>>>>>>>> -                               break;
> >>>>>>>>>>>>>>>>> -                       if (!folio_trylock(folio))
> >>>>>>>>>>>>>>>>> -                               break;
> >>>>>>>>>>>>>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
> >>>>>>>>>>>>>>>>> +                           !folio_trylock(folio))
> >>>>>>>>>>>>>>>>> +                               goto skip_large_folio;
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> I don't think we can skip all the PTEs for nr_pages, as some of them
> >>>>>>>>>>>>>>>> might be
> >>>>>>>>>>>>>>>> pointing to other folios.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
> >>>>>>>>>>>>>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
> >>>>>>>>>>>>>>>> and PTE16 will point to two different small folios. We can only skip
> >>>>>>>>>>>>>>>> when we
> >>>>>>>>>>>>>>>> are sure nr_pages == folio_pte_batch() is sure.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Agreed. Thanks for pointing that out.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
> >>>>>>>>>>>>>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
> >>>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>>> +                       /*
> >>>>>>>>>>>>>>>>> +                        * If we mark only the subpages as lazyfree, or
> >>>>>>>>>>>>>>>>> +                        * cannot mark the entire large folio as
> >>>>>>>>>>>>>>>>> lazyfree,
> >>>>>>>>>>>>>>>>> +                        * then just split it.
> >>>>>>>>>>>>>>>>> +                        */
> >>>>>>>>>>>>>>>>> +                       if (next_addr > end || next_addr - addr !=
> >>>>>>>>>>>>>>>>> align ||
> >>>>>>>>>>>>>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio,
> >>>>>>>>>>>>>>>>> pte))
> >>>>>>>>>>>>>>>>> +                               goto split_large_folio;
> >>>>>>>>>>>>>>>>> +
> >>>>>>>>>>>>>>>>> +                       /*
> >>>>>>>>>>>>>>>>> +                        * Avoid unnecessary folio splitting if the
> >>>>>>>>>>>>>>>>> large
> >>>>>>>>>>>>>>>>> +                        * folio is entirely within the given range.
> >>>>>>>>>>>>>>>>> +                        */
> >>>>>>>>>>>>>>>>> +                       folio_clear_dirty(folio);
> >>>>>>>>>>>>>>>>> +                       folio_unlock(folio);
> >>>>>>>>>>>>>>>>> +                       for (; addr != next_addr; pte++, addr +=
> >>>>>>>>>>>>>>>>> PAGE_SIZE) {
> >>>>>>>>>>>>>>>>> +                               ptent = ptep_get(pte);
> >>>>>>>>>>>>>>>>> +                               if (pte_young(ptent) ||
> >>>>>>>>>>>>>>>>> pte_dirty(ptent)) {
> >>>>>>>>>>>>>>>>> +                                       ptent =
> >>>>>>>>>>>>>>>>> ptep_get_and_clear_full(
> >>>>>>>>>>>>>>>>> +                                               mm, addr, pte,
> >>>>>>>>>>>>>>>>> tlb->fullmm);
> >>>>>>>>>>>>>>>>> +                                       ptent = pte_mkold(ptent);
> >>>>>>>>>>>>>>>>> +                                       ptent = pte_mkclean(ptent);
> >>>>>>>>>>>>>>>>> +                                       set_pte_at(mm, addr, pte,
> >>>>>>>>>>>>>>>>> ptent);
> >>>>>>>>>>>>>>>>> +                                       tlb_remove_tlb_entry(tlb, pte,
> >>>>>>>>>>>>>>>>> addr);
> >>>>>>>>>>>>>>>>> +                               }
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are
> >>>>>>>>>>>>>>>> unfolding
> >>>>>>>>>>>>>>>> and folding again. It seems quite expensive.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I'm not convinced we should be doing this in batches. We want the initial
> >>>>>>>>>>>>> folio_pte_batch() to be as loose as possible regarding permissions so
> >>>>>>>>>>>>> that we
> >>>>>>>>>>>>> reduce our chances of splitting folios to the min. (e.g. ignore SW bits
> >>>>>>>>>>>>> like
> >>>>>>>>>>>>> soft dirty, etc). I think it might be possible that some PTEs are RO and
> >>>>>>>>>>>>> other
> >>>>>>>>>>>>> RW too (e.g. due to cow - although with the current cow impl, probably not.
> >>>>>>>>>>>>> But
> >>>>>>>>>>>>> its fragile to assume that). Anyway, if we do an initial batch that ignores
> >>>>>>>>>>>>> all
> >>>>>>>>>>>>
> >>>>>>>>>>>> You are correct. I believe this scenario could indeed occur. For instance,
> >>>>>>>>>>>> if process A forks process B and then unmaps itself, leaving B as the
> >>>>>>>>>>>> sole process owning the large folio.  The current wp_page_reuse() function
> >>>>>>>>>>>> will reuse PTE one by one while the specific subpage is written.
> >>>>>>>>>>>
> >>>>>>>>>>> Hmm - I thought it would only reuse if the total mapcount for the folio
> >>>>>>>>>>> was 1.
> >>>>>>>>>>> And since it is a large folio with each page mapped once in proc B, I thought
> >>>>>>>>>>> every subpage write would cause a copy except the last one? I haven't
> >>>>>>>>>>> looked at
> >>>>>>>>>>> the code for a while. But I had it in my head that this is an area we need to
> >>>>>>>>>>> improve for mTHP.
> >>>>>>>
> >>>>>>> So sad I am wrong again 

Ryan Roberts March 11, 2024, 9:55 a.m. UTC | #27

[...]

>>>>> we don't want reclamation overhead later. and we want memories immediately
>>>>> available to others.
>>>>
>>>> But by that logic, you also don't want to leave the large folio partially mapped
>>>> all the way until the last subpage is CoWed. Surely you would want to reclaim it
>>>> when you reach partial map status?
>>>
>>> To some extent, I agree. But then we will have two many copies. The last
>>> subpage is small, and a safe place to copy instead.
>>>
>>> We actually had to tune userspace to decrease partial map as too much
>>> partial map both unfolded CONT-PTE and wasted too much memory. if a
>>> vma had too much partial map, we disabled mTHP on this VMA.
>>
>> I actually had a whacky idea around introducing selectable page size ABI
>> per-process that might help here. I know Android is doing work to make the
>> system 16K page compatible. You could run most of the system processes with 16K
>> ABI on top of 4K kernel. Then those processes don't even have the ability to
>> madvise/munmap/mprotect/mremap anything less than 16K alignment so that acts as
>> an anti-fragmentation mechanism while allowing non-16K capable processes to run
>> side-by-side. Just a passing thought...
> 
> Right, this project faces a challenge in supporting legacy
> 4KiB-aligned applications.
> but I don't find it will be an issue to run 16KiB-aligned applications
> on a kernel whose
> page size is 4KiB.

Yes, agreed that a 16K-aligned (or 64K-aligned) app will work without issue on
4K kernel, but it will also use getpagesize() and know what the page size is.
I'm suggesting you could actually run these apps on a 4K kernel but with a 16K
ABI and potentially get close to the native 16K performance out of them. It's
just a thought though - I don't have any data that actually shows this is better
than just running on a 4K kernel with a 4K ABI, and using 16K or 64K mTHP
opportunistically.

Barry Song March 11, 2024, 10:01 a.m. UTC | #28

On Mon, Mar 11, 2024 at 5:55 PM Ryan Roberts <[e-mail]> wrote:
>
> [...]
>
> >>>>> we don't want reclamation overhead later. and we want memories immediately
> >>>>> available to others.
> >>>>
> >>>> But by that logic, you also don't want to leave the large folio partially mapped
> >>>> all the way until the last subpage is CoWed. Surely you would want to reclaim it
> >>>> when you reach partial map status?
> >>>
> >>> To some extent, I agree. But then we will have two many copies. The last
> >>> subpage is small, and a safe place to copy instead.
> >>>
> >>> We actually had to tune userspace to decrease partial map as too much
> >>> partial map both unfolded CONT-PTE and wasted too much memory. if a
> >>> vma had too much partial map, we disabled mTHP on this VMA.
> >>
> >> I actually had a whacky idea around introducing selectable page size ABI
> >> per-process that might help here. I know Android is doing work to make the
> >> system 16K page compatible. You could run most of the system processes with 16K
> >> ABI on top of 4K kernel. Then those processes don't even have the ability to
> >> madvise/munmap/mprotect/mremap anything less than 16K alignment so that acts as
> >> an anti-fragmentation mechanism while allowing non-16K capable processes to run
> >> side-by-side. Just a passing thought...
> >
> > Right, this project faces a challenge in supporting legacy
> > 4KiB-aligned applications.
> > but I don't find it will be an issue to run 16KiB-aligned applications
> > on a kernel whose
> > page size is 4KiB.
>
> Yes, agreed that a 16K-aligned (or 64K-aligned) app will work without issue on
> 4K kernel, but it will also use getpagesize() and know what the page size is.
> I'm suggesting you could actually run these apps on a 4K kernel but with a 16K
> ABI and potentially get close to the native 16K performance out of them. It's
> just a thought though - I don't have any data that actually shows this is better
> than just running on a 4K kernel with a 4K ABI, and using 16K or 64K mTHP
> opportunistically.

 I fully agree with this as my Ubuntu filesystem can run on 4KiB, 16KiB and
64KiB basepage size as its elf files are 64KiB aligned. so I would expect
new Android apps/middleware move to 64KiB ABI though it might want to
change the base page size to 16KiB instead.
I believe this is the case.

Thanks
Barry

Ryan Roberts March 11, 2024, 3:07 p.m. UTC | #29

On 07/03/2024 09:07, Ryan Roberts wrote:
> On 07/03/2024 08:10, Barry Song wrote:
>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
>>>
>>> Hey Barry,
>>>
>>> Thanks for taking time to review!
>>>
>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
>>>>
>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
>>>>>
>>> [...]
>>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
>>>>> +                                                struct folio *folio, pte_t *start_pte)
>>>>> +{
>>>>> +       int nr_pages = folio_nr_pages(folio);
>>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
>>>>> +
>>>>> +       for (int i = 0; i < nr_pages; i++)
>>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
>>>>> +                       return false;
>>>>
>>>> we have moved to folio_estimated_sharers though it is not precise, so
>>>> we don't do
>>>> this check with lots of loops and depending on the subpage's mapcount.
>>>
>>> If we don't check the subpage’s mapcount, and there is a cow folio associated
>>> with this folio and the cow folio has smaller size than this folio,
>>> should we still
>>> mark this folio as lazyfree?
>>
>> I agree, this is true. However, we've somehow accepted the fact that
>> folio_likely_mapped_shared
>> can result in false negatives or false positives to balance the
>> overhead.  So I really don't know :-)
>>
>> Maybe David and Vishal can give some comments here.
>>
>>>
>>>> BTW, do we need to rebase our work against David's changes[1]?
>>>> [1] https://lore.kernel.org/linux-mm/[e-mail]/
>>>
>>> Yes, we should rebase our work against David’s changes.
>>>
>>>>
>>>>> +
>>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
>>>>> +                                        ptep_get(start_pte), nr_pages, flags, NULL);
>>>>> +}
>>>>> +
>>>>>  static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>                                 unsigned long end, struct mm_walk *walk)
>>>>>
>>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>                  */
>>>>>                 if (folio_test_large(folio)) {
>>>>>                         int err;
>>>>> +                       unsigned long next_addr, align;
>>>>>
>>>>> -                       if (folio_estimated_sharers(folio) != 1)
>>>>> -                               break;
>>>>> -                       if (!folio_trylock(folio))
>>>>> -                               break;
>>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
>>>>> +                           !folio_trylock(folio))
>>>>> +                               goto skip_large_folio;
>>>>
>>>>
>>>> I don't think we can skip all the PTEs for nr_pages, as some of them might be
>>>> pointing to other folios.
>>>>
>>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
>>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
>>>> and PTE16 will point to two different small folios. We can only skip when we
>>>> are sure nr_pages == folio_pte_batch() is sure.
>>>
>>> Agreed. Thanks for pointing that out.
>>>
>>>>
>>>>> +
>>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
>>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
>>>>> +
>>>>> +                       /*
>>>>> +                        * If we mark only the subpages as lazyfree, or
>>>>> +                        * cannot mark the entire large folio as lazyfree,
>>>>> +                        * then just split it.
>>>>> +                        */
>>>>> +                       if (next_addr > end || next_addr - addr != align ||
>>>>> +                           !can_mark_large_folio_lazyfree(addr, folio, pte))
>>>>> +                               goto split_large_folio;
>>>>> +
>>>>> +                       /*
>>>>> +                        * Avoid unnecessary folio splitting if the large
>>>>> +                        * folio is entirely within the given range.
>>>>> +                        */
>>>>> +                       folio_clear_dirty(folio);
>>>>> +                       folio_unlock(folio);
>>>>> +                       for (; addr != next_addr; pte++, addr += PAGE_SIZE) {
>>>>> +                               ptent = ptep_get(pte);
>>>>> +                               if (pte_young(ptent) || pte_dirty(ptent)) {
>>>>> +                                       ptent = ptep_get_and_clear_full(
>>>>> +                                               mm, addr, pte, tlb->fullmm);
>>>>> +                                       ptent = pte_mkold(ptent);
>>>>> +                                       ptent = pte_mkclean(ptent);
>>>>> +                                       set_pte_at(mm, addr, pte, ptent);
>>>>> +                                       tlb_remove_tlb_entry(tlb, pte, addr);
>>>>> +                               }
>>>>
>>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are unfolding
>>>> and folding again. It seems quite expensive.
> 
> I'm not convinced we should be doing this in batches. We want the initial
> folio_pte_batch() to be as loose as possible regarding permissions so that we
> reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
> soft dirty, etc). I think it might be possible that some PTEs are RO and other
> RW too (e.g. due to cow - although with the current cow impl, probably not. But
> its fragile to assume that). Anyway, if we do an initial batch that ignores all
> that then do this bit as a batch, you will end up smeering all the ptes with
> whatever properties were set on the first pte, which probably isn't right.
> 
> I've done a similar conversion for madvise_cold_or_pageout_pte_range() as part
> of my swap-out series v4 (hoping to post imminently, but still working out a
> latent bug that it triggers). I use ptep_test_and_clear_young() in that, which
> arm64 can apply per-pte but avoid doing a contpte unfold/fold. I know you have
> to clear dirty here too, but I think this pattern is preferable.
> 
> FYI, my swap-out series also halfway-batches madvise_free_pte_range() so that I
> can batch free_swap_and_cache() for the swap entry case. Ideally the work you
> are doing here would be rebased on top of that and plug-in to the approach
> implemented there. (subject to others' views of course).
> 
> I'll cc you when I post it.

I just sent out the swap-out series v4, as I presed the button I realized I
forgot to cc you - sorry about that! It's at [1]. Patch 2 and 6 are the
interesting ones from this PoV.

[1] https://lore.kernel.org/linux-mm/[e-mail]/


> 
>>>
>>> Thanks for your suggestion. I'll do this in batches in v3.
>>>
>>> Thanks again for your time!
>>>
>>> Best,
>>> Lance
>>>
>>>>
>>>>> +                       }
>>>>> +                       folio_mark_lazyfree(folio);
>>>>> +                       goto next_folio;
>>>>> +
>>>>> +split_large_folio:
>>>>>                         folio_get(folio);
>>>>>                         arch_leave_lazy_mmu_mode();
>>>>>                         pte_unmap_unlock(start_pte, ptl);
>>>>> @@ -688,13 +736,28 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
>>>>>                         err = split_folio(folio);
>>>>>                         folio_unlock(folio);
>>>>>                         folio_put(folio);
>>>>> -                       if (err)
>>>>> -                               break;
>>>>> -                       start_pte = pte =
>>>>> -                               pte_offset_map_lock(mm, pmd, addr, &ptl);
>>>>> -                       if (!start_pte)
>>>>> -                               break;
>>>>> -                       arch_enter_lazy_mmu_mode();
>>>>> +
>>>>> +                       /*
>>>>> +                        * If the large folio is locked or cannot be split,
>>>>> +                        * we just skip it.
>>>>> +                        */
>>>>> +                       if (err) {
>>>>> +skip_large_folio:
>>>>> +                               if (next_addr >= end)
>>>>> +                                       break;
>>>>> +                               pte += (next_addr - addr) / PAGE_SIZE;
>>>>> +                               addr = next_addr;
>>>>> +                       }
>>>>> +
>>>>> +                       if (!start_pte) {
>>>>> +                               start_pte = pte = pte_offset_map_lock(
>>>>> +                                       mm, pmd, addr, &ptl);
>>>>> +                               if (!start_pte)
>>>>> +                                       break;
>>>>> +                               arch_enter_lazy_mmu_mode();
>>>>> +                       }
>>>>> +
>>>>> +next_folio:
>>>>>                         pte--;
>>>>>                         addr -= PAGE_SIZE;
>>>>>                         continue;
>>>>> --
>>>>> 2.33.1
>>>>>
>>
>> Thanks
>> Barry
>

Lance Yang March 12, 2024, 10:20 a.m. UTC | #30

On Mon, Mar 11, 2024 at 11:07 PM Ryan Roberts <[e-mail]> wrote:
>
> On 07/03/2024 09:07, Ryan Roberts wrote:
> > On 07/03/2024 08:10, Barry Song wrote:
> >> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[e-mail]> wrote:
> >>>
> >>> Hey Barry,
> >>>
> >>> Thanks for taking time to review!
> >>>
> >>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[e-mail]> wrote:
> >>>>
> >>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[e-mail]> wrote:
> >>>>>
> >>> [...]
> >>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> >>>>> +                                                struct folio *folio, pte_t *start_pte)
> >>>>> +{
> >>>>> +       int nr_pages = folio_nr_pages(folio);
> >>>>> +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> >>>>> +
> >>>>> +       for (int i = 0; i < nr_pages; i++)
> >>>>> +               if (page_mapcount(folio_page(folio, i)) != 1)
> >>>>> +                       return false;
> >>>>
> >>>> we have moved to folio_estimated_sharers though it is not precise, so
> >>>> we don't do
> >>>> this check with lots of loops and depending on the subpage's mapcount.
> >>>
> >>> If we don't check the subpage’s mapcount, and there is a cow folio associated
> >>> with this folio and the cow folio has smaller size than this folio,
> >>> should we still
> >>> mark this folio as lazyfree?
> >>
> >> I agree, this is true. However, we've somehow accepted the fact that
> >> folio_likely_mapped_shared
> >> can result in false negatives or false positives to balance the
> >> overhead.  So I really don't know :-)
> >>
> >> Maybe David and Vishal can give some comments here.
> >>
> >>>
> >>>> BTW, do we need to rebase our work against David's changes[1]?
> >>>> [1] https://lore.kernel.org/linux-mm/[e-mail]/
> >>>
> >>> Yes, we should rebase our work against David’s changes.
> >>>
> >>>>
> >>>>> +
> >>>>> +       return nr_pages == folio_pte_batch(folio, addr, start_pte,
> >>>>> +                                        ptep_get(start_pte), nr_pages, flags, NULL);
> >>>>> +}
> >>>>> +
> >>>>>  static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>>                                 unsigned long end, struct mm_walk *walk)
> >>>>>
> >>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>>                  */
> >>>>>                 if (folio_test_large(folio)) {
> >>>>>                         int err;
> >>>>> +                       unsigned long next_addr, align;
> >>>>>
> >>>>> -                       if (folio_estimated_sharers(folio) != 1)
> >>>>> -                               break;
> >>>>> -                       if (!folio_trylock(folio))
> >>>>> -                               break;
> >>>>> +                       if (folio_estimated_sharers(folio) != 1 ||
> >>>>> +                           !folio_trylock(folio))
> >>>>> +                               goto skip_large_folio;
> >>>>
> >>>>
> >>>> I don't think we can skip all the PTEs for nr_pages, as some of them might be
> >>>> pointing to other folios.
> >>>>
> >>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16),
> >>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15
> >>>> and PTE16 will point to two different small folios. We can only skip when we
> >>>> are sure nr_pages == folio_pte_batch() is sure.
> >>>
> >>> Agreed. Thanks for pointing that out.
> >>>
> >>>>
> >>>>> +
> >>>>> +                       align = folio_nr_pages(folio) * PAGE_SIZE;
> >>>>> +                       next_addr = ALIGN_DOWN(addr + align, align);
> >>>>> +
> >>>>> +                       /*
> >>>>> +                        * If we mark only the subpages as lazyfree, or
> >>>>> +                        * cannot mark the entire large folio as lazyfree,
> >>>>> +                        * then just split it.
> >>>>> +                        */
> >>>>> +                       if (next_addr > end || next_addr - addr != align ||
> >>>>> +                           !can_mark_large_folio_lazyfree(addr, folio, pte))
> >>>>> +                               goto split_large_folio;
> >>>>> +
> >>>>> +                       /*
> >>>>> +                        * Avoid unnecessary folio splitting if the large
> >>>>> +                        * folio is entirely within the given range.
> >>>>> +                        */
> >>>>> +                       folio_clear_dirty(folio);
> >>>>> +                       folio_unlock(folio);
> >>>>> +                       for (; addr != next_addr; pte++, addr += PAGE_SIZE) {
> >>>>> +                               ptent = ptep_get(pte);
> >>>>> +                               if (pte_young(ptent) || pte_dirty(ptent)) {
> >>>>> +                                       ptent = ptep_get_and_clear_full(
> >>>>> +                                               mm, addr, pte, tlb->fullmm);
> >>>>> +                                       ptent = pte_mkold(ptent);
> >>>>> +                                       ptent = pte_mkclean(ptent);
> >>>>> +                                       set_pte_at(mm, addr, pte, ptent);
> >>>>> +                                       tlb_remove_tlb_entry(tlb, pte, addr);
> >>>>> +                               }
> >>>>
> >>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are unfolding
> >>>> and folding again. It seems quite expensive.
> >
> > I'm not convinced we should be doing this in batches. We want the initial
> > folio_pte_batch() to be as loose as possible regarding permissions so that we
> > reduce our chances of splitting folios to the min. (e.g. ignore SW bits like
> > soft dirty, etc). I think it might be possible that some PTEs are RO and other
> > RW too (e.g. due to cow - although with the current cow impl, probably not. But
> > its fragile to assume that). Anyway, if we do an initial batch that ignores all
> > that then do this bit as a batch, you will end up smeering all the ptes with
> > whatever properties were set on the first pte, which probably isn't right.
> >
> > I've done a similar conversion for madvise_cold_or_pageout_pte_range() as part
> > of my swap-out series v4 (hoping to post imminently, but still working out a
> > latent bug that it triggers). I use ptep_test_and_clear_young() in that, which
> > arm64 can apply per-pte but avoid doing a contpte unfold/fold. I know you have
> > to clear dirty here too, but I think this pattern is preferable.
> >
> > FYI, my swap-out series also halfway-batches madvise_free_pte_range() so that I
> > can batch free_swap_and_cache() for the swap entry case. Ideally the work you
> > are doing here would be rebased on top of that and plug-in to the approach
> > implemented there. (subject to others' views of course).
> >
> > I'll cc you when I post it.
>
> I just sent out the swap-out series v4, as I presed the button I realized I
> forgot to cc you - sorry about that! It's at [1]. Patch 2 and 6 are the

No worries about that. Thanks for letting me know!
I will rebase our work on Patch 2 and 6.

Thanks,
Lance

> interesting ones from this PoV.
>
> [1] https://lore.kernel.org/linux-mm/[e-mail]/



>
>
> >
> >>>
> >>> Thanks for your suggestion. I'll do this in batches in v3.
> >>>
> >>> Thanks again for your time!
> >>>
> >>> Best,
> >>> Lance
> >>>
> >>>>
> >>>>> +                       }
> >>>>> +                       folio_mark_lazyfree(folio);
> >>>>> +                       goto next_folio;
> >>>>> +
> >>>>> +split_large_folio:
> >>>>>                         folio_get(folio);
> >>>>>                         arch_leave_lazy_mmu_mode();
> >>>>>                         pte_unmap_unlock(start_pte, ptl);
> >>>>> @@ -688,13 +736,28 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> >>>>>                         err = split_folio(folio);
> >>>>>                         folio_unlock(folio);
> >>>>>                         folio_put(folio);
> >>>>> -                       if (err)
> >>>>> -                               break;
> >>>>> -                       start_pte = pte =
> >>>>> -                               pte_offset_map_lock(mm, pmd, addr, &ptl);
> >>>>> -                       if (!start_pte)
> >>>>> -                               break;
> >>>>> -                       arch_enter_lazy_mmu_mode();
> >>>>> +
> >>>>> +                       /*
> >>>>> +                        * If the large folio is locked or cannot be split,
> >>>>> +                        * we just skip it.
> >>>>> +                        */
> >>>>> +                       if (err) {
> >>>>> +skip_large_folio:
> >>>>> +                               if (next_addr >= end)
> >>>>> +                                       break;
> >>>>> +                               pte += (next_addr - addr) / PAGE_SIZE;
> >>>>> +                               addr = next_addr;
> >>>>> +                       }
> >>>>> +
> >>>>> +                       if (!start_pte) {
> >>>>> +                               start_pte = pte = pte_offset_map_lock(
> >>>>> +                                       mm, pmd, addr, &ptl);
> >>>>> +                               if (!start_pte)
> >>>>> +                                       break;
> >>>>> +                               arch_enter_lazy_mmu_mode();
> >>>>> +                       }
> >>>>> +
> >>>>> +next_folio:
> >>>>>                         pte--;
> >>>>>                         addr -= PAGE_SIZE;
> >>>>>                         continue;
> >>>>> --
> >>>>> 2.33.1
> >>>>>
> >>
> >> Thanks
> >> Barry
> >
>

Patch

diff --git a/mm/madvise.c b/mm/madvise.c
index 44a498c94158..1437ac6eb25e 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -616,6 +616,20 @@  static long madvise_pageout(struct vm_area_struct *vma,
 	return 0;
 }
 
+static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
+						 struct folio *folio, pte_t *start_pte)
+{
+	int nr_pages = folio_nr_pages(folio);
+	fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
+
+	for (int i = 0; i < nr_pages; i++)
+		if (page_mapcount(folio_page(folio, i)) != 1)
+			return false;
+
+	return nr_pages == folio_pte_batch(folio, addr, start_pte,
+					 ptep_get(start_pte), nr_pages, flags, NULL);
+}
+
 static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
 				unsigned long end, struct mm_walk *walk)
 
@@ -676,11 +690,45 @@  static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
 		 */
 		if (folio_test_large(folio)) {
 			int err;
+			unsigned long next_addr, align;
 
-			if (folio_estimated_sharers(folio) != 1)
-				break;
-			if (!folio_trylock(folio))
-				break;
+			if (folio_estimated_sharers(folio) != 1 ||
+			    !folio_trylock(folio))
+				goto skip_large_folio;
+
+			align = folio_nr_pages(folio) * PAGE_SIZE;
+			next_addr = ALIGN_DOWN(addr + align, align);
+
+			/*
+			 * If we mark only the subpages as lazyfree, or
+			 * cannot mark the entire large folio as lazyfree,
+			 * then just split it.
+			 */
+			if (next_addr > end || next_addr - addr != align ||
+			    !can_mark_large_folio_lazyfree(addr, folio, pte))
+				goto split_large_folio;
+
+			/*
+			 * Avoid unnecessary folio splitting if the large
+			 * folio is entirely within the given range.
+			 */
+			folio_clear_dirty(folio);
+			folio_unlock(folio);
+			for (; addr != next_addr; pte++, addr += PAGE_SIZE) {
+				ptent = ptep_get(pte);
+				if (pte_young(ptent) || pte_dirty(ptent)) {
+					ptent = ptep_get_and_clear_full(
+						mm, addr, pte, tlb->fullmm);
+					ptent = pte_mkold(ptent);
+					ptent = pte_mkclean(ptent);
+					set_pte_at(mm, addr, pte, ptent);
+					tlb_remove_tlb_entry(tlb, pte, addr);
+				}
+			}
+			folio_mark_lazyfree(folio);
+			goto next_folio;
+
+split_large_folio:
 			folio_get(folio);
 			arch_leave_lazy_mmu_mode();
 			pte_unmap_unlock(start_pte, ptl);
@@ -688,13 +736,28 @@  static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
 			err = split_folio(folio);
 			folio_unlock(folio);
 			folio_put(folio);
-			if (err)
-				break;
-			start_pte = pte =
-				pte_offset_map_lock(mm, pmd, addr, &ptl);
-			if (!start_pte)
-				break;
-			arch_enter_lazy_mmu_mode();
+
+			/*
+			 * If the large folio is locked or cannot be split,
+			 * we just skip it.
+			 */
+			if (err) {
+skip_large_folio:
+				if (next_addr >= end)
+					break;
+				pte += (next_addr - addr) / PAGE_SIZE;
+				addr = next_addr;
+			}
+
+			if (!start_pte) {
+				start_pte = pte = pte_offset_map_lock(
+					mm, pmd, addr, &ptl);
+				if (!start_pte)
+					break;
+				arch_enter_lazy_mmu_mode();
+			}
+
+next_folio:
 			pte--;
 			addr -= PAGE_SIZE;
 			continue;