linux-kernel - Re: [PATCH 13/24] swap: simplify swap_cache_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAF8kJuMe3K1bUCWFSWB7c2tw6KKWr49MJMfu2q-ET5tM6vWXXA@mail.gmail.com>
Date:   Tue, 21 Nov 2023 08:50:35 -0800
From:   Chris Li <chrisl@...nel.org>
To:     Kairui Song <kasong@...cent.com>
Cc:     linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        "Huang, Ying" <ying.huang@...el.com>,
        David Hildenbrand <david@...hat.com>,
        Hugh Dickins <hughd@...gle.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Matthew Wilcox <willy@...radead.org>,
        Michal Hocko <mhocko@...e.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 13/24] swap: simplify swap_cache_get_folio

Hi Kairui,

I agree the resulting code is marginally better.

However, this change does not bring enough value to justify a stand alone patch.
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 AM Kairui Song <ryncsn@...il.com> wrote:
>
> From: Kairui Song <kasong@...cent.com>
>
> Rearrange the if statement, reduce the code indent, no feature change.
>
> Signed-off-by: Kairui Song <kasong@...cent.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 swap_info_struct *si)
>   */
>  struct folio *swap_cache_get_folio(swp_entry_t entry, struct vm_fault *vmf)
>  {
> +       bool vma_ra, readahead;
>         struct folio *folio;
>
>         folio = filemap_get_folio(swap_address_space(entry), swp_offset(entry));
> -       if (!IS_ERR(folio)) {
> -               bool vma_ra = swap_use_vma_readahead(swp_swap_info(entry));
> -               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 = folio_test_clear_readahead(folio);
> -               if (vmf && vma_ra) {
> -                       unsigned long ra_val;
> -                       int win, hits;
> -
> -                       ra_val = GET_SWAP_RA_VAL(vmf->vma);
> -                       win = SWAP_RA_WIN(ra_val);
> -                       hits = SWAP_RA_HITS(ra_val);
> -                       if (readahead)
> -                               hits = min_t(int, hits + 1, SWAP_RA_HITS_MAX);
> -                       atomic_long_set(&vmf->vma->swap_readahead_info,
> -                                       SWAP_RA_VAL(vmf->address, win, hits));
> -               }
> +       /*
> +        * 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 = NULL;
> +       vma_ra = swap_use_vma_readahead(swp_swap_info(entry));
> +       readahead = folio_test_clear_readahead(folio);
> +       if (vmf && vma_ra) {
> +               unsigned long ra_val;
> +               int win, hits;
> +
> +               ra_val = GET_SWAP_RA_VAL(vmf->vma);
> +               win = SWAP_RA_WIN(ra_val);
> +               hits = SWAP_RA_HITS(ra_val);
> +               if (readahead)
> +                       hits = 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
>
>