Re: [PATCH] mm, vmalloc: use __GFP_HIGHMEM implicitly

[Vlastimil Babka]

On 03/07/2017 03:10 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@xxxxxxxx>
> 
> __vmalloc* allows users to provide gfp flags for the underlying
> allocation. This API is quite popular
> $ git grep "=[[:space:]]__vmalloc\|return[[:space:]]*__vmalloc" | wc -l
> 77
> 
> the only problem is that many people are not aware that they really want
> to give __GFP_HIGHMEM along with other flags because there is really no
> reason to consume precious lowmemory on CONFIG_HIGHMEM systems for pages
> which are mapped to the kernel vmalloc space. About half of users don't
> use this flag, though. This signals that we make the API unnecessarily
> too complex.
> 
> This patch simply uses __GFP_HIGHMEM implicitly when allocating pages to
> be mapped to the vmalloc space. Current users which add __GFP_HIGHMEM
> are simplified and drop the flag.
> 
> Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>
> ---
> 
> Hi,
> this has been posted [1] as an RFC before and nobody really screamed
> about this. Well, there was only little feedback, to be honest. I still
> believe this is an improvement and the implicit __GFP_HIGHMEM will make
> the __vmalloc* usage less error prone.
> 
> [1] http://lkml.kernel.org/r/20170201140530.1325-1-mhocko@xxxxxxxxxx
> 
>  arch/parisc/kernel/module.c            |  2 +-
>  arch/x86/kernel/module.c               |  2 +-
>  drivers/block/drbd/drbd_bitmap.c       |  2 +-
>  drivers/gpu/drm/etnaviv/etnaviv_dump.c |  4 ++--
>  drivers/md/dm-bufio.c                  |  2 +-
>  fs/btrfs/free-space-tree.c             |  3 +--
>  fs/file.c                              |  2 +-
>  fs/xfs/kmem.c                          |  2 +-
>  include/drm/drm_mem_util.h             |  9 +++------
>  kernel/bpf/core.c                      |  9 +++------
>  kernel/bpf/syscall.c                   |  3 +--
>  kernel/fork.c                          |  2 +-
>  kernel/groups.c                        |  2 +-
>  kernel/module.c                        |  2 +-
>  mm/kasan/kasan.c                       |  2 +-
>  mm/nommu.c                             |  3 +--
>  mm/util.c                              |  2 +-
>  mm/vmalloc.c                           | 14 +++++++-------
>  net/ceph/ceph_common.c                 |  2 +-
>  net/netfilter/x_tables.c               |  3 +--
>  20 files changed, 31 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
> index a0ecdb4abcc8..3d4f5660a2e0 100644
> --- a/arch/parisc/kernel/module.c
> +++ b/arch/parisc/kernel/module.c
> @@ -218,7 +218,7 @@ void *module_alloc(unsigned long size)
>  	 * easier than trying to map the text, data, init_text and
>  	 * init_data correctly */
>  	return __vmalloc_node_range(size, 1, VMALLOC_START, VMALLOC_END,
> -				    GFP_KERNEL | __GFP_HIGHMEM,
> +				    GFP_KERNEL,
>  				    PAGE_KERNEL_RWX, 0, NUMA_NO_NODE,
>  				    __builtin_return_address(0));
>  }
> diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
> index 477ae806c2fa..f67bd3205df7 100644
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -85,7 +85,7 @@ void *module_alloc(unsigned long size)
>  
>  	p = __vmalloc_node_range(size, MODULE_ALIGN,
>  				    MODULES_VADDR + get_module_load_offset(),
> -				    MODULES_END, GFP_KERNEL | __GFP_HIGHMEM,
> +				    MODULES_END, GFP_KERNEL,
>  				    PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
>  				    __builtin_return_address(0));
>  	if (p && (kasan_module_alloc(p, size) < 0)) {
> diff --git a/drivers/block/drbd/drbd_bitmap.c b/drivers/block/drbd/drbd_bitmap.c
> index dece26f119d4..a804a4107fbc 100644
> --- a/drivers/block/drbd/drbd_bitmap.c
> +++ b/drivers/block/drbd/drbd_bitmap.c
> @@ -409,7 +409,7 @@ static struct page **bm_realloc_pages(struct drbd_bitmap *b, unsigned long want)
>  	new_pages = kzalloc(bytes, GFP_NOIO | __GFP_NOWARN);
>  	if (!new_pages) {
>  		new_pages = __vmalloc(bytes,
> -				GFP_NOIO | __GFP_HIGHMEM | __GFP_ZERO,
> +				GFP_NOIO | __GFP_ZERO,

This should be converted to memalloc_noio_save(), right? And then
kvmalloc? Unless that happens in your other series :)

>  				PAGE_KERNEL);
>  		if (!new_pages)
>  			return NULL;
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_dump.c b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> index d019b5e311cc..2d955d7d7b6d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_dump.c
> @@ -161,8 +161,8 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu)
>  	file_size += sizeof(*iter.hdr) * n_obj;
>  
>  	/* Allocate the file in vmalloc memory, it's likely to be big */
> -	iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_HIGHMEM |
> -			       __GFP_NOWARN | __GFP_NORETRY, PAGE_KERNEL);
> +	iter.start = __vmalloc(file_size, GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY,
> +			       PAGE_KERNEL);
>  	if (!iter.start) {
>  		dev_warn(gpu->dev, "failed to allocate devcoredump file\n");
>  		return;
> diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
> index df4859f6ac6a..c058ae86f51b 100644
> --- a/drivers/md/dm-bufio.c
> +++ b/drivers/md/dm-bufio.c
> @@ -404,7 +404,7 @@ static void *alloc_buffer_data(struct dm_bufio_client *c, gfp_t gfp_mask,
>  	if (gfp_mask & __GFP_NORETRY)
>  		noio_flag = memalloc_noio_save();
>  
> -	ptr = __vmalloc(c->block_size, gfp_mask | __GFP_HIGHMEM, PAGE_KERNEL);
> +	ptr = __vmalloc(c->block_size, gfp_mask, PAGE_KERNEL);
>  
>  	if (gfp_mask & __GFP_NORETRY)
>  		memalloc_noio_restore(noio_flag);
> diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> index dd7fb22a955a..fc0bd8406758 100644
> --- a/fs/btrfs/free-space-tree.c
> +++ b/fs/btrfs/free-space-tree.c
> @@ -167,8 +167,7 @@ static u8 *alloc_bitmap(u32 bitmap_size)
>  	if (mem)
>  		return mem;
>  
> -	return __vmalloc(bitmap_size, GFP_NOFS | __GFP_HIGHMEM | __GFP_ZERO,
> -			 PAGE_KERNEL);
> +	return __vmalloc(bitmap_size, GFP_NOFS | __GFP_ZERO, PAGE_KERNEL);

memalloc_nofs_save() and plain vzalloc()?

>  }
>  
>  int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,

[...]

> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -693,7 +693,7 @@ int kasan_module_alloc(void *addr, size_t size)
>  
>  	ret = __vmalloc_node_range(shadow_size, 1, shadow_start,
>  			shadow_start + shadow_size,
> -			GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO,
> +			GFP_KERNEL | __GFP_ZERO,
>  			PAGE_KERNEL, VM_NO_GUARD, NUMA_NO_NODE,
>  			__builtin_return_address(0));
>  
> diff --git a/mm/nommu.c b/mm/nommu.c
> index a80411d258fc..fc184f597d59 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -246,8 +246,7 @@ void *vmalloc_user(unsigned long size)
>  {
>  	void *ret;
>  
> -	ret = __vmalloc(size, GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO,
> -			PAGE_KERNEL);
> +	ret = __vmalloc(size, GFP_KERNEL | __GFP_ZERO, PAGE_KERNEL);

vzalloc()?

>  	if (ret) {
>  		struct vm_area_struct *vma;
>  
> diff --git a/mm/util.c b/mm/util.c
> index 6ed3e49bf1e5..e5b0623df89d 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -382,7 +382,7 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
>  	if (ret || size <= PAGE_SIZE)
>  		return ret;
>  
> -	return __vmalloc_node_flags(size, node, flags | __GFP_HIGHMEM);
> +	return __vmalloc_node_flags(size, node, flags);
>  }
>  EXPORT_SYMBOL(kvmalloc_node);
>  
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 32979d945766..9fa9274d8f6d 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1619,7 +1619,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>  	struct page **pages;
>  	unsigned int nr_pages, array_size, i;
>  	const gfp_t nested_gfp = (gfp_mask & GFP_RECLAIM_MASK) | __GFP_ZERO;
> -	const gfp_t alloc_mask = gfp_mask | __GFP_NOWARN;
> +	const gfp_t alloc_mask = gfp_mask | __GFP_HIGHMEM | __GFP_NOWARN;
>  
>  	nr_pages = get_vm_area_size(area) >> PAGE_SHIFT;
>  	array_size = (nr_pages * sizeof(struct page *));
> @@ -1788,7 +1788,7 @@ void *__vmalloc_node_flags(unsigned long size,
>  void *vmalloc(unsigned long size)
>  {
>  	return __vmalloc_node_flags(size, NUMA_NO_NODE,
> -				    GFP_KERNEL | __GFP_HIGHMEM);
> +				    GFP_KERNEL);

Nit: this could now fit on single line.

>  }
>  EXPORT_SYMBOL(vmalloc);
>  
> @@ -1805,7 +1805,7 @@ EXPORT_SYMBOL(vmalloc);
>  void *vzalloc(unsigned long size)
>  {
>  	return __vmalloc_node_flags(size, NUMA_NO_NODE,
> -				GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO);
> +				GFP_KERNEL | __GFP_ZERO);
>  }
>  EXPORT_SYMBOL(vzalloc);
>  
> @@ -1822,7 +1822,7 @@ void *vmalloc_user(unsigned long size)
>  	void *ret;
>  
>  	ret = __vmalloc_node(size, SHMLBA,
> -			     GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO,
> +			     GFP_KERNEL | __GFP_ZERO,
>  			     PAGE_KERNEL, NUMA_NO_NODE,
>  			     __builtin_return_address(0));
>  	if (ret) {
> @@ -1846,7 +1846,7 @@ EXPORT_SYMBOL(vmalloc_user);
>   */
>  void *vmalloc_node(unsigned long size, int node)
>  {
> -	return __vmalloc_node(size, 1, GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL,
> +	return __vmalloc_node(size, 1, GFP_KERNEL, PAGE_KERNEL,
>  					node, __builtin_return_address(0));
>  }
>  EXPORT_SYMBOL(vmalloc_node);
> @@ -1866,7 +1866,7 @@ EXPORT_SYMBOL(vmalloc_node);
>  void *vzalloc_node(unsigned long size, int node)
>  {
>  	return __vmalloc_node_flags(size, node,
> -			 GFP_KERNEL | __GFP_HIGHMEM | __GFP_ZERO);
> +			 GFP_KERNEL | __GFP_ZERO);

This too.