Re: [RFC PATCH 1/8] mm: Switch mm->get_unmapped_area() to a flag

[Mike Rapoport <rppt@kernel.org>]

On Thu, Feb 15, 2024 at 03:13:25PM -0800, Rick Edgecombe wrote:
> The mm_struct contains a function pointer *get_unmapped_area(), which
> is set to either arch_get_unmapped_area() or
> arch_get_unmapped_area_topdown() during the initialization of the mm.
> 
> Since the function pointer only ever points to two functions that are named
> the same across all arch's, a function pointer is not really required. In
> addition future changes will want to add versions of the functions that
> take additional arguments. So to save a pointers worth of bytes in
> mm_struct, and prevent adding additional function pointers to mm_struct in
> future changes, remove it and keep the information about which
> get_unmapped_area() to use in a flag.
> 
> Introduce a helper, mm_get_unmapped_area(), to easily convert code that
> refers to the old function pointer to instead select and call either
> arch_get_unmapped_area() or arch_get_unmapped_area_topdown() based on the
> flag. Then drop the mm->get_unmapped_area() function pointer. Leave the
> get_unmapped_area() pointer in struct file_operations alone. The main
> purpose of this change is to reorganize in preparation for future changes,
> but it also converts the calls of mm->get_unmapped_area() from indirect
> branches into a direct ones.
> 
> The stress-ng bigheap benchmark calls realloc a lot, which calls through
> get_unmapped_area() in the kernel. On x86, the change yielded a ~4%
> improvement there. (bogo ops/s (usr+sys time))
> 
> In testing a few x86 configs, removing the pointer unfortunately didn't
> result in any actual size reductions in the compiled layout of mm_struct.
> But depending on compiler or arch alignment requirements, the change could
> possibly shrink the size of mm_struct.
> 
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
>  arch/s390/mm/hugetlbpage.c       |  2 +-
>  arch/s390/mm/mmap.c              |  4 ++--
>  arch/sparc/kernel/sys_sparc_64.c | 15 ++++++---------
>  arch/sparc/mm/hugetlbpage.c      |  2 +-
>  arch/x86/kernel/cpu/sgx/driver.c |  2 +-
>  arch/x86/mm/hugetlbpage.c        |  2 +-
>  arch/x86/mm/mmap.c               |  4 ++--
>  drivers/char/mem.c               |  2 +-
>  drivers/dax/device.c             |  6 +++---
>  fs/hugetlbfs/inode.c             |  2 +-
>  fs/proc/inode.c                  | 15 ++++++++-------
>  fs/ramfs/file-mmu.c              |  2 +-
>  include/linux/mm_types.h         |  6 +-----
>  include/linux/sched/coredump.h   |  1 +
>  include/linux/sched/mm.h         |  5 +++++
>  io_uring/io_uring.c              |  2 +-
>  mm/debug.c                       |  6 ------
>  mm/huge_memory.c                 |  6 +++---
>  mm/mmap.c                        | 21 ++++++++++++++++++---
>  mm/shmem.c                       | 11 +++++------
>  mm/util.c                        |  6 +++---
>  21 files changed, 65 insertions(+), 57 deletions(-)
> 
> diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
> index 02f5090ffea2..428e440424c5 100644
> --- a/include/linux/sched/coredump.h
> +++ b/include/linux/sched/coredump.h
> @@ -74,6 +74,7 @@ static inline int get_dumpable(struct mm_struct *mm)
>  #define MMF_DISABLE_THP_MASK	(1 << MMF_DISABLE_THP)
>  #define MMF_OOM_REAP_QUEUED	25	/* mm was queued for oom_reaper */
>  #define MMF_MULTIPROCESS	26	/* mm is shared between processes */
> +#define MMF_TOPDOWN		27	/* mm is shared between processes */

Nit: you may want to update the comment here ;-)

>  /*
>   * MMF_HAS_PINNED: Whether this mm has pinned any pages.  This can be either
>   * replaced in the future by mm.pinned_vm when it becomes stable, or grow into

-- 
Sincerely yours,
Mike.