Re: [PATCH] mm: Remove mm argument from mm_get_unmapped_area()
On Mon May 6, 2024 at 7:07 PM EEST, Rick Edgecombe wrote:
> Recently the get_unmapped_area() pointer on mm_struct was removed in
> favor of direct callable function that can determines which of two
> handlers to call based on an mm flag. This function,
> mm_get_unmapped_area(), checks the flag of the mm passed as an argument.
>
> Dan Williams pointed out (see link) that all callers pass curret->mm, so
> the mm argument is unneeded. It could be conceivable for a caller to want
> to pass a different mm in the future, but in this case a new helper could
> easily be added.
>
> So remove the mm argument, and rename the function
> current_get_unmapped_area().
>
> Fixes: 529ce23a764f ("mm: switch mm->get_unmapped_area() to a flag")
> Suggested-by: Dan Williams
> Signed-off-by: Rick Edgecombe
> Link:
> https://lore.kernel.org/lkml/6603bed6662a_4a98a29...@dwillia2-mobl3.amr.corp.intel.com.notmuch/
> ---
> Based on linux-next.
> ---
> arch/sparc/kernel/sys_sparc_64.c | 9 +
> arch/x86/kernel/cpu/sgx/driver.c | 2 +-
> drivers/char/mem.c | 2 +-
> drivers/dax/device.c | 6 +++---
> fs/proc/inode.c | 2 +-
> fs/ramfs/file-mmu.c | 2 +-
> include/linux/sched/mm.h | 6 +++---
> io_uring/memmap.c| 2 +-
> kernel/bpf/arena.c | 2 +-
> kernel/bpf/syscall.c | 2 +-
> mm/mmap.c| 11 +--
> mm/shmem.c | 9 -
> 12 files changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/arch/sparc/kernel/sys_sparc_64.c
> b/arch/sparc/kernel/sys_sparc_64.c
> index d9c3b34ca744..cf0b4ace5bf9 100644
> --- a/arch/sparc/kernel/sys_sparc_64.c
> +++ b/arch/sparc/kernel/sys_sparc_64.c
> @@ -220,7 +220,7 @@ unsigned long get_fb_unmapped_area(struct file *filp,
> unsigned long orig_addr, u
>
> if (flags & MAP_FIXED) {
> /* Ok, don't mess with it. */
> - return mm_get_unmapped_area(current->mm, NULL, orig_addr, len,
> pgoff, flags);
> + return current_get_unmapped_area(NULL, orig_addr, len, pgoff,
> flags);
> }
> flags &= ~MAP_SHARED;
>
> @@ -233,8 +233,9 @@ unsigned long get_fb_unmapped_area(struct file *filp,
> unsigned long orig_addr, u
> align_goal = (64UL * 1024);
>
> do {
> - addr = mm_get_unmapped_area(current->mm, NULL, orig_addr,
> - len + (align_goal - PAGE_SIZE),
> pgoff, flags);
> + addr = current_get_unmapped_area(NULL, orig_addr,
> + len + (align_goal - PAGE_SIZE),
> + pgoff, flags);
> if (!(addr & ~PAGE_MASK)) {
> addr = (addr + (align_goal - 1UL)) & ~(align_goal -
> 1UL);
> break;
> @@ -252,7 +253,7 @@ unsigned long get_fb_unmapped_area(struct file *filp,
> unsigned long orig_addr, u
>* be obtained.
>*/
> if (addr & ~PAGE_MASK)
> - addr = mm_get_unmapped_area(current->mm, NULL, orig_addr, len,
> pgoff, flags);
> + addr = current_get_unmapped_area(NULL, orig_addr, len, pgoff,
> flags);
>
> return addr;
> }
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> b/arch/x86/kernel/cpu/sgx/driver.c
> index 22b65a5f5ec6..5f7bfd9035f7 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -113,7 +113,7 @@ static unsigned long sgx_get_unmapped_area(struct file
> *file,
> if (flags & MAP_FIXED)
> return addr;
>
> - return mm_get_unmapped_area(current->mm, file, addr, len, pgoff, flags);
> + return current_get_unmapped_area(file, addr, len, pgoff, flags);
> }
>
> #ifdef CONFIG_COMPAT
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 7c359cc406d5..a29c4bd506d5 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -546,7 +546,7 @@ static unsigned long get_unmapped_area_zero(struct file
> *file,
> }
>
> /* Otherwise flags & MAP_PRIVATE: with no shmem object beneath it */
> - return mm_get_unmapped_area(current->mm, file, addr, len, pgoff, flags);
> + return current_get_unmapped_area(file, addr, len, pgoff, flags);
> #else
> return -ENOSYS;
> #endif
> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index eb61598247a9..c379902307b7 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -329,14 +329,14 @@ static unsigned long dax_get_unmapped_area(struct file
> *filp,
> if ((off + len_align) < off)
> goto out;
>
> - addr_align = mm_get_unmapped_area(current->mm, filp, addr, len_align,
> - pgoff, flags);
> + addr_align = current_get_unmapped_area(filp, addr, len_align,
> +pgoff, flags);
> if (!IS_ERR_VALUE(addr_align)) {
>
Re: [PATCH] mm: Remove mm argument from mm_get_unmapped_area()
* Edgecombe, Rick P [240507 09:51]: > On Mon, 2024-05-06 at 12:32 -0400, Liam R. Howlett wrote: > > > > I like this patch. > > Thanks for taking a look. > > > > > I think the context of current->mm is implied. IOW, could we call it > > get_unmapped_area() instead? There are other functions today that use > > current->mm that don't start with current_. I probably should > > have responded to Dan's suggestion with my comment. > > Yes, get_unmapped_area() is already taken. What else to call it... It is kind > of > the process "default" get_unmapped_area(). But with Christoph's proposal it > would basically be arch_get_unmapped_area(). unmapped_area(), but that's also taken.. arch_get_unmapped_area() are all quite close. If you look into it, many of the arch versions were taken from the sparc 32 version. Subsequent changes were made and they are no longer exactly the same, but I believe functionally equivalent - rather tricky to test though. I wanted to unite these to simplify the mm code a while back, but have not gotten back to it. One aspect that some archs have is "cache coloring" which does affect the VMAs. The other difference is VDSO, which I may be looking into soon. Someone once called me a glutton for punishment and there may be some truth in that... Cheers, Liam
Re: [PATCH] mm: Remove mm argument from mm_get_unmapped_area()
On Mon, 2024-05-06 at 09:18 -0700, Christoph Hellwig wrote:
> > On Mon, May 06, 2024 at 09:07:47AM -0700, Rick Edgecombe wrote:
> > > > if (flags & MAP_FIXED) {
> > > > /* Ok, don't mess with it. */
> > > > - return mm_get_unmapped_area(current->mm, NULL,
> > > > orig_addr, > > len, pgoff, flags);
> > > > + return current_get_unmapped_area(NULL, orig_addr, len, >
> > > > > pgoff, flags);
> >
> > The old name seems preferable because it's not as crazy long. In fact
> > just get_unmapped_area would be even better, but that's already taken
> > by something else.
Ok.
> >
> > Can we maybe take a step back and sort out the mess of the various
> > _get_unmapped_area helpers?
> >
> > e.g. mm_get_unmapped_area_vmflags just wraps
> > arch_get_unmapped_area_topdown_vmflags and
> > arch_get_unmapped_area_vmflags, and we might as well merge all three
> > by moving the MMF_TOPDOWN into two actual implementations?
> >
> > And then just update all the implementations to always pass the
> > vm_flags instead of having separate implementations with our without
> > the flags.
> >
> > And then make __get_unmapped_area static in mmap.c nad move the
> > get_unmapped_area wrappers there. And eventually write some
> > documentation for the functions based on the learnings who actually
> > uses what..
The rest of the series[0] is in the mm-tree/linux-next currently. Are you
suggesting we not do this patch, and leave the rest you describe here for the
future? I think the removal of the indirect branch is at least a positive step
forward.
[0]
https://lore.kernel.org/lkml/20240326021656.202649-1-rick.p.edgeco...@intel.com/
Re: [PATCH] mm: Remove mm argument from mm_get_unmapped_area()
On Mon, 2024-05-06 at 12:32 -0400, Liam R. Howlett wrote: > > I like this patch. Thanks for taking a look. > > I think the context of current->mm is implied. IOW, could we call it > get_unmapped_area() instead? There are other functions today that use > current->mm that don't start with current_. I probably should > have responded to Dan's suggestion with my comment. Yes, get_unmapped_area() is already taken. What else to call it... It is kind of the process "default" get_unmapped_area(). But with Christoph's proposal it would basically be arch_get_unmapped_area(). > > Either way, this is a minor thing so feel free to add: > Reviewed-by: Liam R. Howlett
Re: [PATCH] mm: Remove mm argument from mm_get_unmapped_area()
* Rick Edgecombe [240506 12:08]:
> Recently the get_unmapped_area() pointer on mm_struct was removed in
> favor of direct callable function that can determines which of two
> handlers to call based on an mm flag. This function,
> mm_get_unmapped_area(), checks the flag of the mm passed as an argument.
>
> Dan Williams pointed out (see link) that all callers pass curret->mm, so
> the mm argument is unneeded. It could be conceivable for a caller to want
> to pass a different mm in the future, but in this case a new helper could
> easily be added.
>
> So remove the mm argument, and rename the function
> current_get_unmapped_area().
I like this patch.
I think the context of current->mm is implied. IOW, could we call it
get_unmapped_area() instead? There are other functions today that use
current->mm that don't start with current_. I probably should
have responded to Dan's suggestion with my comment.
Either way, this is a minor thing so feel free to add:
Reviewed-by: Liam R. Howlett
>
> Fixes: 529ce23a764f ("mm: switch mm->get_unmapped_area() to a flag")
> Suggested-by: Dan Williams
> Signed-off-by: Rick Edgecombe
> Link:
> https://lore.kernel.org/lkml/6603bed6662a_4a98a29...@dwillia2-mobl3.amr.corp.intel.com.notmuch/
> ---
> Based on linux-next.
> ---
> arch/sparc/kernel/sys_sparc_64.c | 9 +
> arch/x86/kernel/cpu/sgx/driver.c | 2 +-
> drivers/char/mem.c | 2 +-
> drivers/dax/device.c | 6 +++---
> fs/proc/inode.c | 2 +-
> fs/ramfs/file-mmu.c | 2 +-
> include/linux/sched/mm.h | 6 +++---
> io_uring/memmap.c| 2 +-
> kernel/bpf/arena.c | 2 +-
> kernel/bpf/syscall.c | 2 +-
> mm/mmap.c| 11 +--
> mm/shmem.c | 9 -
> 12 files changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/arch/sparc/kernel/sys_sparc_64.c
> b/arch/sparc/kernel/sys_sparc_64.c
> index d9c3b34ca744..cf0b4ace5bf9 100644
> --- a/arch/sparc/kernel/sys_sparc_64.c
> +++ b/arch/sparc/kernel/sys_sparc_64.c
> @@ -220,7 +220,7 @@ unsigned long get_fb_unmapped_area(struct file *filp,
> unsigned long orig_addr, u
>
> if (flags & MAP_FIXED) {
> /* Ok, don't mess with it. */
> - return mm_get_unmapped_area(current->mm, NULL, orig_addr, len,
> pgoff, flags);
> + return current_get_unmapped_area(NULL, orig_addr, len, pgoff,
> flags);
> }
> flags &= ~MAP_SHARED;
>
> @@ -233,8 +233,9 @@ unsigned long get_fb_unmapped_area(struct file *filp,
> unsigned long orig_addr, u
> align_goal = (64UL * 1024);
>
> do {
> - addr = mm_get_unmapped_area(current->mm, NULL, orig_addr,
> - len + (align_goal - PAGE_SIZE),
> pgoff, flags);
> + addr = current_get_unmapped_area(NULL, orig_addr,
> + len + (align_goal - PAGE_SIZE),
> + pgoff, flags);
> if (!(addr & ~PAGE_MASK)) {
> addr = (addr + (align_goal - 1UL)) & ~(align_goal -
> 1UL);
> break;
> @@ -252,7 +253,7 @@ unsigned long get_fb_unmapped_area(struct file *filp,
> unsigned long orig_addr, u
>* be obtained.
>*/
> if (addr & ~PAGE_MASK)
> - addr = mm_get_unmapped_area(current->mm, NULL, orig_addr, len,
> pgoff, flags);
> + addr = current_get_unmapped_area(NULL, orig_addr, len, pgoff,
> flags);
>
> return addr;
> }
> diff --git a/arch/x86/kernel/cpu/sgx/driver.c
> b/arch/x86/kernel/cpu/sgx/driver.c
> index 22b65a5f5ec6..5f7bfd9035f7 100644
> --- a/arch/x86/kernel/cpu/sgx/driver.c
> +++ b/arch/x86/kernel/cpu/sgx/driver.c
> @@ -113,7 +113,7 @@ static unsigned long sgx_get_unmapped_area(struct file
> *file,
> if (flags & MAP_FIXED)
> return addr;
>
> - return mm_get_unmapped_area(current->mm, file, addr, len, pgoff, flags);
> + return current_get_unmapped_area(file, addr, len, pgoff, flags);
> }
>
> #ifdef CONFIG_COMPAT
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 7c359cc406d5..a29c4bd506d5 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -546,7 +546,7 @@ static unsigned long get_unmapped_area_zero(struct file
> *file,
> }
>
> /* Otherwise flags & MAP_PRIVATE: with no shmem object beneath it */
> - return mm_get_unmapped_area(current->mm, file, addr, len, pgoff, flags);
> + return current_get_unmapped_area(file, addr, len, pgoff, flags);
> #else
> return -ENOSYS;
> #endif
> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index eb61598247a9..c379902307b7 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -329,14 +329,14 @@ static unsigned long dax_get_unmapped_area(struct file
> *filp,
> if ((off + len_align) < off)
>
Re: [PATCH] mm: Remove mm argument from mm_get_unmapped_area()
On Mon, May 06, 2024 at 09:07:47AM -0700, Rick Edgecombe wrote:
> if (flags & MAP_FIXED) {
> /* Ok, don't mess with it. */
> - return mm_get_unmapped_area(current->mm, NULL, orig_addr, len,
> pgoff, flags);
> + return current_get_unmapped_area(NULL, orig_addr, len, pgoff,
> flags);
The old name seems preferable because it's not as crazy long. In fact
just get_unmapped_area would be even better, but that's already taken
by something else.
Can we maybe take a step back and sort out the mess of the various
_get_unmapped_area helpers?
e.g. mm_get_unmapped_area_vmflags just wraps
arch_get_unmapped_area_topdown_vmflags and
arch_get_unmapped_area_vmflags, and we might as well merge all three
by moving the MMF_TOPDOWN into two actual implementations?
And then just update all the implementations to always pass the
vm_flags instead of having separate implementations with our without
the flags.
And then make __get_unmapped_area static in mmap.c nad move the
get_unmapped_area wrappers there. And eventually write some
documentation for the functions based on the learnings who actually
uses what..
[PATCH] mm: Remove mm argument from mm_get_unmapped_area()
Recently the get_unmapped_area() pointer on mm_struct was removed in
favor of direct callable function that can determines which of two
handlers to call based on an mm flag. This function,
mm_get_unmapped_area(), checks the flag of the mm passed as an argument.
Dan Williams pointed out (see link) that all callers pass curret->mm, so
the mm argument is unneeded. It could be conceivable for a caller to want
to pass a different mm in the future, but in this case a new helper could
easily be added.
So remove the mm argument, and rename the function
current_get_unmapped_area().
Fixes: 529ce23a764f ("mm: switch mm->get_unmapped_area() to a flag")
Suggested-by: Dan Williams
Signed-off-by: Rick Edgecombe
Link:
https://lore.kernel.org/lkml/6603bed6662a_4a98a29...@dwillia2-mobl3.amr.corp.intel.com.notmuch/
---
Based on linux-next.
---
arch/sparc/kernel/sys_sparc_64.c | 9 +
arch/x86/kernel/cpu/sgx/driver.c | 2 +-
drivers/char/mem.c | 2 +-
drivers/dax/device.c | 6 +++---
fs/proc/inode.c | 2 +-
fs/ramfs/file-mmu.c | 2 +-
include/linux/sched/mm.h | 6 +++---
io_uring/memmap.c| 2 +-
kernel/bpf/arena.c | 2 +-
kernel/bpf/syscall.c | 2 +-
mm/mmap.c| 11 +--
mm/shmem.c | 9 -
12 files changed, 27 insertions(+), 28 deletions(-)
diff --git a/arch/sparc/kernel/sys_sparc_64.c b/arch/sparc/kernel/sys_sparc_64.c
index d9c3b34ca744..cf0b4ace5bf9 100644
--- a/arch/sparc/kernel/sys_sparc_64.c
+++ b/arch/sparc/kernel/sys_sparc_64.c
@@ -220,7 +220,7 @@ unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long orig_addr, u
if (flags & MAP_FIXED) {
/* Ok, don't mess with it. */
- return mm_get_unmapped_area(current->mm, NULL, orig_addr, len,
pgoff, flags);
+ return current_get_unmapped_area(NULL, orig_addr, len, pgoff,
flags);
}
flags &= ~MAP_SHARED;
@@ -233,8 +233,9 @@ unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long orig_addr, u
align_goal = (64UL * 1024);
do {
- addr = mm_get_unmapped_area(current->mm, NULL, orig_addr,
- len + (align_goal - PAGE_SIZE),
pgoff, flags);
+ addr = current_get_unmapped_area(NULL, orig_addr,
+len + (align_goal - PAGE_SIZE),
+pgoff, flags);
if (!(addr & ~PAGE_MASK)) {
addr = (addr + (align_goal - 1UL)) & ~(align_goal -
1UL);
break;
@@ -252,7 +253,7 @@ unsigned long get_fb_unmapped_area(struct file *filp,
unsigned long orig_addr, u
* be obtained.
*/
if (addr & ~PAGE_MASK)
- addr = mm_get_unmapped_area(current->mm, NULL, orig_addr, len,
pgoff, flags);
+ addr = current_get_unmapped_area(NULL, orig_addr, len, pgoff,
flags);
return addr;
}
diff --git a/arch/x86/kernel/cpu/sgx/driver.c b/arch/x86/kernel/cpu/sgx/driver.c
index 22b65a5f5ec6..5f7bfd9035f7 100644
--- a/arch/x86/kernel/cpu/sgx/driver.c
+++ b/arch/x86/kernel/cpu/sgx/driver.c
@@ -113,7 +113,7 @@ static unsigned long sgx_get_unmapped_area(struct file
*file,
if (flags & MAP_FIXED)
return addr;
- return mm_get_unmapped_area(current->mm, file, addr, len, pgoff, flags);
+ return current_get_unmapped_area(file, addr, len, pgoff, flags);
}
#ifdef CONFIG_COMPAT
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 7c359cc406d5..a29c4bd506d5 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -546,7 +546,7 @@ static unsigned long get_unmapped_area_zero(struct file
*file,
}
/* Otherwise flags & MAP_PRIVATE: with no shmem object beneath it */
- return mm_get_unmapped_area(current->mm, file, addr, len, pgoff, flags);
+ return current_get_unmapped_area(file, addr, len, pgoff, flags);
#else
return -ENOSYS;
#endif
diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index eb61598247a9..c379902307b7 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -329,14 +329,14 @@ static unsigned long dax_get_unmapped_area(struct file
*filp,
if ((off + len_align) < off)
goto out;
- addr_align = mm_get_unmapped_area(current->mm, filp, addr, len_align,
- pgoff, flags);
+ addr_align = current_get_unmapped_area(filp, addr, len_align,
+ pgoff, flags);
if (!IS_ERR_VALUE(addr_align)) {
addr_align += (off - addr_align) & (align - 1);
return addr_align;
}
out:
- return mm_get_unmapped_area(current->mm, filp, addr, len, pgoff, flags);
+ return current_get_unmapped_area(filp,
