Received-SPF: pass (google.com: domain of linux-kernel+bounces-203501-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <CAGsJ_4wFyFBTDDfsBpi0sKs1WOH2jaKKoYWWj9Ln_PsNjP2uuA@mail.gmail.com>
 <20240605095406.891512-1-ranxiaokai627@163.com> <D667F08C-0CCE-4D5E-89A3-56674B0893DE@nvidia.com>
 <c110eb46-3c9d-40c3-ab16-5bd9f75b6501@redhat.com>
In-Reply-To: <c110eb46-3c9d-40c3-ab16-5bd9f75b6501@redhat.com>
From: Barry Song <21cnbao@gmail.com>
Date: Thu, 6 Jun 2024 13:34:17 +1200
Message-ID: <CAGsJ_4yBqBTLKBNME1vUTqz6XMdngVSp3V5RP7HqiQkLU-NWtA@mail.gmail.com>
Subject: Re: [PATCH linux-next] mm: huge_memory: fix misused
 mapping_large_folio_support() for anon folios
To: David Hildenbrand <david@redhat.com>
Cc: Zi Yan <ziy@nvidia.com>, ran xiaokai <ranxiaokai627@163.com>, akpm@linux-foundation.org, 
	linux-kernel@vger.kernel.org, linux-mm@kvack.org, mhocko@kernel.org, 
	v-songbaohua@oppo.com, xu.xin16@zte.com.cn, yang.yang29@zte.com.cn
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jun 6, 2024 at 2:42=E2=80=AFAM David Hildenbrand <david@redhat.com>=
 wrote:
>
> On 05.06.24 16:08, Zi Yan wrote:
> > On 5 Jun 2024, at 2:54, ran xiaokai wrote:
> >
> >>> On Tue, Jun 4, 2024 at 5:47?PM <xu.xin16@zte.com.cn> wrote:
> >>>>
> >>>> From: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> >>>>
> >>>> When I did a large folios split test, a WARNING
> >>>> "[ 5059.122759][  T166] Cannot split file folio to non-0 order"
> >>>> was triggered. But my test cases are only for anonmous folios.
> >>>> while mapping_large_folio_support() is only reasonable for page
> >>>> cache folios.
> >>>>
> >>>> In split_huge_page_to_list_to_order(), the folio passed to
> >>>> mapping_large_folio_support() maybe anonmous folio. The
> >>>> folio_test_anon() check is missing. So the split of the anonmous THP
> >>>> is failed. This is also the same for shmem_mapping(). We'd better ad=
d
> >>>> a check for both. But the shmem_mapping() in __split_huge_page() is
> >>>> not involved, as for anonmous folios, the end parameter is set to -1=
, so
> >>>> (head[i].index >=3D end) is always false. shmem_mapping() is not cal=
led.
> >>>>
> >>>> Using /sys/kernel/debug/split_huge_pages to verify this, with this
> >>>> patch, large anon THP is successfully split and the warning is cease=
d.
> >>>>
> >>>> Signed-off-by: Ran Xiaokai <ran.xiaokai@zte.com.cn>
> >>>> Cc: xu xin <xu.xin16@zte.com.cn>
> >>>> Cc: Yang Yang <yang.yang29@zte.com.cn>
> >>>> ---
> >>>>   mm/huge_memory.c | 38 ++++++++++++++++++++------------------
> >>>>   1 file changed, 20 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >>>> index 317de2afd371..4c9c7e5ea20c 100644
> >>>> --- a/mm/huge_memory.c
> >>>> +++ b/mm/huge_memory.c
> >>>> @@ -3009,31 +3009,33 @@ int split_huge_page_to_list_to_order(struct =
page *page, struct list_head *list,
> >>>>          if (new_order >=3D folio_order(folio))
> >>>>                  return -EINVAL;
> >>>>
> >>>> -       /* Cannot split anonymous THP to order-1 */
> >>>> -       if (new_order =3D=3D 1 && folio_test_anon(folio)) {
> >>>> -               VM_WARN_ONCE(1, "Cannot split to order-1 folio");
> >>>> -               return -EINVAL;
> >>>> -       }
> >>>> -
> >>>>          if (new_order) {
> >>>>                  /* Only swapping a whole PMD-mapped folio is suppor=
ted */
> >>>>                  if (folio_test_swapcache(folio))
> >>>>                          return -EINVAL;
> >>>> -               /* Split shmem folio to non-zero order not supported=
 */
> >>>> -               if (shmem_mapping(folio->mapping)) {
> >>>> -                       VM_WARN_ONCE(1,
> >>>> -                               "Cannot split shmem folio to non-0 o=
rder");
> >>>> -                       return -EINVAL;
> >>>> -               }
> >>>> -               /* No split if the file system does not support larg=
e folio */
> >>>> -               if (!mapping_large_folio_support(folio->mapping)) {
> >>>> -                       VM_WARN_ONCE(1,
> >>>> -                               "Cannot split file folio to non-0 or=
der");
> >>>> -                       return -EINVAL;
> >>>> +
> >>>> +               if (folio_test_anon(folio)) {
> >>>> +                       /* Cannot split anonymous THP to order-1 */
> >>>> +                       if (new_order =3D=3D 1) {
> >>>> +                               VM_WARN_ONCE(1, "Cannot split to ord=
er-1 folio");
> >>>> +                               return -EINVAL;
> >>>> +                       }
> >>>> +               } else {
> >>>> +                       /* Split shmem folio to non-zero order not s=
upported */
> >>>> +                       if (shmem_mapping(folio->mapping)) {
> >>>> +                               VM_WARN_ONCE(1,
> >>>> +                                       "Cannot split shmem folio to=
 non-0 order");
> >>>> +                               return -EINVAL;
> >>>> +                       }
> >>>> +                       /* No split if the file system does not supp=
ort large folio */
> >>>> +                       if (!mapping_large_folio_support(folio->mapp=
ing)) {
> >>>> +                               VM_WARN_ONCE(1,
> >>>> +                                       "Cannot split file folio to =
non-0 order");
> >>>> +                               return -EINVAL;
> >>>> +                       }
> >>>
> >>> Am I missing something? if file system doesn't support large folio,
> >>> how could the large folio start to exist from the first place while i=
ts
> >>> mapping points to a file which doesn't support large folio?
> >>
> >> I think it is the CONFIG_READ_ONLY_THP_FOR_FS case.
> >> khugepaged will try to collapse read-only file-backed pages to 2M THP.
> >
> > Can you add this information to the commit log in your next version?
>
> Can we also add that as a comment to that function, like "Note that we
> might still
> have THPs in such mappings due to CONFIG_READ_ONLY_THP_FOR_FS. But in
> that case,
> the mapping does not actually support large folios properly.
> "Or sth. like that.

+1. Otherwise, the code appears quite confusing.
Would using "#ifdef" help to further clarify it?

#ifdef CONFIG_READ_ONLY_THP_FOR_FS
            if (!mapping_large_folio_support(folio->mapping)) {
                          ....
            }
#endif

>
> --
> Cheers,
>
> David / dhildenb
>

Thanks
Barry