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

[Michal Hocko]

On Tue 07-03-17 10:57:48, Matthew Wilcox wrote:
> On Tue, Mar 07, 2017 at 10:28:41AM -0800, Matthew Wilcox wrote:
> > On Tue, Mar 07, 2017 at 03:10:20PM +0100, Michal Hocko wrote:
> > > 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.
> 
> btw, I had another idea for GFP_HIGHMEM -- remove it when CONFIG_HIGHMEM
> isn't enabled.  Saves 26 bytes of .text and 64 bytes of .data on my
> laptop's kernel build.  What do you think?

I wouldn't be opposed. The downside would be a slight confusion when
printing gfp flags but we already have this for ___GFP_NOLOCKDEP ;)

> Also, I suspect the layout of bits is suboptimal from an assembly
> language perspective.  I still mostly care about x86 which doesn't
> benefit, so I'm not inclined to do the work, but certainly ARM, PA-RISC,
> SPARC and Itanium would all benefit from having frequently-used bits
> (ie those used in GFP_KERNEL and GFP_ATOMIC) placed in the low 8 bits.

be careful that there is some elaborate logic around low gfp bits to map
to proper zones and ALLOC_ constants.

> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 0fe0b6295ab5..d88cb532d7c8 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -16,7 +16,11 @@ struct vm_area_struct;
>  
>  /* Plain integer GFP bitmasks. Do not use this directly. */
>  #define ___GFP_DMA   0x01u
> +#ifdef CONFIG_HIGHMEM
>  #define ___GFP_HIGHMEM   0x02u
> +#else
> +#define ___GFP_HIGHMEM   0x0u
> +#endif
>  #define ___GFP_DMA32 0x04u
>  #define ___GFP_MOVABLE   0x08u
>  #define ___GFP_RECLAIMABLE   0x10u

Anyway, thanks for your review!
-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Tue 07-03-17 10:57:48, Matthew Wilcox wrote:
> On Tue, Mar 07, 2017 at 10:28:41AM -0800, Matthew Wilcox wrote:
> > On Tue, Mar 07, 2017 at 03:10:20PM +0100, Michal Hocko wrote:
> > > 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.
> 
> btw, I had another idea for GFP_HIGHMEM -- remove it when CONFIG_HIGHMEM
> isn't enabled.  Saves 26 bytes of .text and 64 bytes of .data on my
> laptop's kernel build.  What do you think?

I wouldn't be opposed. The downside would be a slight confusion when
printing gfp flags but we already have this for ___GFP_NOLOCKDEP ;)

> Also, I suspect the layout of bits is suboptimal from an assembly
> language perspective.  I still mostly care about x86 which doesn't
> benefit, so I'm not inclined to do the work, but certainly ARM, PA-RISC,
> SPARC and Itanium would all benefit from having frequently-used bits
> (ie those used in GFP_KERNEL and GFP_ATOMIC) placed in the low 8 bits.

be careful that there is some elaborate logic around low gfp bits to map
to proper zones and ALLOC_ constants.

> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 0fe0b6295ab5..d88cb532d7c8 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -16,7 +16,11 @@ struct vm_area_struct;
>  
>  /* Plain integer GFP bitmasks. Do not use this directly. */
>  #define ___GFP_DMA   0x01u
> +#ifdef CONFIG_HIGHMEM
>  #define ___GFP_HIGHMEM   0x02u
> +#else
> +#define ___GFP_HIGHMEM   0x0u
> +#endif
>  #define ___GFP_DMA32 0x04u
>  #define ___GFP_MOVABLE   0x08u
>  #define ___GFP_RECLAIMABLE   0x10u

Anyway, thanks for your review!
-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Tue 07-03-17 15:08:45, Andrew Morton wrote:
> On Tue,  7 Mar 2017 15:10:20 +0100 Michal Hocko  wrote:
> 
> > __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.
> 
> hm.  What happens if a caller wants only lowmem pages?  Drivers do
> weird stuff...
 
Yes they do and we have vmalloc_32 API which works as intended because
GFP_VMALLOC32 contains GFP_DMA32 and that will override __GFP_HIGHMEM.

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Tue 07-03-17 15:08:45, Andrew Morton wrote:
> On Tue,  7 Mar 2017 15:10:20 +0100 Michal Hocko  wrote:
> 
> > __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.
> 
> hm.  What happens if a caller wants only lowmem pages?  Drivers do
> weird stuff...
 
Yes they do and we have vmalloc_32 API which works as intended because
GFP_VMALLOC32 contains GFP_DMA32 and that will override __GFP_HIGHMEM.

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Wed 08-03-17 08:33:58, Vlastimil Babka wrote:
> On 03/07/2017 03:10 PM, Michal Hocko wrote:
[...]
> > 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 :)

yeah, that would be for a separate patch(es).

[...]
> > 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()?

I would really prefer to check whether GFP_NOFS is really needed here
and if yes then place memalloc_nofs_save where the locking really
requires it so this would become plan vmalloc as a side effect
 
> > 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()?

after some code moving in mm/nommu.c yes. But I am not sure this is a
huge win

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Wed 08-03-17 08:33:58, Vlastimil Babka wrote:
> On 03/07/2017 03:10 PM, Michal Hocko wrote:
[...]
> > 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 :)

yeah, that would be for a separate patch(es).

[...]
> > 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()?

I would really prefer to check whether GFP_NOFS is really needed here
and if yes then place memalloc_nofs_save where the locking really
requires it so this would become plan vmalloc as a side effect
 
> > 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()?

after some code moving in mm/nommu.c yes. But I am not sure this is a
huge win

-- 
Michal Hocko
SUSE Labs

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

[Vlastimil Babka]

On 03/07/2017 03:10 PM, Michal Hocko wrote:
> From: Michal Hocko 
> 
> __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 
> ---
> 
> 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-mho...@kernel.org
> 
>  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, 

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

[Vlastimil Babka]

On 03/07/2017 03:10 PM, Michal Hocko wrote:
> From: Michal Hocko 
> 
> __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 
> ---
> 
> 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-mho...@kernel.org
> 
>  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 |
> -   

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

[Vlastimil Babka]

On 03/07/2017 07:57 PM, Matthew Wilcox wrote:
> On Tue, Mar 07, 2017 at 10:28:41AM -0800, Matthew Wilcox wrote:
>> On Tue, Mar 07, 2017 at 03:10:20PM +0100, Michal Hocko wrote:
>>> 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.
> 
> btw, I had another idea for GFP_HIGHMEM -- remove it when CONFIG_HIGHMEM
> isn't enabled.  Saves 26 bytes of .text and 64 bytes of .data on my
> laptop's kernel build.  What do you think?
> 
> Also, I suspect the layout of bits is suboptimal from an assembly
> language perspective.  I still mostly care about x86 which doesn't
> benefit, so I'm not inclined to do the work, but certainly ARM, PA-RISC,
> SPARC and Itanium would all benefit from having frequently-used bits
> (ie those used in GFP_KERNEL and GFP_ATOMIC) placed in the low 8 bits.
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 0fe0b6295ab5..d88cb532d7c8 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -16,7 +16,11 @@ struct vm_area_struct;
>  
>  /* Plain integer GFP bitmasks. Do not use this directly. */
>  #define ___GFP_DMA   0x01u
> +#ifdef CONFIG_HIGHMEM
>  #define ___GFP_HIGHMEM   0x02u
> +#else
> +#define ___GFP_HIGHMEM   0x0u

Make sure you don't break the users of __def_gfpflag_names e.g.
format_flags(). IIRC zero is a terminator in the table.

But the savings don't seem to be worth the trouble.

> +#endif
>  #define ___GFP_DMA32 0x04u
>  #define ___GFP_MOVABLE   0x08u
>  #define ___GFP_RECLAIMABLE   0x10u
> 

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

[Vlastimil Babka]

On 03/07/2017 07:57 PM, Matthew Wilcox wrote:
> On Tue, Mar 07, 2017 at 10:28:41AM -0800, Matthew Wilcox wrote:
>> On Tue, Mar 07, 2017 at 03:10:20PM +0100, Michal Hocko wrote:
>>> 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.
> 
> btw, I had another idea for GFP_HIGHMEM -- remove it when CONFIG_HIGHMEM
> isn't enabled.  Saves 26 bytes of .text and 64 bytes of .data on my
> laptop's kernel build.  What do you think?
> 
> Also, I suspect the layout of bits is suboptimal from an assembly
> language perspective.  I still mostly care about x86 which doesn't
> benefit, so I'm not inclined to do the work, but certainly ARM, PA-RISC,
> SPARC and Itanium would all benefit from having frequently-used bits
> (ie those used in GFP_KERNEL and GFP_ATOMIC) placed in the low 8 bits.
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 0fe0b6295ab5..d88cb532d7c8 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -16,7 +16,11 @@ struct vm_area_struct;
>  
>  /* Plain integer GFP bitmasks. Do not use this directly. */
>  #define ___GFP_DMA   0x01u
> +#ifdef CONFIG_HIGHMEM
>  #define ___GFP_HIGHMEM   0x02u
> +#else
> +#define ___GFP_HIGHMEM   0x0u

Make sure you don't break the users of __def_gfpflag_names e.g.
format_flags(). IIRC zero is a terminator in the table.

But the savings don't seem to be worth the trouble.

> +#endif
>  #define ___GFP_DMA32 0x04u
>  #define ___GFP_MOVABLE   0x08u
>  #define ___GFP_RECLAIMABLE   0x10u
> 

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

[Matthew Wilcox]

On Tue, Mar 07, 2017 at 03:08:45PM -0800, Andrew Morton wrote:
> On Tue,  7 Mar 2017 15:10:20 +0100 Michal Hocko  wrote:
> 
> > __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.
> 
> hm.  What happens if a caller wants only lowmem pages?  Drivers do
> weird stuff...

That's not something drivers actually want ... they might want "only pages
under 4GB", which is why we have vmalloc_32(), but drivers don't really
care where the HIGHMEM / LOWMEM split is.  I suppose we might find some
cases where drivers have mistakenly used vmalloc() and "got away with it".

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

[Matthew Wilcox]

On Tue, Mar 07, 2017 at 03:08:45PM -0800, Andrew Morton wrote:
> On Tue,  7 Mar 2017 15:10:20 +0100 Michal Hocko  wrote:
> 
> > __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.
> 
> hm.  What happens if a caller wants only lowmem pages?  Drivers do
> weird stuff...

That's not something drivers actually want ... they might want "only pages
under 4GB", which is why we have vmalloc_32(), but drivers don't really
care where the HIGHMEM / LOWMEM split is.  I suppose we might find some
cases where drivers have mistakenly used vmalloc() and "got away with it".

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

[Andrew Morton]

On Tue,  7 Mar 2017 15:10:20 +0100 Michal Hocko  wrote:

> __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.

hm.  What happens if a caller wants only lowmem pages?  Drivers do
weird stuff...

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

[Andrew Morton]

On Tue,  7 Mar 2017 15:10:20 +0100 Michal Hocko  wrote:

> __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.

hm.  What happens if a caller wants only lowmem pages?  Drivers do
weird stuff...

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

[Matthew Wilcox]

On Tue, Mar 07, 2017 at 10:28:41AM -0800, Matthew Wilcox wrote:
> On Tue, Mar 07, 2017 at 03:10:20PM +0100, Michal Hocko wrote:
> > 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.

btw, I had another idea for GFP_HIGHMEM -- remove it when CONFIG_HIGHMEM
isn't enabled.  Saves 26 bytes of .text and 64 bytes of .data on my
laptop's kernel build.  What do you think?

Also, I suspect the layout of bits is suboptimal from an assembly
language perspective.  I still mostly care about x86 which doesn't
benefit, so I'm not inclined to do the work, but certainly ARM, PA-RISC,
SPARC and Itanium would all benefit from having frequently-used bits
(ie those used in GFP_KERNEL and GFP_ATOMIC) placed in the low 8 bits.

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 0fe0b6295ab5..d88cb532d7c8 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -16,7 +16,11 @@ struct vm_area_struct;
 
 /* Plain integer GFP bitmasks. Do not use this directly. */
 #define ___GFP_DMA 0x01u
+#ifdef CONFIG_HIGHMEM
 #define ___GFP_HIGHMEM 0x02u
+#else
+#define ___GFP_HIGHMEM 0x0u
+#endif
 #define ___GFP_DMA32   0x04u
 #define ___GFP_MOVABLE 0x08u
 #define ___GFP_RECLAIMABLE 0x10u

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

[Matthew Wilcox]

On Tue, Mar 07, 2017 at 10:28:41AM -0800, Matthew Wilcox wrote:
> On Tue, Mar 07, 2017 at 03:10:20PM +0100, Michal Hocko wrote:
> > 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.

btw, I had another idea for GFP_HIGHMEM -- remove it when CONFIG_HIGHMEM
isn't enabled.  Saves 26 bytes of .text and 64 bytes of .data on my
laptop's kernel build.  What do you think?

Also, I suspect the layout of bits is suboptimal from an assembly
language perspective.  I still mostly care about x86 which doesn't
benefit, so I'm not inclined to do the work, but certainly ARM, PA-RISC,
SPARC and Itanium would all benefit from having frequently-used bits
(ie those used in GFP_KERNEL and GFP_ATOMIC) placed in the low 8 bits.

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 0fe0b6295ab5..d88cb532d7c8 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -16,7 +16,11 @@ struct vm_area_struct;
 
 /* Plain integer GFP bitmasks. Do not use this directly. */
 #define ___GFP_DMA 0x01u
+#ifdef CONFIG_HIGHMEM
 #define ___GFP_HIGHMEM 0x02u
+#else
+#define ___GFP_HIGHMEM 0x0u
+#endif
 #define ___GFP_DMA32   0x04u
 #define ___GFP_MOVABLE 0x08u
 #define ___GFP_RECLAIMABLE 0x10u

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

[Matthew Wilcox]

On Tue, Mar 07, 2017 at 03:10:20PM +0100, Michal Hocko wrote:
> From: Michal Hocko 
> 
> __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 

Reviewed-by: Matthew Wilcox 

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

[Matthew Wilcox]

On Tue, Mar 07, 2017 at 03:10:20PM +0100, Michal Hocko wrote:
> From: Michal Hocko 
> 
> __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 

Reviewed-by: Matthew Wilcox