| Message ID | 20230522082412.56685-1-lstoakes@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/mmap: refactor mlock_future_check() | expand |
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(...)) ?
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!
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);
On 5/22/23 10:24, Lorenzo Stoakes 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. > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/internal.h | 4 ++-- > mm/mmap.c | 33 +++++++++++++++++---------------- > mm/mremap.c | 2 +- > mm/secretmem.c | 2 +- > 4 files changed, 21 insertions(+), 20 deletions(-) > > diff --git a/mm/internal.h b/mm/internal.h > index bb6542279599..66dd214b302a 100644 > --- a/mm/internal.h > +++ b/mm/internal.h > @@ -576,8 +576,8 @@ extern long populate_vma_page_range(struct vm_area_struct *vma, > extern long faultin_vma_page_range(struct vm_area_struct *vma, > unsigned long start, unsigned long end, > bool write, int *locked); > -extern int mlock_future_check(struct mm_struct *mm, unsigned long flags, > - unsigned long len); > +extern bool mlock_future_check(struct mm_struct *mm, unsigned long flags, > + unsigned long bytes); This would have been good opportunity to drop the extern.
diff --git a/mm/internal.h b/mm/internal.h index bb6542279599..66dd214b302a 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -576,8 +576,8 @@ extern long populate_vma_page_range(struct vm_area_struct *vma, extern long faultin_vma_page_range(struct vm_area_struct *vma, unsigned long start, unsigned long end, bool write, int *locked); -extern int mlock_future_check(struct mm_struct *mm, unsigned long flags, - unsigned long len); +extern bool mlock_future_check(struct mm_struct *mm, unsigned long flags, + unsigned long bytes); /* * mlock_vma_folio() and munlock_vma_folio(): * should be called with vma's mmap_lock held for read or write, diff --git a/mm/mmap.c b/mm/mmap.c index 877696464c09..10f2d5d689da 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -182,7 +182,8 @@ static int check_brk_limits(unsigned long addr, unsigned long len) if (IS_ERR_VALUE(mapped_addr)) return mapped_addr; - return mlock_future_check(current->mm, current->mm->def_flags, len); + return mlock_future_check(current->mm, current->mm->def_flags, len) + ? 0 : -EAGAIN; } static int do_brk_flags(struct vma_iterator *vmi, struct vm_area_struct *brkvma, unsigned long addr, unsigned long request, unsigned long flags); @@ -1148,21 +1149,21 @@ static inline unsigned long round_hint_to_min(unsigned long hint) return hint; } -int mlock_future_check(struct mm_struct *mm, unsigned long flags, - unsigned long len) +bool mlock_future_check(struct mm_struct *mm, unsigned long flags, + unsigned long bytes) { - unsigned long locked, lock_limit; + unsigned long locked_pages, limit_pages; - /* mlock MCL_FUTURE? */ - if (flags & VM_LOCKED) { - locked = len >> PAGE_SHIFT; - locked += mm->locked_vm; - lock_limit = rlimit(RLIMIT_MEMLOCK); - lock_limit >>= PAGE_SHIFT; - if (locked > lock_limit && !capable(CAP_IPC_LOCK)) - return -EAGAIN; - } - return 0; + if (!(flags & VM_LOCKED) || capable(CAP_IPC_LOCK)) + return true; + + locked_pages = bytes >> PAGE_SHIFT; + locked_pages += mm->locked_vm; + + limit_pages = rlimit(RLIMIT_MEMLOCK); + limit_pages >>= PAGE_SHIFT; + + return locked_pages <= limit_pages; } static inline u64 file_mmap_size_max(struct file *file, struct inode *inode) @@ -1274,7 +1275,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr, if (!can_do_mlock()) return -EPERM; - if (mlock_future_check(mm, vm_flags, len)) + if (!mlock_future_check(mm, vm_flags, len)) return -EAGAIN; if (file) { @@ -1926,7 +1927,7 @@ static int acct_stack_growth(struct vm_area_struct *vma, return -ENOMEM; /* mlock limit tests */ - if (mlock_future_check(mm, vma->vm_flags, grow << PAGE_SHIFT)) + if (!mlock_future_check(mm, vma->vm_flags, grow << PAGE_SHIFT)) return -ENOMEM; /* Check to ensure the stack will not grow into a hugetlb-only region */ diff --git a/mm/mremap.c b/mm/mremap.c index b11ce6c92099..bcfcb8df5875 100644 --- a/mm/mremap.c +++ b/mm/mremap.c @@ -775,7 +775,7 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr, 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_check(mm, vma->vm_flags, new_len - old_len)) return ERR_PTR(-EAGAIN); if (!may_expand_vm(mm, vma->vm_flags, diff --git a/mm/secretmem.c b/mm/secretmem.c index 974b32ba8b9d..58d2af12df4f 100644 --- a/mm/secretmem.c +++ b/mm/secretmem.c @@ -125,7 +125,7 @@ static int secretmem_mmap(struct file *file, struct vm_area_struct *vma) 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_check(vma->vm_mm, vma->vm_flags | VM_LOCKED, len)) return -EAGAIN; vm_flags_set(vma, VM_LOCKED | VM_DONTDUMP);
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. Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> --- mm/internal.h | 4 ++-- mm/mmap.c | 33 +++++++++++++++++---------------- mm/mremap.c | 2 +- mm/secretmem.c | 2 +- 4 files changed, 21 insertions(+), 20 deletions(-)