diff mbox series

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

Message ID 20231119194740.94101-22-ryncsn@gmail.com
State New
Headers
Series Swapin path refactor for optimization and bugfix |

Commit Message

Kairui Song Nov. 19, 2023, 7:47 p.m. UTC 
  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(-)

Comments

Chris Li Nov. 22, 2023, 5:15 a.m. UTC | #1 
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
Kairui Song Nov. 24, 2023, 8:14 a.m. UTC | #2 
Chris Li <chrisl@kernel.org> 于2023年11月22日周三 13:18写道：
>
> 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.

Yes, that's less error-prone indeed, saving a "if" seems doesn't worth
all the potential trouble, will drop this.
diff mbox series

Patch

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;
 		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);
 	}
 	mpol_cond_put(mpol);
 done:
 	put_swap_device(si);
-	if (result)
-		*result = cache_result;
 
 	return page;
 }