Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220303222014.517033-1-shy828301@gmail.com> <CADFyXm6W9CVkO4XPYep-tHg55c8m8NES783kcVYrdjSMbzYoDA@mail.gmail.com>
In-Reply-To: <CADFyXm6W9CVkO4XPYep-tHg55c8m8NES783kcVYrdjSMbzYoDA@mail.gmail.com>
From:   Yang Shi <shy828301@gmail.com>
Date:   Fri, 4 Mar 2022 10:30:35 -0800
Message-ID: <CAHbLzkriyBy2HqjssurLSnhoyuUzpJRZjMPNx34MTgxeO0dddg@mail.gmail.com>
Subject: Re: [PATCH] mm: thp: don't have to lock page anymore when splitting PMD
To:     David Hildenbrand <david@redhat.com>
Cc:     Andrea Arcangeli <aarcange@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Hugh Dickins <hughd@google.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux MM <linux-mm@kvack.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Mar 3, 2022 at 9:06 PM David Hildenbrand <david@redhat.com> wrote:
>
> Hi,
>
> This probably bounces on the list due to html junk from the gmail app.
>
> What happened to
>
> https://lore.kernel.org/linux-mm/20220131162940.210846-10-david@redhat.co=
m/
>
> Included in the very series mentioned below?
>
> Was this silently dropped due to folio conversion collisions? :/

I really didn't notice you already proposed this. Maybe folio
conversion, maybe mlock cleanup, I can't tell. But anyway this patch
needs to get rebased. I will submit v2 to solve the comment, will add
your signed-off-by.

>
> Yang Shi <shy828301@gmail.com> schrieb am Do. 3. M=C3=A4rz 2022 um 23:20:
>>
>> The commit c444eb564fb1 ("mm: thp: make the THP mapcount atomic against
>> __split_huge_pmd_locked()") locked the page for PMD split to make
>> mapcount stable for reuse_swap_page(), then commit 1c2f67308af4 ("mm:
>> thp: fix MADV_REMOVE deadlock on shmem THP") reduce the scope to
>> anonymous page only.
>>
>> However COW has not used mapcount to determine if the page is shared or
>> not anymore due to the COW fixes [1] from David Hildenbrand and the
>> reuse_swap_page() was removed as well.  So PMD split doesn't have to
>> lock the page anymore.  This patch basically reverted the above two
>> commits.
>>
>> [1] https://lore.kernel.org/linux-mm/20220131162940.210846-1-david@redha=
t.com/
>>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>
>> Signed-off-by: Yang Shi <shy828301@gmail.com>
>> ---
>>  mm/huge_memory.c | 44 +++++---------------------------------------
>>  1 file changed, 5 insertions(+), 39 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index b49e1a11df2e..daaa698bd273 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -2134,8 +2134,6 @@ void __split_huge_pmd(struct vm_area_struct *vma, =
pmd_t *pmd,
>>  {
>>         spinlock_t *ptl;
>>         struct mmu_notifier_range range;
>> -       bool do_unlock_folio =3D false;
>> -       pmd_t _pmd;
>>
>>         mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->v=
m_mm,
>>                                 address & HPAGE_PMD_MASK,
>> @@ -2148,48 +2146,16 @@ void __split_huge_pmd(struct vm_area_struct *vma=
, pmd_t *pmd,
>>          * pmd against. Otherwise we can end up replacing wrong folio.
>>          */
>>         VM_BUG_ON(freeze && !folio);
>> -       if (folio) {
>> -               VM_WARN_ON_ONCE(!folio_test_locked(folio));
>> -               if (folio !=3D page_folio(pmd_page(*pmd)))
>> -                       goto out;
>> -       }
>> +       if (folio && folio !=3D page_folio(pmd_page(*pmd)))
>> +               goto out;
>>
>> -repeat:
>> -       if (pmd_trans_huge(*pmd)) {
>> -               if (!folio) {
>> -                       folio =3D page_folio(pmd_page(*pmd));
>> -                       /*
>> -                        * An anonymous page must be locked, to ensure t=
hat a
>> -                        * concurrent reuse_swap_page() sees stable mapc=
ount;
>> -                        * but reuse_swap_page() is not used on shmem or=
 file,
>> -                        * and page lock must not be taken when zap_pmd_=
range()
>> -                        * calls __split_huge_pmd() while i_mmap_lock is=
 held.
>> -                        */
>> -                       if (folio_test_anon(folio)) {
>> -                               if (unlikely(!folio_trylock(folio))) {
>> -                                       folio_get(folio);
>> -                                       _pmd =3D *pmd;
>> -                                       spin_unlock(ptl);
>> -                                       folio_lock(folio);
>> -                                       spin_lock(ptl);
>> -                                       if (unlikely(!pmd_same(*pmd, _pm=
d))) {
>> -                                               folio_unlock(folio);
>> -                                               folio_put(folio);
>> -                                               folio =3D NULL;
>> -                                               goto repeat;
>> -                                       }
>> -                                       folio_put(folio);
>> -                               }
>> -                               do_unlock_folio =3D true;
>> -                       }
>> -               }
>> -       } else if (!(pmd_devmap(*pmd) || is_pmd_migration_entry(*pmd)))
>> +       if (!(pmd_devmap(*pmd) || is_pmd_migration_entry(*pmd)))
>>                 goto out;
>> +
>>         __split_huge_pmd_locked(vma, pmd, range.start, freeze);
>>  out:
>>         spin_unlock(ptl);
>> -       if (do_unlock_folio)
>> -               folio_unlock(folio);
>> +
>>         /*
>>          * No need to double call mmu_notifier->invalidate_range() callb=
ack.
>>          * They are 3 cases to consider inside __split_huge_pmd_locked()=
:
>> --
>> 2.26.3
>>