Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20231119194740.94101-1-ryncsn@gmail.com> <20231119194740.94101-22-ryncsn@gmail.com>
In-Reply-To: <20231119194740.94101-22-ryncsn@gmail.com>
From:   Chris Li <chrisl@kernel.org>
Date:   Tue, 21 Nov 2023 21:15:22 -0800
Message-ID: <CAF8kJuN5GELAdh5ZJHthtgJCaYkgQ38BmzLZerUp6fS+pVU=WQ@mail.gmail.com>
Subject: Re: [PATCH 21/24] swap: make swapin_readahead result checking
 argument mandatory
To:     Kairui Song <kasong@tencent.com>
Cc:     linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
        "Huang, Ying" <ying.huang@intel.com>,
        David Hildenbrand <david@redhat.com>,
        Hugh Dickins <hughd@google.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Matthew Wilcox <willy@infradead.org>,
        Michal Hocko <mhocko@suse.com>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Nov 19, 2023 at 11:49=E2=80=AFAM Kairui Song <ryncsn@gmail.com> wro=
te:
>
> From: Kairui Song <kasong@tencent.com>
>
> This is only one caller now in page fault path, make the result return
> argument mandatory.
>
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>  mm/swap_state.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 6f39aa8394f1..0433a2586c6d 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -913,7 +913,6 @@ static struct page *swapin_no_readahead(swp_entry_t e=
ntry, gfp_t gfp_mask,
>  struct page *swapin_readahead(swp_entry_t entry, gfp_t gfp_mask,
>                               struct vm_fault *vmf, enum swap_cache_resul=
t *result)
>  {
> -       enum swap_cache_result cache_result;
>         struct swap_info_struct *si;
>         struct mempolicy *mpol;
>         void *shadow =3D NULL;
> @@ -928,29 +927,27 @@ struct page *swapin_readahead(swp_entry_t entry, gf=
p_t gfp_mask,
>
>         folio =3D swap_cache_get_folio(entry, vmf, &shadow);
>         if (folio) {
> +               *result =3D SWAP_CACHE_HIT;
>                 page =3D folio_file_page(folio, swp_offset(entry));
> -               cache_result =3D SWAP_CACHE_HIT;
>                 goto done;
>         }
>
>         mpol =3D get_vma_policy(vmf->vma, vmf->address, 0, &ilx);
>         if (swap_use_no_readahead(si, swp_offset(entry))) {
> +               *result =3D SWAP_CACHE_BYPASS;

Each of this "*result" will compile into memory store instructions.
The compiler most likely can't optimize and combine them together
because the store can cause segfault from the compiler's point of
view. The multiple local variable assignment can be compiled into a
few registers assignment so it does not cost as much as multiple
memory stores.

>                 page =3D swapin_no_readahead(entry, gfp_mask, mpol, ilx, =
vmf->vma->vm_mm);
> -               cache_result =3D SWAP_CACHE_BYPASS;
>                 if (shadow)
>                         workingset_refault(page_folio(page), shadow);
> -       } else if (swap_use_vma_readahead(si)) {
> -               page =3D swap_vma_readahead(entry, gfp_mask, mpol, ilx, v=
mf);
> -               cache_result =3D SWAP_CACHE_MISS;
>         } else {
> -               page =3D swap_cluster_readahead(entry, gfp_mask, mpol, il=
x);
> -               cache_result =3D SWAP_CACHE_MISS;
> +               *result =3D SWAP_CACHE_MISS;
> +               if (swap_use_vma_readahead(si))
> +                       page =3D swap_vma_readahead(entry, gfp_mask, mpol=
, ilx, vmf);
> +               else
> +                       page =3D swap_cluster_readahead(entry, gfp_mask, =
mpol, ilx);

I recall you introduce or heavy modify this function in previous patch befo=
re.
Consider combine some of the patch and present the final version sooner.
From the reviewing point of view, don't need to review so many
internal version which get over written any way.

>         }
>         mpol_cond_put(mpol);
>  done:
>         put_swap_device(si);
> -       if (result)
> -               *result =3D cache_result;

The original version with check and assign it at one place is better.
Safer and produce better code.

Chris