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/[email protected]
[2] https://lore.kernel.org/linux-mm/[email protected]/
Signed-off-by: Lance Yang <[email protected]>
---
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/[email protected]/
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;
+
+ 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;
--
2.33.1
On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]
> [2] https://lore.kernel.org/linux-mm/20240214204435.167852-1-david@redhatcom/
>
> Signed-off-by: Lance Yang <[email protected]>
> ---
> 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/[email protected]/
>
> 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/[email protected]/
> +
> + 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
Hey Barry,
Thanks for taking time to review!
On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>
> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/20240227201548.857831-1-david@redhatcom/
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
On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>
> Hey Barry,
>
> Thanks for taking time to review!
>
> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
> >
> > On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>
> 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
On 07.03.24 09:10, Barry Song wrote:
> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>
>> Hey Barry,
>>
>> Thanks for taking time to review!
>>
>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>
>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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)
No new direct subpage mapcount users please if avoidable. Also, without
holding the page lock this might be racy (did not stare at the bigger code).
>>>> + 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.
Well, I did not accept the fact yet that folio_likely_mapped_shared()
can give false negatives :)
So I've been working on a replacement [1], also in the context of
removing the subpage mapcounts. But there is still a lot to resolve
around that, first step being to introduce the total mapcount.
But independent of that, MADV_FREE is special (compared to
MADV_DONTNEED), because we really have to make sure all page table
mappings that map the folio will be covered here.
What could work for large folios is making sure that #ptes that map the
folio here correspond to the folio_mapcount(). And folio_mapcount()
should be called under folio lock, to avoid racing with swapout/migration.
So instead of checking each page_mapcount(), see how many PTEs you could
batch, try taking the folio lock, and compare the number of batched PTEs
against folio_mapcount(). If they match, we're good. if not, there is
some other folio mapping somewhere else (other process, mremap).
folio_likely_mapped_shared() should still be used as an early exit point.
[1]
https://lore.kernel.org/all/[email protected]/T/#u
--
Cheers,
David / dhildenb
On 07/03/2024 08:10, Barry Song wrote:
> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>
>> Hey Barry,
>>
>> Thanks for taking time to review!
>>
>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>
>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>
>> 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
On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[email protected]> wrote:
>
> On 07/03/2024 08:10, Barry Song wrote:
> > On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
> >>
> >> Hey Barry,
> >>
> >> Thanks for taking time to review!
> >>
> >> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
> >>>
> >>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
> >>
> >> 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
On 07/03/2024 09:33, Barry Song wrote:
> On Thu, Mar 7, 2024 at 10:07 PM Ryan Roberts <[email protected]> wrote:
>>
>> On 07/03/2024 08:10, Barry Song wrote:
>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>
>>>> Hey Barry,
>>>>
>>>> Thanks for taking time to review!
>>>>
>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>
>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>
>>>> 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
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 <[email protected]> wrote:
>>>
>>> On 07/03/2024 08:10, Barry Song wrote:
>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>
>>>>> Hey Barry,
>>>>>
>>>>> Thanks for taking time to review!
>>>>>
>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>
>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>
>>>>> 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).
--
Cheers,
David / dhildenb
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 <[email protected]> wrote:
>>>>
>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>
>>>>>> Hey Barry,
>>>>>>
>>>>>> Thanks for taking time to review!
>>>>>>
>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>
>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>
>>>>>> 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
--
Cheers,
David / dhildenb
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 <[email protected]> wrote:
>>>>>
>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>>
>>>>>>> Hey Barry,
>>>>>>>
>>>>>>> Thanks for taking time to review!
>>>>>>>
>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>>
>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>>
>>>>>>> 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.
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 <[email protected]> wrote:
>>>>>>
>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>>>
>>>>>>>> Hey Barry,
>>>>>>>>
>>>>>>>> Thanks for taking time to review!
>>>>>>>>
>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>>>
>>>>>>>> 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
On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[email protected]> 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 <[email protected]> wrote:
> >>>>>
> >>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <ioworker0@gmailcom> wrote:
> >>>>>>>
> >>>>>>> Hey Barry,
> >>>>>>>
> >>>>>>> Thanks for taking time to review!
> >>>>>>>
> >>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
> >>>>>>>>
> >>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
> >>>>>>>
> >>>>>>> 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 ????
> >>
> >> wp_page_reuse() will currently reuse a PTE part of a large folio only if
> >> a single PTE remains mapped (refcount == 0).
> >
> > ^ == 1
seems this needs improvement. it is a waste the last subpage can
reuse the whole large folio. i was doing it in a quite different way,
if the large folio had only one subpage left, i would do copy and
released the large folio[1]. and if i could reuse the whole large folio
with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
we don't have this cont-pte luxury exposed to mm, so i guess we can
not do [2] easily, but [1] seems to be an optimization.
[1] https://github.com/OnePlusOSS/android_kernel_oneplus_sm8650/blob/oneplus/sm8650_u_14.0.0_oneplus12/mm/memory.c#L3977
[2] https://github.com/OnePlusOSS/android_kernel_oneplus_sm8650/blob/oneplus/sm8650_u_14.0.0_oneplus12/mm/memory.c#L3812
>
> 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.
>
Thanks
Barry
On 07.03.24 12:26, Barry Song wrote:
> On Thu, Mar 7, 2024 at 7:13 PM Ryan Roberts <[email protected]> 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 <[email protected]> wrote:
>>>>>>>
>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>> Hey Barry,
>>>>>>>>>
>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>
>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>>>>
>>>>>>>>> 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 ????
>
>>>>
>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
>>>> a single PTE remains mapped (refcount == 0).
>>>
>>> ^ == 1
>
> seems this needs improvement. it is a waste the last subpage can
My take that is WIP:
https://lore.kernel.org/all/[email protected]/T/#u
> reuse the whole large folio. i was doing it in a quite different way,
> if the large folio had only one subpage left, i would do copy and
> released the large folio[1]. and if i could reuse the whole large folio
> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
> we don't have this cont-pte luxury exposed to mm, so i guess we can
> not do [2] easily, but [1] seems to be an optimization.
Yeah, I had essentially the same idea: just free up the large folio if
most of the stuff is unmapped. But that's rather a corner-case
optimization, so I did not proceed with that.
--
Cheers,
David / dhildenb
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 <[email protected]> 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 <[email protected]> wrote:
>>>>>>>>
>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>> Hey Barry,
>>>>>>>>>>
>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>
>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>>>>>
>>>>>>>>>> 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 ????
>>
>>>>>
>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
>>>>> a single PTE remains mapped (refcount == 0).
>>>>
>>>> ^ == 1
>>
>> seems this needs improvement. it is a waste the last subpage can
>
> My take that is WIP:
>
> https://lore.kernel.org/all/[email protected]/T/#u
>
>> reuse the whole large folio. i was doing it in a quite different way,
>> if the large folio had only one subpage left, i would do copy and
>> released the large folio[1]. and if i could reuse the whole large folio
>> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
>> we don't have this cont-pte luxury exposed to mm, so i guess we can
>> not do [2] easily, but [1] seems to be an optimization.
>
> Yeah, I had essentially the same idea: just free up the large folio if most of
> the stuff is unmapped. But that's rather a corner-case optimization, so I did
> not proceed with that.
>
I'm not sure it's a corner case, really? - process forks, then both parent and
child and write to all pages in what was previously a fully & contiguously
mapped large folio?
Reggardless, why is it an optimization to do the copy for the last subpage and
syncrhonously free the large folio? It's already partially mapped so is on the
deferred split list and can be split if memory is tight.
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 <[email protected]> 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 <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>
>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>>>>>>
>>>>>>>>>>> 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 ????
>>>
>>>>>>
>>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
>>>>>> a single PTE remains mapped (refcount == 0).
>>>>>
>>>>> ^ == 1
>>>
>>> seems this needs improvement. it is a waste the last subpage can
>>
>> My take that is WIP:
>>
>> https://lore.kernel.org/all/[email protected]/T/#u
>>
>>> reuse the whole large folio. i was doing it in a quite different way,
>>> if the large folio had only one subpage left, i would do copy and
>>> released the large folio[1]. and if i could reuse the whole large folio
>>> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
>>> we don't have this cont-pte luxury exposed to mm, so i guess we can
>>> not do [2] easily, but [1] seems to be an optimization.
>>
>> Yeah, I had essentially the same idea: just free up the large folio if most of
>> the stuff is unmapped. But that's rather a corner-case optimization, so I did
>> not proceed with that.
>>
>
> I'm not sure it's a corner case, really? - process forks, then both parent and
> child and write to all pages in what was previously a fully & contiguously
> mapped large folio?
Well, with 2 MiB my assumption was that while it can happen, it's rather
rare. With smaller THP it might get more likely, agreed.
>
> Reggardless, why is it an optimization to do the copy for the last subpage and
> syncrhonously free the large folio? It's already partially mapped so is on the
> deferred split list and can be split if memory is tight.
At least for 2 MiB THP, it might make sense to make that large folio
available immediately again, even without memory pressure. Even
compaction would not compact it.
--
Cheers,
David / dhildenb
On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[email protected]> 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 <ryan.roberts@armcom> 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 <[email protected]> wrote:
> >>>>>>>>>
> >>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> Hey Barry,
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks for taking time to review!
> >>>>>>>>>>>
> >>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
> >>>>>>>>>>>
> >>>>>>>>>>> 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 ????
> >>>
> >>>>>>
> >>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
> >>>>>> a single PTE remains mapped (refcount == 0).
> >>>>>
> >>>>> ^ == 1
> >>>
> >>> seems this needs improvement. it is a waste the last subpage can
> >>
> >> My take that is WIP:
> >>
> >> https://lore.kernel.org/all/[email protected]/T/#u
> >>
> >>> reuse the whole large folio. i was doing it in a quite different way,
> >>> if the large folio had only one subpage left, i would do copy and
> >>> released the large folio[1]. and if i could reuse the whole large folio
> >>> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
> >>> we don't have this cont-pte luxury exposed to mm, so i guess we can
> >>> not do [2] easily, but [1] seems to be an optimization.
> >>
> >> Yeah, I had essentially the same idea: just free up the large folio if most of
> >> the stuff is unmapped. But that's rather a corner-case optimization, so I did
> >> not proceed with that.
> >>
> >
> > I'm not sure it's a corner case, really? - process forks, then both parent and
> > child and write to all pages in what was previously a fully & contiguously
> > mapped large folio?
>
> Well, with 2 MiB my assumption was that while it can happen, it's rather
> rare. With smaller THP it might get more likely, agreed.
>
> >
> > Reggardless, why is it an optimization to do the copy for the last subpage and
> > syncrhonously free the large folio? It's already partially mapped so is on the
> > deferred split list and can be split if memory is tight.
we don't want reclamation overhead later. and we want memories immediately
available to others. reclamation will always cause latency and affect User
experience. split_folio is not cheap :-) if the number of this kind of
large folios
is huge, the waste can be huge for some while.
it is not a corner case for large folio swap-in. while someone writes
one subpage, I swap-in a large folio, wp_reuse will immediately
be called. This can cause waste quite often. One outcome of this
discussion is that I realize I should investigate this issue immediately
in the swap-in series as my off-tree code has optimized reuse but
mainline hasn't.
>
> At least for 2 MiB THP, it might make sense to make that large folio
> available immediately again, even without memory pressure. Even
> compaction would not compact it.
It is also true for 64KiB. as we want other processes to allocate
64KiB successfully as much as possible, and reduce the rate of
falling back to small folios. by releasing 64KiB directly to buddy
rather than splitting and returning 15*4KiB in shrinker, we reduce
buddy fragmentation too.
>
> --
> Cheers,
>
> David / dhildenb
>
Thanks
Barry
On 07.03.24 13:01, Barry Song wrote:
> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>>>>>>>>
>>>>>>>>>>>>> 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 ????
>>>>>
>>>>>>>>
>>>>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
>>>>>>>> a single PTE remains mapped (refcount == 0).
>>>>>>>
>>>>>>> ^ == 1
>>>>>
>>>>> seems this needs improvement. it is a waste the last subpage can
>>>>
>>>> My take that is WIP:
>>>>
>>>> https://lore.kernel.org/all/[email protected]/T/#u
>>>>
>>>>> reuse the whole large folio. i was doing it in a quite different way,
>>>>> if the large folio had only one subpage left, i would do copy and
>>>>> released the large folio[1]. and if i could reuse the whole large folio
>>>>> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
>>>>> we don't have this cont-pte luxury exposed to mm, so i guess we can
>>>>> not do [2] easily, but [1] seems to be an optimization.
>>>>
>>>> Yeah, I had essentially the same idea: just free up the large folio if most of
>>>> the stuff is unmapped. But that's rather a corner-case optimization, so I did
>>>> not proceed with that.
>>>>
>>>
>>> I'm not sure it's a corner case, really? - process forks, then both parent and
>>> child and write to all pages in what was previously a fully & contiguously
>>> mapped large folio?
>>
>> Well, with 2 MiB my assumption was that while it can happen, it's rather
>> rare. With smaller THP it might get more likely, agreed.
>>
>>>
>>> Reggardless, why is it an optimization to do the copy for the last subpage and
>>> syncrhonously free the large folio? It's already partially mapped so is on the
>>> deferred split list and can be split if memory is tight.
>
> we don't want reclamation overhead later. and we want memories immediately
> available to others. reclamation will always cause latency and affect User
> experience. split_folio is not cheap :-) if the number of this kind of
> large folios
> is huge, the waste can be huge for some while.
>
> it is not a corner case for large folio swap-in. while someone writes
> one subpage, I swap-in a large folio, wp_reuse will immediately
> be called. This can cause waste quite often. One outcome of this
> discussion is that I realize I should investigate this issue immediately
> in the swap-in series as my off-tree code has optimized reuse but
> mainline hasn't.
Note that if the swp entry was exclusive, the subpage will be marked
PAE, so wp_reuse() will (and must!) reuse it.
We fallback to the refcount==1 scheme only if PAE is not set for that
subpage.
--
Cheers,
David / dhildenb
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 <[email protected]> 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 <[email protected]> wrote:
> >>>>>>
> >>>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
> >>>>>>>>
> >>>>>>>> Hey Barry,
> >>>>>>>>
> >>>>>>>> Thanks for taking time to review!
> >>>>>>>>
> >>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <21cnbao@gmailcom> wrote:
> >>>>>>>>>
> >>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
> >>>>>>>>
> >>>>>>>> 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
>
Thanks a lot, David!
Got it. I'll do my best.
Thanks,
Lance
On Thu, Mar 7, 2024 at 10:58 PM David Hildenbrand <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> >>>>>>>>
> >>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Hey Barry,
> >>>>>>>>>>
> >>>>>>>>>> Thanks for taking time to review!
> >>>>>>>>>>
> >>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
> >>>>>>>>>>
> >>>>>>>>>> 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
>
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 <[email protected]> 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 <[email protected]> wrote:
>>>>>>>>
>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>>>>>
>>>>>>>>>> Hey Barry,
>>>>>>>>>>
>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>
>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>>>>>
>>>>>>>>>> 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
On 07/03/2024 12:01, Barry Song wrote:
> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>>>>>>>>
>>>>>>>>>>>>> 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 ????
>>>>>
>>>>>>>>
>>>>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
>>>>>>>> a single PTE remains mapped (refcount == 0).
>>>>>>>
>>>>>>> ^ == 1
>>>>>
>>>>> seems this needs improvement. it is a waste the last subpage can
>>>>
>>>> My take that is WIP:
>>>>
>>>> https://lore.kernel.org/all/[email protected]/T/#u
>>>>
>>>>> reuse the whole large folio. i was doing it in a quite different way,
>>>>> if the large folio had only one subpage left, i would do copy and
>>>>> released the large folio[1]. and if i could reuse the whole large folio
>>>>> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
>>>>> we don't have this cont-pte luxury exposed to mm, so i guess we can
>>>>> not do [2] easily, but [1] seems to be an optimization.
>>>>
>>>> Yeah, I had essentially the same idea: just free up the large folio if most of
>>>> the stuff is unmapped. But that's rather a corner-case optimization, so I did
>>>> not proceed with that.
>>>>
>>>
>>> I'm not sure it's a corner case, really? - process forks, then both parent and
>>> child and write to all pages in what was previously a fully & contiguously
>>> mapped large folio?
>>
>> Well, with 2 MiB my assumption was that while it can happen, it's rather
>> rare. With smaller THP it might get more likely, agreed.
>>
>>>
>>> Reggardless, why is it an optimization to do the copy for the last subpage and
>>> syncrhonously free the large folio? It's already partially mapped so is on the
>>> deferred split list and can be split if memory is tight.
>
> 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?
> reclamation will always cause latency and affect User
> experience. split_folio is not cheap :-)
But neither is memcpy(4K) I'd imagine. But I get your point.
> if the number of this kind of
> large folios
> is huge, the waste can be huge for some while.
>
> it is not a corner case for large folio swap-in. while someone writes
> one subpage, I swap-in a large folio, wp_reuse will immediately
> be called. This can cause waste quite often. One outcome of this
> discussion is that I realize I should investigate this issue immediately
> in the swap-in series as my off-tree code has optimized reuse but
> mainline hasn't.
>
>>
>> At least for 2 MiB THP, it might make sense to make that large folio
>> available immediately again, even without memory pressure. Even
>> compaction would not compact it.
>
> It is also true for 64KiB. as we want other processes to allocate
> 64KiB successfully as much as possible, and reduce the rate of
> falling back to small folios. by releasing 64KiB directly to buddy
> rather than splitting and returning 15*4KiB in shrinker, we reduce
> buddy fragmentation too.
>
>>
>> --
>> Cheers,
>>
>> David / dhildenb
>>
>
> Thanks
> Barry
On Fri, Mar 8, 2024 at 12:31 AM Ryan Roberts <[email protected]> wrote:
>
> On 07/03/2024 12:01, Barry Song wrote:
> > On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Hey Barry,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Thanks for taking time to review!
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 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 ????
> >>>>>
> >>>>>>>>
> >>>>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
> >>>>>>>> a single PTE remains mapped (refcount == 0).
> >>>>>>>
> >>>>>>> ^ == 1
> >>>>>
> >>>>> seems this needs improvement. it is a waste the last subpage can
> >>>>
> >>>> My take that is WIP:
> >>>>
> >>>> https://lore.kernel.org/all/[email protected]/T/#u
> >>>>
> >>>>> reuse the whole large folio. i was doing it in a quite different way,
> >>>>> if the large folio had only one subpage left, i would do copy and
> >>>>> released the large folio[1]. and if i could reuse the whole large folio
> >>>>> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
> >>>>> we don't have this cont-pte luxury exposed to mm, so i guess we can
> >>>>> not do [2] easily, but [1] seems to be an optimization.
> >>>>
> >>>> Yeah, I had essentially the same idea: just free up the large folio if most of
> >>>> the stuff is unmapped. But that's rather a corner-case optimization, so I did
> >>>> not proceed with that.
> >>>>
> >>>
> >>> I'm not sure it's a corner case, really? - process forks, then both parent and
> >>> child and write to all pages in what was previously a fully & contiguously
> >>> mapped large folio?
> >>
> >> Well, with 2 MiB my assumption was that while it can happen, it's rather
> >> rare. With smaller THP it might get more likely, agreed.
> >>
> >>>
> >>> Reggardless, why is it an optimization to do the copy for the last subpage and
> >>> syncrhonously free the large folio? It's already partially mapped so is on the
> >>> deferred split list and can be split if memory is tight.
> >
> > 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.
>
> > reclamation will always cause latency and affect User
> > experience. split_folio is not cheap :-)
>
> But neither is memcpy(4K) I'd imagine. But I get your point.
In a real product scenario, we need to consider the success rate of
allocating large folios.
Currently, it's only 7%, as reported here[1], with no method to keep
large folios intact in a
buddy system.
Yu's TAO[2] chose to release the large folio entirely after copying
the mapped parts
onto smaller folios in vmscan,
[1] https://lore.kernel.org/linux-mm/[email protected]/
[2] https://lore.kernel.org/linux-mm/20240229183436.4110845-1-yuzhao@googlecom/
>
> > if the number of this kind of
> > large folios
> > is huge, the waste can be huge for some while.
> >
> > it is not a corner case for large folio swap-in. while someone writes
> > one subpage, I swap-in a large folio, wp_reuse will immediately
> > be called. This can cause waste quite often. One outcome of this
> > discussion is that I realize I should investigate this issue immediately
> > in the swap-in series as my off-tree code has optimized reuse but
> > mainline hasn't.
> >
> >>
> >> At least for 2 MiB THP, it might make sense to make that large folio
> >> available immediately again, even without memory pressure. Even
> >> compaction would not compact it.
> >
> > It is also true for 64KiB. as we want other processes to allocate
> > 64KiB successfully as much as possible, and reduce the rate of
> > falling back to small folios. by releasing 64KiB directly to buddy
> > rather than splitting and returning 15*4KiB in shrinker, we reduce
> > buddy fragmentation too.
> >
> >>
> >> --
> >> Cheers,
> >>
> >> David / dhildenb
Thanks
Barry
On 07.03.24 19:54, Barry Song wrote:
> On Fri, Mar 8, 2024 at 12:31 AM Ryan Roberts <[email protected]> wrote:
>>
>> On 07/03/2024 12:01, Barry Song wrote:
>>> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 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 ????
>>>>>>>
>>>>>>>>>>
>>>>>>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
>>>>>>>>>> a single PTE remains mapped (refcount == 0).
>>>>>>>>>
>>>>>>>>> ^ == 1
>>>>>>>
>>>>>>> seems this needs improvement. it is a waste the last subpage can
>>>>>>
>>>>>> My take that is WIP:
>>>>>>
>>>>>> https://lore.kernel.org/all/[email protected]/T/#u
>>>>>>
>>>>>>> reuse the whole large folio. i was doing it in a quite different way,
>>>>>>> if the large folio had only one subpage left, i would do copy and
>>>>>>> released the large folio[1]. and if i could reuse the whole large folio
>>>>>>> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
>>>>>>> we don't have this cont-pte luxury exposed to mm, so i guess we can
>>>>>>> not do [2] easily, but [1] seems to be an optimization.
>>>>>>
>>>>>> Yeah, I had essentially the same idea: just free up the large folio if most of
>>>>>> the stuff is unmapped. But that's rather a corner-case optimization, so I did
>>>>>> not proceed with that.
>>>>>>
>>>>>
>>>>> I'm not sure it's a corner case, really? - process forks, then both parent and
>>>>> child and write to all pages in what was previously a fully & contiguously
>>>>> mapped large folio?
>>>>
>>>> Well, with 2 MiB my assumption was that while it can happen, it's rather
>>>> rare. With smaller THP it might get more likely, agreed.
>>>>
>>>>>
>>>>> Reggardless, why is it an optimization to do the copy for the last subpage and
>>>>> syncrhonously free the large folio? It's already partially mapped so is on the
>>>>> deferred split list and can be split if memory is tight.
>>>
>>> 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.
Right, it's essentially a simplistic page migration at a point where you
know you can safely replace the page (PAE not set, so it cannot be
pinned using FOLL_PIN). No rmap walk, no migration entries, no worry
about additional page references.
--
Cheers,
David / dhildenb
On 07/03/2024 18:54, Barry Song wrote:
> On Fri, Mar 8, 2024 at 12:31 AM Ryan Roberts <[email protected]> wrote:
>>
>> On 07/03/2024 12:01, Barry Song wrote:
>>> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 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 ????
>>>>>>>
>>>>>>>>>>
>>>>>>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
>>>>>>>>>> a single PTE remains mapped (refcount == 0).
>>>>>>>>>
>>>>>>>>> ^ == 1
>>>>>>>
>>>>>>> seems this needs improvement. it is a waste the last subpage can
>>>>>>
>>>>>> My take that is WIP:
>>>>>>
>>>>>> https://lore.kernel.org/all/[email protected]/T/#u
>>>>>>
>>>>>>> reuse the whole large folio. i was doing it in a quite different way,
>>>>>>> if the large folio had only one subpage left, i would do copy and
>>>>>>> released the large folio[1]. and if i could reuse the whole large folio
>>>>>>> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
>>>>>>> we don't have this cont-pte luxury exposed to mm, so i guess we can
>>>>>>> not do [2] easily, but [1] seems to be an optimization.
>>>>>>
>>>>>> Yeah, I had essentially the same idea: just free up the large folio if most of
>>>>>> the stuff is unmapped. But that's rather a corner-case optimization, so I did
>>>>>> not proceed with that.
>>>>>>
>>>>>
>>>>> I'm not sure it's a corner case, really? - process forks, then both parent and
>>>>> child and write to all pages in what was previously a fully & contiguously
>>>>> mapped large folio?
>>>>
>>>> Well, with 2 MiB my assumption was that while it can happen, it's rather
>>>> rare. With smaller THP it might get more likely, agreed.
>>>>
>>>>>
>>>>> Reggardless, why is it an optimization to do the copy for the last subpage and
>>>>> syncrhonously free the large folio? It's already partially mapped so is on the
>>>>> deferred split list and can be split if memory is tight.
>>>
>>> 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...
>
>>
>>> reclamation will always cause latency and affect User
>>> experience. split_folio is not cheap :-)
>>
>> But neither is memcpy(4K) I'd imagine. But I get your point.
>
> In a real product scenario, we need to consider the success rate of
> allocating large folios.
> Currently, it's only 7%, as reported here[1], with no method to keep
> large folios intact in a
> buddy system.
Yes I saw that email - interesting. Is that 7% number for the Oppo
implementation or upstream implementation? (I think Oppo?). Do you know how the
other one compares (my guess is that upstream isn't complete enough yet to give
viable numbers)? And I'm guessing you are running on a kernel/fs that doesn't
support large folios in the page cache? What about large folio swap? My feeling
is that once we have all these features, that number should significantly
increase because you can create a system that essentially uses large quantities
of a couple of sizes of page (e.g. 4K & (16K | 64K)) and fragmentation will be
less of a problem. Perhaps that's wishful thinking though.
>
> Yu's TAO[2] chose to release the large folio entirely after copying
> the mapped parts
> onto smaller folios in vmscan,
Yes, TAO looks very interesting! It essentially partitions the memory IIUC?
>
> [1] https://lore.kernel.org/linux-mm/[email protected]/
> [2] https://lore.kernel.org/linux-mm/[email protected]/
>
>>
>>> if the number of this kind of
>>> large folios
>>> is huge, the waste can be huge for some while.
>>>
>>> it is not a corner case for large folio swap-in. while someone writes
>>> one subpage, I swap-in a large folio, wp_reuse will immediately
>>> be called. This can cause waste quite often. One outcome of this
>>> discussion is that I realize I should investigate this issue immediately
>>> in the swap-in series as my off-tree code has optimized reuse but
>>> mainline hasn't.
>>>
>>>>
>>>> At least for 2 MiB THP, it might make sense to make that large folio
>>>> available immediately again, even without memory pressure. Even
>>>> compaction would not compact it.
>>>
>>> It is also true for 64KiB. as we want other processes to allocate
>>> 64KiB successfully as much as possible, and reduce the rate of
>>> falling back to small folios. by releasing 64KiB directly to buddy
>>> rather than splitting and returning 15*4KiB in shrinker, we reduce
>>> buddy fragmentation too.
>>>
>>>>
>>>> --
>>>> Cheers,
>>>>
>>>> David / dhildenb
>
> Thanks
> Barry
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 <[email protected]> wrote:
>>>
>>> On 07/03/2024 12:01, Barry Song wrote:
>>>> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 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 ????
>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
>>>>>>>>>>> a single PTE remains mapped (refcount == 0).
>>>>>>>>>>
>>>>>>>>>> ^ == 1
>>>>>>>>
>>>>>>>> seems this needs improvement. it is a waste the last subpage can
>>>>>>>
>>>>>>> My take that is WIP:
>>>>>>>
>>>>>>> https://lore.kernel.org/all/[email protected]/T/#u
>>>>>>>
>>>>>>>> reuse the whole large folio. i was doing it in a quite different way,
>>>>>>>> if the large folio had only one subpage left, i would do copy and
>>>>>>>> released the large folio[1]. and if i could reuse the whole large folio
>>>>>>>> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
>>>>>>>> we don't have this cont-pte luxury exposed to mm, so i guess we can
>>>>>>>> not do [2] easily, but [1] seems to be an optimization.
>>>>>>>
>>>>>>> Yeah, I had essentially the same idea: just free up the large folio if most of
>>>>>>> the stuff is unmapped. But that's rather a corner-case optimization, so I did
>>>>>>> not proceed with that.
>>>>>>>
>>>>>>
>>>>>> I'm not sure it's a corner case, really? - process forks, then both parent and
>>>>>> child and write to all pages in what was previously a fully & contiguously
>>>>>> mapped large folio?
>>>>>
>>>>> Well, with 2 MiB my assumption was that while it can happen, it's rather
>>>>> rare. With smaller THP it might get more likely, agreed.
>>>>>
>>>>>>
>>>>>> Reggardless, why is it an optimization to do the copy for the last subpage and
>>>>>> syncrhonously free the large folio? It's already partially mapped so is on the
>>>>>> deferred split list and can be split if memory is tight.
>>>>
>>>> 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...
It sounds interesting, but and also like a lot of work.
--
Cheers,
David / dhildenb
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 <[email protected]> wrote:
>>>>
>>>> On 07/03/2024 12:01, Barry Song wrote:
>>>>> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[email protected]> 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 <[email protected]> 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
>>>>>>>>>>>>>> <[email protected]> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]>
>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hey Barry,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks for taking time to review!
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]>
>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang
>>>>>>>>>>>>>>>>>> <[email protected]> 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/[email protected]/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> 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 ????
>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio
>>>>>>>>>>>> only if
>>>>>>>>>>>> a single PTE remains mapped (refcount == 0).
>>>>>>>>>>>
>>>>>>>>>>> ^ == 1
>>>>>>>>>
>>>>>>>>> seems this needs improvement. it is a waste the last subpage can
>>>>>>>>
>>>>>>>> My take that is WIP:
>>>>>>>>
>>>>>>>> https://lore.kernel.org/all/[email protected]/T/#u
>>>>>>>>
>>>>>>>>> reuse the whole large folio. i was doing it in a quite different way,
>>>>>>>>> if the large folio had only one subpage left, i would do copy and
>>>>>>>>> released the large folio[1]. and if i could reuse the whole large folio
>>>>>>>>> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
>>>>>>>>> we don't have this cont-pte luxury exposed to mm, so i guess we can
>>>>>>>>> not do [2] easily, but [1] seems to be an optimization.
>>>>>>>>
>>>>>>>> Yeah, I had essentially the same idea: just free up the large folio if
>>>>>>>> most of
>>>>>>>> the stuff is unmapped. But that's rather a corner-case optimization, so
>>>>>>>> I did
>>>>>>>> not proceed with that.
>>>>>>>>
>>>>>>>
>>>>>>> I'm not sure it's a corner case, really? - process forks, then both
>>>>>>> parent and
>>>>>>> child and write to all pages in what was previously a fully & contiguously
>>>>>>> mapped large folio?
>>>>>>
>>>>>> Well, with 2 MiB my assumption was that while it can happen, it's rather
>>>>>> rare. With smaller THP it might get more likely, agreed.
>>>>>>
>>>>>>>
>>>>>>> Reggardless, why is it an optimization to do the copy for the last
>>>>>>> subpage and
>>>>>>> syncrhonously free the large folio? It's already partially mapped so is
>>>>>>> on the
>>>>>>> deferred split list and can be split if memory is tight.
>>>>>
>>>>> 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...
>
> It sounds interesting, but and also like a lot of work.
I have working patches. But I was originally doing it in the context of also
using the 16K (or 64K) page table structure for those processes. But that was
too hard because there are lots of edge cases (page cache, drivers, current CoW
impl, etc) where 4K mapping is needed. So abandoned. The user space ABI part was
the easy bit. I think Google have also mentioned something similar (at
plumbers?) that they were doing to allow emulation of 16K ABI on x86.
On Fri, Mar 8, 2024 at 9:05 PM Ryan Roberts <[email protected]> wrote:
>
> On 07/03/2024 18:54, Barry Song wrote:
> > On Fri, Mar 8, 2024 at 12:31 AM Ryan Roberts <[email protected]> wrote:
> >>
> >> On 07/03/2024 12:01, Barry Song wrote:
> >>> On Thu, Mar 7, 2024 at 7:45 PM David Hildenbrand <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On 07/03/2024 08:10, Barry Song wrote:
> >>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <[email protected]> wrote:
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Hey Barry,
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Thanks for taking time to review!
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> 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 ????
> >>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> wp_page_reuse() will currently reuse a PTE part of a large folio only if
> >>>>>>>>>> a single PTE remains mapped (refcount == 0).
> >>>>>>>>>
> >>>>>>>>> ^ == 1
> >>>>>>>
> >>>>>>> seems this needs improvement. it is a waste the last subpage can
> >>>>>>
> >>>>>> My take that is WIP:
> >>>>>>
> >>>>>> https://lore.kernel.org/all/[email protected]/T/#u
> >>>>>>
> >>>>>>> reuse the whole large folio. i was doing it in a quite different way,
> >>>>>>> if the large folio had only one subpage left, i would do copy and
> >>>>>>> released the large folio[1]. and if i could reuse the whole large folio
> >>>>>>> with CONT-PTE, i would reuse the whole large folio[2]. in mainline,
> >>>>>>> we don't have this cont-pte luxury exposed to mm, so i guess we can
> >>>>>>> not do [2] easily, but [1] seems to be an optimization.
> >>>>>>
> >>>>>> Yeah, I had essentially the same idea: just free up the large folio if most of
> >>>>>> the stuff is unmapped. But that's rather a corner-case optimization, so I did
> >>>>>> not proceed with that.
> >>>>>>
> >>>>>
> >>>>> I'm not sure it's a corner case, really? - process forks, then both parent and
> >>>>> child and write to all pages in what was previously a fully & contiguously
> >>>>> mapped large folio?
> >>>>
> >>>> Well, with 2 MiB my assumption was that while it can happen, it's rather
> >>>> rare. With smaller THP it might get more likely, agreed.
> >>>>
> >>>>>
> >>>>> Reggardless, why is it an optimization to do the copy for the last subpage and
> >>>>> syncrhonously free the large folio? It's already partially mapped so is on the
> >>>>> deferred split list and can be split if memory is tight.
> >>>
> >>> 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.
>
> >
> >>
> >>> reclamation will always cause latency and affect User
> >>> experience. split_folio is not cheap :-)
> >>
> >> But neither is memcpy(4K) I'd imagine. But I get your point.
> >
> > In a real product scenario, we need to consider the success rate of
> > allocating large folios.
> > Currently, it's only 7%, as reported here[1], with no method to keep
> > large folios intact in a
> > buddy system.
>
> Yes I saw that email - interesting. Is that 7% number for the Oppo
> implementation or upstream implementation? (I think Oppo?). Do you know how the
> other one compares (my guess is that upstream isn't complete enough yet to give
> viable numbers)? And I'm guessing you are running on a kernel/fs that doesn't
> support large folios in the page cache? What about large folio swap? My feeling
> is that once we have all these features, that number should significantly
> increase because you can create a system that essentially uses large quantities
> of a couple of sizes of page (e.g. 4K & (16K | 64K)) and fragmentation will be
> less of a problem. Perhaps that's wishful thinking though.
This is the number of OPPO's implementations which supports one kind of
large folio size - 64KiB only. Meanwhile, OPPO has a TAO-like
optimization by extending
migrate_type and marking some pageblocks dedicated for large folios(except some
corner cases , 3-order can also use them), it brings success rate to
around 50% in
do_anon_page and more than 90% in do_swap_page(we give this lower water as
we save large objects in zsmalloc/zram - compressing and decompressing 64KiB
as a whole instead of doing 16 * 4KiB).
The reported data is disabling the TAO-like optimization and just using buddy.
BTW, based on the previous observation, 16KiB allocation could still
be a problem
on phones, for example, kernel stacks allocation was a pain before it
was changed to
vmalloc.
>
> >
> > Yu's TAO[2] chose to release the large folio entirely after copying
> > the mapped parts
> > onto smaller folios in vmscan,
>
> Yes, TAO looks very interesting! It essentially partitions the memory IIUC?
kind of, adding two virtual zones to decrease compaction and keep
large folios intact/not being splitted.
>
> >
> > [1] https://lore.kernel.org/linux-mm/[email protected]/
> > [2] https://lore.kernel.org/linux-mm/[email protected]/
> >
> >>
> >>> if the number of this kind of
> >>> large folios
> >>> is huge, the waste can be huge for some while.
> >>>
> >>> it is not a corner case for large folio swap-in. while someone writes
> >>> one subpage, I swap-in a large folio, wp_reuse will immediately
> >>> be called. This can cause waste quite often. One outcome of this
> >>> discussion is that I realize I should investigate this issue immediately
> >>> in the swap-in series as my off-tree code has optimized reuse but
> >>> mainline hasn't.
> >>>
> >>>>
> >>>> At least for 2 MiB THP, it might make sense to make that large folio
> >>>> available immediately again, even without memory pressure. Even
> >>>> compaction would not compact it.
> >>>
> >>> It is also true for 64KiB. as we want other processes to allocate
> >>> 64KiB successfully as much as possible, and reduce the rate of
> >>> falling back to small folios. by releasing 64KiB directly to buddy
> >>> rather than splitting and returning 15*4KiB in shrinker, we reduce
> >>> buddy fragmentation too.
> >>>
> >>>>
> >>>> --
> >>>> Cheers,
> >>>>
> >>>> David / dhildenb
> >
Thanks
Barry
>
[...]
>>>>> 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.
On Mon, Mar 11, 2024 at 5:55 PM Ryan Roberts <[email protected]> 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
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 <[email protected]> wrote:
>>>
>>> Hey Barry,
>>>
>>> Thanks for taking time to review!
>>>
>>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
>>>>
>>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
>>>
>>> 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/[email protected]/
>
>>>
>>> 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
>
On Mon, Mar 11, 2024 at 11:07 PM Ryan Roberts <[email protected]> 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 <[email protected]> wrote:
> >>>
> >>> Hey Barry,
> >>>
> >>> Thanks for taking time to review!
> >>>
> >>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <[email protected]> wrote:
> >>>>
> >>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <[email protected]> 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/[email protected]/
> >>>
> >>> 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/[email protected]/
>
>
> >
> >>>
> >>> 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
> >
>