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

[Chris Li <chrisl@kernel.org>]

On Sun, Nov 19, 2023 at 11:49 AM Kairui Song <ryncsn@gmail.com> wrote:
>
> 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 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