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-14-ryncsn@gmail.com>
In-Reply-To: <20231119194740.94101-14-ryncsn@gmail.com>
From:   Chris Li <chrisl@kernel.org>
Date:   Tue, 21 Nov 2023 08:50:35 -0800
Message-ID: <CAF8kJuMe3K1bUCWFSWB7c2tw6KKWr49MJMfu2q-ET5tM6vWXXA@mail.gmail.com>
Subject: Re: [PATCH 13/24] swap: simplify swap_cache_get_folio
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

Hi Kairui,

I agree the resulting code is marginally better.

However, this change does not bring enough value to justify a stand alone p=
atch.
It has no impact on the resulting kernel.

It would be much better if the code was checkin like this, or if you
are modifying this function, rewrite it better. In my opinion, doing
very trivial code shuffling for the sake of cleaning up is not
justifiable for the value it brings.
For one it will make the git blame less obvious who actually changed
that code for what reason. I am against trivial code shuffling.

Chris

On Sun, Nov 19, 2023 at 11:48=E2=80=AFAM Kairui Song <ryncsn@gmail.com> wro=
te:
>
> From: Kairui Song <kasong@tencent.com>
>
> Rearrange the if statement, reduce the code indent, no feature change.
>
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>  mm/swap_state.c | 58 ++++++++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 30 deletions(-)
>
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 91461e26a8cc..3b5a34f47192 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -336,41 +336,39 @@ static inline bool swap_use_vma_readahead(struct sw=
ap_info_struct *si)
>   */
>  struct folio *swap_cache_get_folio(swp_entry_t entry, struct vm_fault *v=
mf)
>  {
> +       bool vma_ra, readahead;
>         struct folio *folio;
>
>         folio =3D filemap_get_folio(swap_address_space(entry), swp_offset=
(entry));
> -       if (!IS_ERR(folio)) {
> -               bool vma_ra =3D swap_use_vma_readahead(swp_swap_info(entr=
y));
> -               bool readahead;
> +       if (IS_ERR(folio))
> +               return NULL;
>
> -               /*
> -                * At the moment, we don't support PG_readahead for anon =
THP
> -                * so let's bail out rather than confusing the readahead =
stat.
> -                */
> -               if (unlikely(folio_test_large(folio)))
> -                       return folio;
> -
> -               readahead =3D folio_test_clear_readahead(folio);
> -               if (vmf && vma_ra) {
> -                       unsigned long ra_val;
> -                       int win, hits;
> -
> -                       ra_val =3D GET_SWAP_RA_VAL(vmf->vma);
> -                       win =3D SWAP_RA_WIN(ra_val);
> -                       hits =3D SWAP_RA_HITS(ra_val);
> -                       if (readahead)
> -                               hits =3D min_t(int, hits + 1, SWAP_RA_HIT=
S_MAX);
> -                       atomic_long_set(&vmf->vma->swap_readahead_info,
> -                                       SWAP_RA_VAL(vmf->address, win, hi=
ts));
> -               }
> +       /*
> +        * At the moment, we don't support PG_readahead for anon THP
> +        * so let's bail out rather than confusing the readahead stat.
> +        */
> +       if (unlikely(folio_test_large(folio)))
> +               return folio;
>
> -               if (readahead) {
> -                       count_vm_event(SWAP_RA_HIT);
> -                       if (!vmf || !vma_ra)
> -                               atomic_inc(&swapin_readahead_hits);
> -               }
> -       } else {
> -               folio =3D NULL;
> +       vma_ra =3D swap_use_vma_readahead(swp_swap_info(entry));
> +       readahead =3D folio_test_clear_readahead(folio);
> +       if (vmf && vma_ra) {
> +               unsigned long ra_val;
> +               int win, hits;
> +
> +               ra_val =3D GET_SWAP_RA_VAL(vmf->vma);
> +               win =3D SWAP_RA_WIN(ra_val);
> +               hits =3D SWAP_RA_HITS(ra_val);
> +               if (readahead)
> +                       hits =3D min_t(int, hits + 1, SWAP_RA_HITS_MAX);
> +               atomic_long_set(&vmf->vma->swap_readahead_info,
> +                               SWAP_RA_VAL(vmf->address, win, hits));
> +       }
> +
> +       if (readahead) {
> +               count_vm_event(SWAP_RA_HIT);
> +               if (!vmf || !vma_ra)
> +                       atomic_inc(&swapin_readahead_hits);
>         }
>
>         return folio;
> --
> 2.42.0
>
>