linux-kernel - Re: [PATCH 21/24] swap: make swapin_readahead result checking argument mandatory

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: <CAF8kJuN5GELAdh5ZJHthtgJCaYkgQ38BmzLZerUp6fS+pVU=WQ@mail.gmail.com>
Date:   Tue, 21 Nov 2023 21:15:22 -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 21/24] swap: make swapin_readahead result checking
 argument mandatory

On Sun, Nov 19, 2023 at 11:49 AM Kairui Song <ryncsn@...il.com> wrote:
>
> From: Kairui Song <kasong@...cent.com>
>
> This is only one caller now in page fault path, make the result return
> argument mandatory.
>
> Signed-off-by: Kairui Song <kasong@...cent.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 entry, gfp_t gfp_mask,
>  struct page *swapin_readahead(swp_entry_t entry, gfp_t gfp_mask,
>                               struct vm_fault *vmf, enum swap_cache_result *result)
>  {
> -       enum swap_cache_result cache_result;
>         struct swap_info_struct *si;
>         struct mempolicy *mpol;
>         void *shadow = NULL;
> @@ -928,29 +927,27 @@ struct page *swapin_readahead(swp_entry_t entry, gfp_t gfp_mask,
>
>         folio = swap_cache_get_folio(entry, vmf, &shadow);
>         if (folio) {
> +               *result = SWAP_CACHE_HIT;
>                 page = folio_file_page(folio, swp_offset(entry));
> -               cache_result = SWAP_CACHE_HIT;
>                 goto done;
>         }
>
>         mpol = get_vma_policy(vmf->vma, vmf->address, 0, &ilx);
>         if (swap_use_no_readahead(si, swp_offset(entry))) {
> +               *result = 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 = swapin_no_readahead(entry, gfp_mask, mpol, ilx, vmf->vma->vm_mm);
> -               cache_result = SWAP_CACHE_BYPASS;
>                 if (shadow)
>                         workingset_refault(page_folio(page), shadow);
> -       } else if (swap_use_vma_readahead(si)) {
> -               page = swap_vma_readahead(entry, gfp_mask, mpol, ilx, vmf);
> -               cache_result = SWAP_CACHE_MISS;
>         } else {
> -               page = swap_cluster_readahead(entry, gfp_mask, mpol, ilx);
> -               cache_result = SWAP_CACHE_MISS;
> +               *result = SWAP_CACHE_MISS;
> +               if (swap_use_vma_readahead(si))
> +                       page = swap_vma_readahead(entry, gfp_mask, mpol, ilx, vmf);
> +               else
> +                       page = swap_cluster_readahead(entry, gfp_mask, mpol, ilx);

I recall you introduce or heavy modify this function in previous patch before.
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 = cache_result;

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

Chris