Received-SPF: pass (google.com: domain of linux-kernel+bounces-212662-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Lance Yang <ioworker0@gmail.com>
To: baolin.wang@linux.alibaba.com,
	david@redhat.com
Cc: ioworker0@gmail.com,
	21cnbao@gmail.com,
	akpm@linux-foundation.org,
	fengwei.yin@intel.com,
	libang.li@antgroup.com,
	linux-kernel@vger.kernel.org,
	linux-mm@kvack.org,
	maskray@google.com,
	mhocko@suse.com,
	minchan@kernel.org,
	peterx@redhat.com,
	ryan.roberts@arm.com,
	shy828301@gmail.com,
	sj@kernel.org,
	songmuchun@bytedance.com,
	wangkefeng.wang@huawei.com,
	willy@infradead.org,
	xiehuan09@gmail.com,
	ziy@nvidia.com,
	zokeefe@google.com
Subject: Re: [PATCH v7 4/4] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()
Date: Thu, 13 Jun 2024 14:55:21 +0800
Message-Id: <20240613065521.15960-1-ioworker0@gmail.com>
In-Reply-To: <20240610120809.66601-1-ioworker0@gmail.com>
References: <20240610120809.66601-1-ioworker0@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On Mon, Jun 10, 2024 at 8:08 PM Lance Yang <ioworker0@gmail.com> wrote:
>
[...]
> +static bool __discard_anon_folio_pmd_locked(struct vm_area_struct *vma,
> +                                           unsigned long addr, pmd_t *pmdp,
> +                                           struct folio *folio)
> +{
> +       VM_WARN_ON_FOLIO(folio_test_swapbacked(folio), folio);
> +       VM_WARN_ON_FOLIO(!folio_test_anon(folio), folio);
> +
> +       struct mm_struct *mm = vma->vm_mm;
> +       int ref_count, map_count;
> +       pmd_t orig_pmd = *pmdp;
> +       struct page *page;
> +
> +       if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
> +               return false;
> +
> +       page = pmd_page(orig_pmd);
> +       if (unlikely(page_folio(page) != folio))
> +               return false;
> +
> +       if (folio_test_dirty(folio) || pmd_dirty(orig_pmd)) {
> +               folio_set_swapbacked(folio);
> +               return false;
> +       }
> +
> +       orig_pmd = pmdp_huge_clear_flush(vma, addr, pmdp);
> +
> +       /*
> +        * Syncing against concurrent GUP-fast:
> +        * - clear PMD; barrier; read refcount
> +        * - inc refcount; barrier; read PMD
> +        */
> +       smp_mb();
> +
> +       ref_count = folio_ref_count(folio);
> +       map_count = folio_mapcount(folio);
> +
> +       /*
> +        * Order reads for folio refcount and dirty flag
> +        * (see comments in __remove_mapping()).
> +        */
> +       smp_rmb();
> +
> +       /*
> +        * If the folio or its PMD is redirtied at this point, or if there
> +        * are unexpected references, we will give up to discard this folio
> +        * and remap it.
> +        *
> +        * The only folio refs must be one from isolation plus the rmap(s).
> +        */
> +       if (folio_test_dirty(folio) || pmd_dirty(orig_pmd))
> +               folio_set_swapbacked(folio);
> +
> +       if (folio_test_swapbacked(folio) || ref_count != map_count + 1) {
> +               set_pmd_at(mm, addr, pmdp, orig_pmd);
> +               return false;
> +       }
> +
> +       folio_remove_rmap_pmd(folio, page, vma);
> +       zap_deposited_table(mm, pmdp);
> +       add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> +       if (vma->vm_flags & VM_LOCKED)
> +               mlock_drain_local();
> +       folio_put(folio);
> +
> +       return true;
> +}
[...]
> diff --git a/mm/rmap.c b/mm/rmap.c
> index b77f88695588..8e901636ade9 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1630,6 +1630,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>         enum ttu_flags flags = (enum ttu_flags)(long)arg;
>         unsigned long pfn;
>         unsigned long hsz = 0;
> +       bool pmd_mapped = false;
>
>         /*
>          * When racing against e.g. zap_pte_range() on another cpu,
> @@ -1676,16 +1677,24 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>                         goto walk_done_err;
>                 }
>
> -               if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> -                       /*
> -                        * We temporarily have to drop the PTL and start once
> -                        * again from that now-PTE-mapped page table.
> -                        */
> -                       split_huge_pmd_locked(vma, pvmw.address, pvmw.pmd,
> -                                             false, folio);
> -                       flags &= ~TTU_SPLIT_HUGE_PMD;
> -                       page_vma_mapped_walk_restart(&pvmw);
> -                       continue;
> +               if (!pvmw.pte) {
> +                       pmd_mapped = true;
> +                       if (unmap_huge_pmd_locked(vma, pvmw.address, pvmw.pmd,
> +                                                 folio))
> +                               goto walk_done;
> +
> +                       if (flags & TTU_SPLIT_HUGE_PMD) {
> +                               /*
> +                                * We temporarily have to drop the PTL and start
> +                                * once again from that now-PTE-mapped page
> +                                * table.
> +                                */
> +                               split_huge_pmd_locked(vma, pvmw.address,
> +                                                     pvmw.pmd, false, folio);
> +                               flags &= ~TTU_SPLIT_HUGE_PMD;
> +                               page_vma_mapped_walk_restart(&pvmw);
> +                               continue;
> +                       }
>                 }
>
>                 /* Unexpected PMD-mapped THP? */
> @@ -1813,7 +1822,12 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>                          */
>                         if (unlikely(folio_test_swapbacked(folio) !=
>                                         folio_test_swapcache(folio))) {
> -                               WARN_ON_ONCE(1);
> +                               /*
> +                                * unmap_huge_pmd_locked() will unmark a
> +                                * PMD-mapped folio as lazyfree if the folio or
> +                                * its PMD was redirtied.
> +                                */
> +                               WARN_ON_ONCE(!pmd_mapped);

Damn it. I forgot to remap the folio to the page table since we've cleared
the PTE here.

But it seems like we neither need to do that, nor unmark a PMD-mapped folio
as lazyfree in unmap_huge_pmd_locked(); the next block[1] will catch the
folio and do the same thing.

Hi David and Baolin, what do you think?

>                                 goto walk_done_err;
>                         }

[1] https://elixir.bootlin.com/linux/v6.10-rc3/source/mm/rmap.c#L1820

/* MADV_FREE page check */
if (!folio_test_swapbacked(folio)) {
	int ref_count, map_count;

[...]
	/*
	 * The only page refs must be one from isolation
	 * plus the rmap(s) (dropped by discard:).
	 */
	if (ref_count == 1 + map_count &&
	    !folio_test_dirty(folio)) {
		dec_mm_counter(mm, MM_ANONPAGES);
		goto discard;
	}

	/*
	 * If the folio was redirtied, it cannot be
	 * discarded. Remap the page to page table.
	 */
	set_pte_at(mm, address, pvmw.pte, pteval);
	folio_set_swapbacked(folio);
	goto walk_done_err;
}


Thanks,
Lance

>
> --
> 2.33.1
>