| Message ID | 20231119194740.94101-20-ryncsn@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Swapin path refactor for optimization and bugfix | expand |
Hi Kairui, On Sun, Nov 19, 2023 at 11:49 AM Kairui Song <ryncsn@gmail.com> wrote: > > From: Kairui Song <kasong@tencent.com> > > It should always check if a swap entry is already swaped in on error, > fix potential false error issue. > > Signed-off-by: Kairui Song <kasong@tencent.com> > --- > mm/shmem.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/mm/shmem.c b/mm/shmem.c > index 81d129aa66d1..6154b5b68b8f 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -1857,13 +1857,11 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index, > page = swapin_page_non_fault(swap, gfp, mpol, ilx, fault_mm, &result); > mpol_cond_put(mpol); > > - if (PTR_ERR(page) == -EBUSY) { > - if (!shmem_confirm_swap(mapping, index, swap)) > - return -EEXIST; Do you intentionally remove checking shmem_confirm_swap()? I am not sure I am following. > + if (IS_ERR_OR_NULL(page)) { > + if (!page) > + error = -ENOMEM; > else > - return -EINVAL; > - } else if (!page) { > - error = -ENOMEM; > + error = -EINVAL; The resulting code is a bit hard to read in diff. In plan source it is like: if (IS_ERR_OR_NULL(page)) { if (!page) error = -ENOMEM; else error = -EINVAL; goto failed; } else { folio = page_folio(page); if (fault_type && result != SWAP_CACHE_HIT) { *fault_type |= VM_FAULT_MAJOR; count_vm_event(PGMAJFAULT); count_memcg_event_mm(fault_mm, PGMAJFAULT); } } First of all, if the first always "goto failed", the second else is not needed. The whole else block can be flatten one indentation. if (IS_ERR_OR_NULL(page)) { if (!page) error = -ENOMEM; else error = -EINVAL; goto failed; } else { Can be rewrite as following with less indentation: if (!page) { error = -ENOMEM; goto failed; } if (IS_ERR(page)) { error = -EINVAL; goto failed; } /* else block */ Am I missing something and misreading your code? Chris
Chris Li <chrisl@kernel.org> 于2023年11月20日周一 15:12写道： > > Hi Kairui, > > On Sun, Nov 19, 2023 at 11:49 AM Kairui Song <ryncsn@gmail.com> wrote: > > > > From: Kairui Song <kasong@tencent.com> > > > > It should always check if a swap entry is already swaped in on error, > > fix potential false error issue. > > > > Signed-off-by: Kairui Song <kasong@tencent.com> > > --- > > mm/shmem.c | 10 ++++------ > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/mm/shmem.c b/mm/shmem.c > > index 81d129aa66d1..6154b5b68b8f 100644 > > --- a/mm/shmem.c > > +++ b/mm/shmem.c > > @@ -1857,13 +1857,11 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index, > > page = swapin_page_non_fault(swap, gfp, mpol, ilx, fault_mm, &result); > > mpol_cond_put(mpol); > > > > - if (PTR_ERR(page) == -EBUSY) { > > - if (!shmem_confirm_swap(mapping, index, swap)) > > - return -EEXIST; > > Do you intentionally remove checking shmem_confirm_swap()? > I am not sure I am following. Hi, Chris, thanks for the review. This was also called under failed: label so I think there is no need to keep it here. > > > + if (IS_ERR_OR_NULL(page)) { > > + if (!page) > > + error = -ENOMEM; > > else > > - return -EINVAL; > > - } else if (!page) { > > - error = -ENOMEM; > > + error = -EINVAL; > > The resulting code is a bit hard to read in diff. In plan source it is like: > > if (IS_ERR_OR_NULL(page)) { > if (!page) > error = -ENOMEM; > else > error = -EINVAL; > goto failed; > } else { > folio = page_folio(page); > if (fault_type && result != SWAP_CACHE_HIT) { > *fault_type |= VM_FAULT_MAJOR; > count_vm_event(PGMAJFAULT); > count_memcg_event_mm(fault_mm, PGMAJFAULT); > } > } > > First of all, if the first always "goto failed", the second else is not needed. > The whole else block can be flatten one indentation. Yes, thanks for the suggestion. > > if (IS_ERR_OR_NULL(page)) { > if (!page) > error = -ENOMEM; > else > error = -EINVAL; > goto failed; > } else { > > Can be rewrite as following with less indentation: > > if (!page) { > error = -ENOMEM; > goto failed; > } > if (IS_ERR(page)) { > error = -EINVAL; > goto failed; > } > /* else block */ > > Am I missing something and misreading your code? Your code looks cleaner :) This patch is trying to clean up the error path after the helper refactor, and your suggestions are very helpful, thanks! > > Chris
diff --git a/mm/shmem.c b/mm/shmem.c index 81d129aa66d1..6154b5b68b8f 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -1857,13 +1857,11 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index, page = swapin_page_non_fault(swap, gfp, mpol, ilx, fault_mm, &result); mpol_cond_put(mpol); - if (PTR_ERR(page) == -EBUSY) { - if (!shmem_confirm_swap(mapping, index, swap)) - return -EEXIST; + if (IS_ERR_OR_NULL(page)) { + if (!page) + error = -ENOMEM; else - return -EINVAL; - } else if (!page) { - error = -ENOMEM; + error = -EINVAL; goto failed; } else { folio = page_folio(page);