Received-SPF: pass (google.com: domain of linux-kernel+bounces-41465-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20231025144546.577640-1-ryan.roberts@arm.com> <20240118111036.72641-1-21cnbao@gmail.com>
 <20240118111036.72641-6-21cnbao@gmail.com>
In-Reply-To: <20240118111036.72641-6-21cnbao@gmail.com>
From: Chris Li <chrisl@kernel.org>
Date: Sat, 27 Jan 2024 15:41:50 -0800
Message-ID: <CAF8kJuN0RkubcLpj-NT8Crycf2-TciM2yGhUDBNpzBji6TQ3rg@mail.gmail.com>
Subject: Re: [PATCH RFC 5/6] mm: rmap: weaken the WARN_ON in __folio_add_anon_rmap()
To: Barry Song <21cnbao@gmail.com>
Cc: ryan.roberts@arm.com, akpm@linux-foundation.org, david@redhat.com, 
	linux-mm@kvack.org, linux-kernel@vger.kernel.org, mhocko@suse.com, 
	shy828301@gmail.com, wangkefeng.wang@huawei.com, willy@infradead.org, 
	xiang@kernel.org, ying.huang@intel.com, yuzhao@google.com, surenb@google.com, 
	steven.price@arm.com, Barry Song <v-songbaohua@oppo.com>, 
	Chuanhua Han <hanchuanhua@oppo.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 18, 2024 at 3:12=E2=80=AFAM Barry Song <21cnbao@gmail.com> wrot=
e:
>
> From: Barry Song <v-songbaohua@oppo.com>
>
> In do_swap_page(), while supporting large folio swap-in, we are using the=
 helper
> folio_add_anon_rmap_ptes. This is triggerring a WARN_ON in __folio_add_an=
on_rmap.
> We can make the warning quiet by two ways
> 1. in do_swap_page, we call folio_add_new_anon_rmap() if we are sure the =
large
> folio is new allocated one; we call folio_add_anon_rmap_ptes() if we find=
 the
> large folio in swapcache.
> 2. we always call folio_add_anon_rmap_ptes() in do_swap_page but weaken t=
he
> WARN_ON in __folio_add_anon_rmap() by letting the WARN_ON less sensitive.
>
> Option 2 seems to be better for do_swap_page() as it can use unified code=
 for
> all cases.
>
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> Tested-by: Chuanhua Han <hanchuanhua@oppo.com>
> ---
>  mm/rmap.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index f5d43edad529..469fcfd32317 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1304,7 +1304,10 @@ static __always_inline void __folio_add_anon_rmap(=
struct folio *folio,
>                  * page.
>                  */
>                 VM_WARN_ON_FOLIO(folio_test_large(folio) &&
> -                                level !=3D RMAP_LEVEL_PMD, folio);
> +                                level !=3D RMAP_LEVEL_PMD &&
> +                                (!IS_ALIGNED(address, nr_pages * PAGE_SI=
ZE) ||
Some minor nitpick here.
There are two leading "(" in this and next line. This is the first "("
> +                                (folio_test_swapcache(folio) && !IS_ALIG=
NED(folio->index, nr_pages)) ||
 Second "("  here.

These two "(" are NOT at the same nested level. They should not have
the same indentation.
On my first glance, I misread the scope of the "||" due to the same
level indentation.
We can do one of the two
1) add more indentation on the second "(" to reflect the nesting level.

> +                                page !=3D &folio->page), folio);

Also moving the folio to the next line, because the multiline
expression is huge and complex. Make it obvious the ending "folio" is
not part of the testing condition.

2) Move the multiline test condition to a checking function. Inside
the function it can return early when the shortcut condition is met.
That will also help the readability of this warning condition.

Chris

>                 __folio_set_anon(folio, vma, address,
>                                  !!(flags & RMAP_EXCLUSIVE));
>         } else if (likely(!folio_test_ksm(folio))) {
> --
> 2.34.1
>
>