Re: [PATCH 13/24] swap: simplify swap_cache_get_folio

[Chris Li]

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@xxxxxxxxx> wrote:
>
> From: Kairui Song <kasong@xxxxxxxxxxx>
>
> Rearrange the if statement, reduce the code indent, no feature change.
>
> Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> ---
>  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
>
>