| Message ID | 20231119194740.94101-22-ryncsn@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Swapin path refactor for optimization and bugfix | expand |
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
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 --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; }