Messages in this thread Patch in this message | | | Date | Mon, 22 May 2023 13:56:15 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] mm/mmap: refactor mlock_future_check() |
| |
On Mon, 22 May 2023 21:34:31 +0100 Lorenzo Stoakes <lstoakes@gmail.com> wrote:
> On Mon, May 22, 2023 at 01:28:51PM -0700, Andrew Morton wrote: > > On Mon, 22 May 2023 09:24:12 +0100 Lorenzo Stoakes <lstoakes@gmail.com> wrote: > > > > > In all but one instance, mlock_future_check() is treated as a boolean > > > function despite returning an error code. In one instance, this error code > > > is ignored and replaced with -ENOMEM. > > > > > > This is confusing, and the inversion of true -> failure, false -> success > > > is not warranted. Convert the function to a bool, lightly refactor and > > > return true if the check passes, false if not. > > > > Yup. > > > > I don't think the name does a good job of conveying the > > function's use. > > > > > - if (mlock_future_check(mm, vm_flags, len)) > > > + if (!mlock_future_check(mm, vm_flags, len)) > > > return -EAGAIN; > > > > if (!may_mlock_future(...)) > > > > or > > > > if (!mlock_future_ok(...)) > > > > ? > > > > > > Yeah I struggled with this, because the check only triggers if VM_LOCKED. I was > originally toying with can_mlock_future() but of course, it also returns true if > !VM_LOCKED... > > I think your suggestion of mlock_future_ok() works well, could you change it to > that? Thanks!
Sure. I'll make it a separate patch.
From: Andrew Morton <akpm@linux-foundation.org> Subject: mm/mlock: rename mlock_future_check() to mlock_future_ok() Date: Mon May 22 01:52:10 PM PDT 2023
It is felt that the name mlock_future_check() is vague - it doesn't particularly convey the function's operation. mlock_future_ok() is a clearer name for a predicate function.
Cc: Liam Howlett <liam.howlett@oracle.com> Cc: Lorenzo Stoakes <lstoakes@gmail.com> Cc: Mike Rapoport (IBM) <rppt@kernel.org> Cc: Vlastimil Babka <vbabka@suse.cz> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> ---
mm/internal.h | 2 +- mm/mmap.c | 8 ++++---- mm/mremap.c | 2 +- mm/secretmem.c | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-)
--- a/mm/internal.h~mm-mlock-rename-mlock_future_check-to-mlock_future_ok +++ a/mm/internal.h @@ -576,7 +576,7 @@ extern long populate_vma_page_range(stru extern long faultin_vma_page_range(struct vm_area_struct *vma, unsigned long start, unsigned long end, bool write, int *locked); -extern bool mlock_future_check(struct mm_struct *mm, unsigned long flags, +extern bool mlock_future_ok(struct mm_struct *mm, unsigned long flags, unsigned long bytes); /* * mlock_vma_folio() and munlock_vma_folio(): --- a/mm/mmap.c~mm-mlock-rename-mlock_future_check-to-mlock_future_ok +++ a/mm/mmap.c @@ -182,7 +182,7 @@ static int check_brk_limits(unsigned lon if (IS_ERR_VALUE(mapped_addr)) return mapped_addr; - return mlock_future_check(current->mm, current->mm->def_flags, len) + return mlock_future_ok(current->mm, current->mm->def_flags, len) ? 0 : -EAGAIN; } static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *brkvma, @@ -1149,7 +1149,7 @@ static inline unsigned long round_hint_t return hint; } -bool mlock_future_check(struct mm_struct *mm, unsigned long flags, +bool mlock_future_ok(struct mm_struct *mm, unsigned long flags, unsigned long bytes) { unsigned long locked_pages, limit_pages; @@ -1275,7 +1275,7 @@ unsigned long do_mmap(struct file *file, if (!can_do_mlock()) return -EPERM; - if (!mlock_future_check(mm, vm_flags, len)) + if (!mlock_future_ok(mm, vm_flags, len)) return -EAGAIN; if (file) { @@ -1927,7 +1927,7 @@ static int acct_stack_growth(struct vm_a return -ENOMEM; /* mlock limit tests */ - if (!mlock_future_check(mm, vma->vm_flags, grow << PAGE_SHIFT)) + if (!mlock_future_ok(mm, vma->vm_flags, grow << PAGE_SHIFT)) return -ENOMEM; /* Check to ensure the stack will not grow into a hugetlb-only region */ --- a/mm/mremap.c~mm-mlock-rename-mlock_future_check-to-mlock_future_ok +++ a/mm/mremap.c @@ -775,7 +775,7 @@ static struct vm_area_struct *vma_to_res if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP)) return ERR_PTR(-EFAULT); - if (!mlock_future_check(mm, vma->vm_flags, new_len - old_len)) + if (!mlock_future_ok(mm, vma->vm_flags, new_len - old_len)) return ERR_PTR(-EAGAIN); if (!may_expand_vm(mm, vma->vm_flags, --- a/mm/secretmem.c~mm-mlock-rename-mlock_future_check-to-mlock_future_ok +++ a/mm/secretmem.c @@ -125,7 +125,7 @@ static int secretmem_mmap(struct file *f if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0) return -EINVAL; - if (!mlock_future_check(vma->vm_mm, vma->vm_flags | VM_LOCKED, len)) + if (!mlock_future_ok(vma->vm_mm, vma->vm_flags | VM_LOCKED, len)) return -EAGAIN; vm_flags_set(vma, VM_LOCKED | VM_DONTDUMP); _
| |