Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1375691 > unrolled thread

[PATCH 0/19] get rid of superfluous __GFP_REPORT

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/19] get rid of superfluous __GFP_REPORT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:10 +0200
    [PATCH 15/19] tile: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:10 +0200
    [PATCH 18/19] crypto: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:10 +0200
      Re: [PATCH 18/19] crypto: get rid of superfluous __GFP_REPEAT Herbert Xu <herbert@gondor.apana.org.au> - 2016-04-14 08:30 +0200
        Re: [PATCH 18/19] crypto: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-14 09:10 +0200
          Re: [PATCH 18/19] crypto: get rid of superfluous __GFP_REPEAT Herbert Xu <herbert@gondor.apana.org.au> - 2016-04-14 10:20 +0200
      [PATCH resend] crypto: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-14 11:00 +0200
        Re: [PATCH resend] crypto: get rid of superfluous __GFP_REPEAT Herbert Xu <herbert@gondor.apana.org.au> - 2016-04-15 16:40 +0200
    [PATCH 09/19] parisc: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:10 +0200
    [PATCH 16/19] unicore32: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:10 +0200
    [PATCH 08/19] nios2: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
    [PATCH 01/19] tree wide: get rid of __GFP_REPEAT for order-0 allocations part I Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
      Re: [PATCH 01/19] tree wide: get rid of __GFP_REPEAT for order-0  allocations part I David Rientjes <rientjes@google.com> - 2016-04-14 22:00 +0200
        Re: [PATCH 01/19] tree wide: get rid of __GFP_REPEAT for order-0  allocations part I Michal Hocko <mhocko@kernel.org> - 2016-04-15 09:50 +0200
    [PATCH 06/19] arc: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
      Re: [PATCH 06/19] arc: get rid of superfluous __GFP_REPEAT Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-04-11 16:30 +0200
    [PATCH 11/19] powerpc: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
    [PATCH 10/19] score: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
    [PATCH 07/19] mips: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
    [PATCH 19/19] jbd2: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
    [PATCH 13/19] s390: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
      Re: [PATCH 13/19] s390: get rid of superfluous __GFP_REPEAT Cornelia Huck <cornelia.huck@de.ibm.com> - 2016-04-11 13:30 +0200
        Re: [PATCH 13/19] s390: get rid of superfluous __GFP_REPEAT Heiko Carstens <heiko.carstens@de.ibm.com> - 2016-04-11 14:50 +0200
    [PATCH 12/19] sparc: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
    [PATCH 04/19] arm: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
    [PATCH 03/19] x86/efi: get rid of superfluous __GFP_REPEAT Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
      Re: [PATCH 03/19] x86/efi: get rid of superfluous __GFP_REPEAT Matt Fleming <matt@codeblueprint.co.uk> - 2016-04-12 18:00 +0200
    [PATCH 17/19] dm: get rid of superfluous gfp flags Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:20 +0200
      Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Mikulas Patocka <mpatocka@redhat.com> - 2016-04-15 14:30 +0200
        Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Michal Hocko <mhocko@kernel.org> - 2016-04-15 15:10 +0200
          Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Mikulas Patocka <mpatocka@redhat.com> - 2016-04-15 20:50 +0200
            Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Michal Hocko <mhocko@kernel.org> - 2016-04-16 22:40 +0200
              Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Michal Hocko <mhocko@kernel.org> - 2016-04-22 14:50 +0200
                Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Mikulas Patocka <mpatocka@redhat.com> - 2016-04-26 19:30 +0200
                  Re: [PATCH 17/19] dm: get rid of superfluous gfp flags Michal Hocko <mhocko@kernel.org> - 2016-04-27 10:40 +0200
    CC in git cover letter vs patches (was Re: [PATCH 0/19] get rid of  superfluous __GFP_REPORT) Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-04-13 13:30 +0200
      Re: CC in git cover letter vs patches (was Re: [PATCH 0/19] get rid  of superfluous __GFP_REPORT) Michal Hocko <mhocko@kernel.org> - 2016-04-13 15:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1375711 — [PATCH 13/19] s390: get rid of superfluous __GFP_REPEAT

From: Michal Hocko <mhocko@suse.com>

__GFP_REPEAT has a rather weak semantic but since it has been introduced
around 2.6.12 it has been ignored for low order allocations.

arch_dup_task_struct uses __GFP_REPEAT for fpu_regs_size which is either
sizeof(__vector128) * __NUM_VXRS = 4069B resp.
sizeof(freg_t) * __NUM_FPRS = 1024B AFAICS. page_table_alloc then uses
the flag for a single page allocation. This means that this flag has
never been actually useful here because it has always been used only for
PAGE_ALLOC_COSTLY requests.

Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: linux-arch@vger.kernel.org
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 arch/s390/kernel/process.c | 2 +-
 arch/s390/mm/pgalloc.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
index f8e79824e284..1837a1901d4b 100644
--- a/arch/s390/kernel/process.c
+++ b/arch/s390/kernel/process.c
@@ -102,7 +102,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 	 */
 	fpu_regs_size = MACHINE_HAS_VX ? sizeof(__vector128) * __NUM_VXRS
 				       : sizeof(freg_t) * __NUM_FPRS;
-	dst->thread.fpu.regs = kzalloc(fpu_regs_size, GFP_KERNEL|__GFP_REPEAT);
+	dst->thread.fpu.regs = kzalloc(fpu_regs_size, GFP_KERNEL);
 	if (!dst->thread.fpu.regs)
 		return -ENOMEM;
 
diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
index f6c3de26cda8..3f716741797a 100644
--- a/arch/s390/mm/pgalloc.c
+++ b/arch/s390/mm/pgalloc.c
@@ -198,7 +198,7 @@ unsigned long *page_table_alloc(struct mm_struct *mm)
 			return table;
 	}
 	/* Allocate a fresh page */
-	page = alloc_page(GFP_KERNEL|__GFP_REPEAT);
+	page = alloc_page(GFP_KERNEL);
 	if (!page)
 		return NULL;
 	if (!pgtable_page_ctor(page)) {
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1375723 — Re: [PATCH 13/19] s390: get rid of superfluous __GFP_REPEAT

On Mon, 11 Apr 2016 13:08:06 +0200
Michal Hocko <mhocko@kernel.org> wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> __GFP_REPEAT has a rather weak semantic but since it has been introduced
> around 2.6.12 it has been ignored for low order allocations.
> 
> arch_dup_task_struct uses __GFP_REPEAT for fpu_regs_size which is either
> sizeof(__vector128) * __NUM_VXRS = 4069B resp.
> sizeof(freg_t) * __NUM_FPRS = 1024B AFAICS. page_table_alloc then uses
> the flag for a single page allocation. This means that this flag has
> never been actually useful here because it has always been used only for
> PAGE_ALLOC_COSTLY requests.
> 
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>

Let's cc: Martin/Heiko instead :)

> Cc: linux-arch@vger.kernel.org
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  arch/s390/kernel/process.c | 2 +-
>  arch/s390/mm/pgalloc.c     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
> index f8e79824e284..1837a1901d4b 100644
> --- a/arch/s390/kernel/process.c
> +++ b/arch/s390/kernel/process.c
> @@ -102,7 +102,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
>  	 */
>  	fpu_regs_size = MACHINE_HAS_VX ? sizeof(__vector128) * __NUM_VXRS
>  				       : sizeof(freg_t) * __NUM_FPRS;
> -	dst->thread.fpu.regs = kzalloc(fpu_regs_size, GFP_KERNEL|__GFP_REPEAT);
> +	dst->thread.fpu.regs = kzalloc(fpu_regs_size, GFP_KERNEL);
>  	if (!dst->thread.fpu.regs)
>  		return -ENOMEM;
> 
> diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
> index f6c3de26cda8..3f716741797a 100644
> --- a/arch/s390/mm/pgalloc.c
> +++ b/arch/s390/mm/pgalloc.c
> @@ -198,7 +198,7 @@ unsigned long *page_table_alloc(struct mm_struct *mm)
>  			return table;
>  	}
>  	/* Allocate a fresh page */
> -	page = alloc_page(GFP_KERNEL|__GFP_REPEAT);
> +	page = alloc_page(GFP_KERNEL);
>  	if (!page)
>  		return NULL;
>  	if (!pgtable_page_ctor(page)) {

[toc] | [prev] | [next] | [standalone]

#1375795 — Re: [PATCH 13/19] s390: get rid of superfluous __GFP_REPEAT

On Mon, Apr 11, 2016 at 01:28:37PM +0200, Cornelia Huck wrote:
> On Mon, 11 Apr 2016 13:08:06 +0200
> Michal Hocko <mhocko@kernel.org> wrote:
> 
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > __GFP_REPEAT has a rather weak semantic but since it has been introduced
> > around 2.6.12 it has been ignored for low order allocations.
> > 
> > arch_dup_task_struct uses __GFP_REPEAT for fpu_regs_size which is either
> > sizeof(__vector128) * __NUM_VXRS = 4069B resp.
> > sizeof(freg_t) * __NUM_FPRS = 1024B AFAICS. page_table_alloc then uses
> > the flag for a single page allocation. This means that this flag has
> > never been actually useful here because it has always been used only for
> > PAGE_ALLOC_COSTLY requests.
> > 
> > Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> > Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> Let's cc: Martin/Heiko instead :)
> 
> > Cc: linux-arch@vger.kernel.org
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> >  arch/s390/kernel/process.c | 2 +-
> >  arch/s390/mm/pgalloc.c     | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)

Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>

[toc] | [prev] | [next] | [standalone]

#1375712 — [PATCH 12/19] sparc: get rid of superfluous __GFP_REPEAT

From: Michal Hocko <mhocko@suse.com>

__GFP_REPEAT has a rather weak semantic but since it has been introduced
around 2.6.12 it has been ignored for low order allocations.

{pud,pmd}_alloc_one is using __GFP_REPEAT but it always allocates from
pgtable_cache which is initialzed to PAGE_SIZE objects. This means that
this flag has never been actually useful here because it has always been
used only for PAGE_ALLOC_COSTLY requests.

Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-arch@vger.kernel.org
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 arch/sparc/include/asm/pgalloc_64.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/sparc/include/asm/pgalloc_64.h b/arch/sparc/include/asm/pgalloc_64.h
index 5e3187185b4a..3529f1378cd8 100644
--- a/arch/sparc/include/asm/pgalloc_64.h
+++ b/arch/sparc/include/asm/pgalloc_64.h
@@ -41,8 +41,7 @@ static inline void __pud_populate(pud_t *pud, pmd_t *pmd)
 
 static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long addr)
 {
-	return kmem_cache_alloc(pgtable_cache,
-				GFP_KERNEL|__GFP_REPEAT);
+	return kmem_cache_alloc(pgtable_cache, GFP_KERNEL);
 }
 
 static inline void pud_free(struct mm_struct *mm, pud_t *pud)
@@ -52,8 +51,7 @@ static inline void pud_free(struct mm_struct *mm, pud_t *pud)
 
 static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long addr)
 {
-	return kmem_cache_alloc(pgtable_cache,
-				GFP_KERNEL|__GFP_REPEAT);
+	return kmem_cache_alloc(pgtable_cache, GFP_KERNEL);
 }
 
 static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1375713 — [PATCH 04/19] arm: get rid of superfluous __GFP_REPEAT

From: Michal Hocko <mhocko@suse.com>

__GFP_REPEAT has a rather weak semantic but since it has been introduced
around 2.6.12 it has been ignored for low order allocations.

PGALLOC_GFP uses __GFP_REPEAT but none of the allocation which uses
this flag is for more than order-2. This means that this flag has never
been actually useful here because it has always been used only for
PAGE_ALLOC_COSTLY requests.

Cc: Russell King <linux@arm.linux.org.uk>
Cc: linux-arch@vger.kernel.org
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 arch/arm/include/asm/pgalloc.h | 2 +-
 arch/arm/mm/pgd.c              | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/pgalloc.h b/arch/arm/include/asm/pgalloc.h
index 20febb368844..b2902a5cd780 100644
--- a/arch/arm/include/asm/pgalloc.h
+++ b/arch/arm/include/asm/pgalloc.h
@@ -57,7 +57,7 @@ static inline void pud_populate(struct mm_struct *mm, pud_t *pud, pmd_t *pmd)
 extern pgd_t *pgd_alloc(struct mm_struct *mm);
 extern void pgd_free(struct mm_struct *mm, pgd_t *pgd);
 
-#define PGALLOC_GFP	(GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO)
+#define PGALLOC_GFP	(GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO)
 
 static inline void clean_pte_table(pte_t *pte)
 {
diff --git a/arch/arm/mm/pgd.c b/arch/arm/mm/pgd.c
index b8d477321730..c1c1a5c67da1 100644
--- a/arch/arm/mm/pgd.c
+++ b/arch/arm/mm/pgd.c
@@ -23,7 +23,7 @@
 #define __pgd_alloc()	kmalloc(PTRS_PER_PGD * sizeof(pgd_t), GFP_KERNEL)
 #define __pgd_free(pgd)	kfree(pgd)
 #else
-#define __pgd_alloc()	(pgd_t *)__get_free_pages(GFP_KERNEL | __GFP_REPEAT, 2)
+#define __pgd_alloc()	(pgd_t *)__get_free_pages(GFP_KERNEL, 2)
 #define __pgd_free(pgd)	free_pages((unsigned long)pgd, 2)
 #endif
 
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1375714 — [PATCH 03/19] x86/efi: get rid of superfluous __GFP_REPEAT

From: Michal Hocko <mhocko@suse.com>

__GFP_REPEAT has a rather weak semantic but since it has been introduced
around 2.6.12 it has been ignored for low order allocations.

efi_alloc_page_tables uses __GFP_REPEAT but it allocates an order-0
page. This means that this flag has never been actually useful here
because it has always been used only for PAGE_ALLOC_COSTLY requests.

Cc: Matt Fleming <matt@codeblueprint.co.uk>
Cc: linux-arch@vger.kernel.org
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 arch/x86/platform/efi/efi_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 49e4dd4a1f58..a7ee3f08074f 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -141,7 +141,7 @@ int __init efi_alloc_page_tables(void)
 	if (efi_enabled(EFI_OLD_MEMMAP))
 		return 0;
 
-	gfp_mask = GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO;
+	gfp_mask = GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO;
 	efi_pgd = (pgd_t *)__get_free_page(gfp_mask);
 	if (!efi_pgd)
 		return -ENOMEM;
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1376988 — Re: [PATCH 03/19] x86/efi: get rid of superfluous __GFP_REPEAT

On Mon, 11 Apr, at 01:07:56PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> __GFP_REPEAT has a rather weak semantic but since it has been introduced
> around 2.6.12 it has been ignored for low order allocations.
> 
> efi_alloc_page_tables uses __GFP_REPEAT but it allocates an order-0
> page. This means that this flag has never been actually useful here
> because it has always been used only for PAGE_ALLOC_COSTLY requests.
> 
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Cc: linux-arch@vger.kernel.org
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  arch/x86/platform/efi/efi_64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks fine. I suspect I copied it from other pgtable creation code,

Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>

[toc] | [prev] | [next] | [standalone]

#1375716 — [PATCH 17/19] dm: get rid of superfluous gfp flags

From: Michal Hocko <mhocko@suse.com>

copy_params seems to be little bit confused about which allocation flags
to use. It enforces GFP_NOIO even though it uses
memalloc_noio_{save,restore} which enforces GFP_NOIO at the page
allocator level automatically (via memalloc_noio_flags). It also
uses __GFP_REPEAT for the __vmalloc request which doesn't make much
sense either because vmalloc doesn't rely on costly high order
allocations.

Cc: Shaohua Li <shli@kernel.org>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 drivers/md/dm-ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 2adf81d81fca..dfe629a294e1 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1723,7 +1723,7 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 	if (!dmi) {
 		unsigned noio_flag;
 		noio_flag = memalloc_noio_save();
-		dmi = __vmalloc(param_kernel->data_size, GFP_NOIO | __GFP_REPEAT | __GFP_HIGH | __GFP_HIGHMEM, PAGE_KERNEL);
+		dmi = __vmalloc(param_kernel->data_size, __GFP_HIGH | __GFP_HIGHMEM, PAGE_KERNEL);
 		memalloc_noio_restore(noio_flag);
 		if (dmi)
 			*param_flags |= DM_PARAMS_VMALLOC;
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1379784 — Re: [PATCH 17/19] dm: get rid of superfluous gfp flags

On Mon, 11 Apr 2016, Michal Hocko wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> copy_params seems to be little bit confused about which allocation flags
> to use. It enforces GFP_NOIO even though it uses
> memalloc_noio_{save,restore} which enforces GFP_NOIO at the page

memalloc_noio_{save,restore} is used because __vmalloc is flawed and 
doesn't respect GFP_NOIO properly (it doesn't use gfp flags when 
allocating pagetables).

The proper fix it to correct __vmalloc (though, it would require change to 
pagetable allocation routine on all architectures), not to remove GFP_NOIO 
from __vmalloc.

Mikulas

> allocator level automatically (via memalloc_noio_flags). It also
> uses __GFP_REPEAT for the __vmalloc request which doesn't make much
> sense either because vmalloc doesn't rely on costly high order
> allocations.
> 
> Cc: Shaohua Li <shli@kernel.org>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  drivers/md/dm-ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 2adf81d81fca..dfe629a294e1 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1723,7 +1723,7 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
>  	if (!dmi) {
>  		unsigned noio_flag;
>  		noio_flag = memalloc_noio_save();
> -		dmi = __vmalloc(param_kernel->data_size, GFP_NOIO | __GFP_REPEAT | __GFP_HIGH | __GFP_HIGHMEM, PAGE_KERNEL);
> +		dmi = __vmalloc(param_kernel->data_size, __GFP_HIGH | __GFP_HIGHMEM, PAGE_KERNEL);
>  		memalloc_noio_restore(noio_flag);
>  		if (dmi)
>  			*param_flags |= DM_PARAMS_VMALLOC;
> -- 
> 2.8.0.rc3
> 

[toc] | [prev] | [next] | [standalone]

#1379804 — Re: [PATCH 17/19] dm: get rid of superfluous gfp flags

On Fri 15-04-16 08:29:28, Mikulas Patocka wrote:
> 
> 
> On Mon, 11 Apr 2016, Michal Hocko wrote:
> 
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > copy_params seems to be little bit confused about which allocation flags
> > to use. It enforces GFP_NOIO even though it uses
> > memalloc_noio_{save,restore} which enforces GFP_NOIO at the page
> 
> memalloc_noio_{save,restore} is used because __vmalloc is flawed and 
> doesn't respect GFP_NOIO properly (it doesn't use gfp flags when 
> allocating pagetables).

Yes and there are no plans to change __vmalloc to properly propagate gfp
flags through the whole call chain and that is why we have
memalloc_noio thingy. If that ever changes later the GFP_NOIO can be
added in favor of memalloc_noio API. Both are clearly redundant.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1380114 — Re: [PATCH 17/19] dm: get rid of superfluous gfp flags

On Fri, 15 Apr 2016, Michal Hocko wrote:

> On Fri 15-04-16 08:29:28, Mikulas Patocka wrote:
> > 
> > 
> > On Mon, 11 Apr 2016, Michal Hocko wrote:
> > 
> > > From: Michal Hocko <mhocko@suse.com>
> > > 
> > > copy_params seems to be little bit confused about which allocation flags
> > > to use. It enforces GFP_NOIO even though it uses
> > > memalloc_noio_{save,restore} which enforces GFP_NOIO at the page
> > 
> > memalloc_noio_{save,restore} is used because __vmalloc is flawed and 
> > doesn't respect GFP_NOIO properly (it doesn't use gfp flags when 
> > allocating pagetables).
> 
> Yes and there are no plans to change __vmalloc to properly propagate gfp
> flags through the whole call chain and that is why we have
> memalloc_noio thingy. If that ever changes later the GFP_NOIO can be
> added in favor of memalloc_noio API. Both are clearly redundant.
> -- 
> Michal Hocko
> SUSE Labs

You could move memalloc_noio_{save,restore} to __vmalloc. Something like

if (!(gfp_mask & __GFP_IO))
	noio_flag = memalloc_noio_save();
...
if (!(gfp_mask & __GFP_IO))
	memalloc_noio_restore(noio_flag);

That would be better than repeating this hack in every __vmalloc caller 
that need GFP_NOIO.

Mikulas

[toc] | [prev] | [next] | [standalone]

#1380619 — Re: [PATCH 17/19] dm: get rid of superfluous gfp flags

On Fri 15-04-16 14:41:29, Mikulas Patocka wrote:
> 
> 
> On Fri, 15 Apr 2016, Michal Hocko wrote:
> 
> > On Fri 15-04-16 08:29:28, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Mon, 11 Apr 2016, Michal Hocko wrote:
> > > 
> > > > From: Michal Hocko <mhocko@suse.com>
> > > > 
> > > > copy_params seems to be little bit confused about which allocation flags
> > > > to use. It enforces GFP_NOIO even though it uses
> > > > memalloc_noio_{save,restore} which enforces GFP_NOIO at the page
> > > 
> > > memalloc_noio_{save,restore} is used because __vmalloc is flawed and 
> > > doesn't respect GFP_NOIO properly (it doesn't use gfp flags when 
> > > allocating pagetables).
> > 
> > Yes and there are no plans to change __vmalloc to properly propagate gfp
> > flags through the whole call chain and that is why we have
> > memalloc_noio thingy. If that ever changes later the GFP_NOIO can be
> > added in favor of memalloc_noio API. Both are clearly redundant.
> > -- 
> > Michal Hocko
> > SUSE Labs
> 
> You could move memalloc_noio_{save,restore} to __vmalloc. Something like
> 
> if (!(gfp_mask & __GFP_IO))
> 	noio_flag = memalloc_noio_save();
> ...
> if (!(gfp_mask & __GFP_IO))
> 	memalloc_noio_restore(noio_flag);
> 
> That would be better than repeating this hack in every __vmalloc caller 
> that need GFP_NOIO.

It is not my intention to change __vmalloc behavior. If you strongly
oppose the GFP_NOIO change I can drop it from the patch. It is
__GFP_REPEAT which I am after.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1385129 — Re: [PATCH 17/19] dm: get rid of superfluous gfp flags

On Sat 16-04-16 16:31:35, Michal Hocko wrote:
> On Fri 15-04-16 14:41:29, Mikulas Patocka wrote:
> > 
> > 
> > On Fri, 15 Apr 2016, Michal Hocko wrote:
> > 
> > > On Fri 15-04-16 08:29:28, Mikulas Patocka wrote:
> > > > 
> > > > 
> > > > On Mon, 11 Apr 2016, Michal Hocko wrote:
> > > > 
> > > > > From: Michal Hocko <mhocko@suse.com>
> > > > > 
> > > > > copy_params seems to be little bit confused about which allocation flags
> > > > > to use. It enforces GFP_NOIO even though it uses
> > > > > memalloc_noio_{save,restore} which enforces GFP_NOIO at the page
> > > > 
> > > > memalloc_noio_{save,restore} is used because __vmalloc is flawed and 
> > > > doesn't respect GFP_NOIO properly (it doesn't use gfp flags when 
> > > > allocating pagetables).
> > > 
> > > Yes and there are no plans to change __vmalloc to properly propagate gfp
> > > flags through the whole call chain and that is why we have
> > > memalloc_noio thingy. If that ever changes later the GFP_NOIO can be
> > > added in favor of memalloc_noio API. Both are clearly redundant.
> > > -- 
> > > Michal Hocko
> > > SUSE Labs
> > 
> > You could move memalloc_noio_{save,restore} to __vmalloc. Something like
> > 
> > if (!(gfp_mask & __GFP_IO))
> > 	noio_flag = memalloc_noio_save();
> > ...
> > if (!(gfp_mask & __GFP_IO))
> > 	memalloc_noio_restore(noio_flag);
> > 
> > That would be better than repeating this hack in every __vmalloc caller 
> > that need GFP_NOIO.
> 
> It is not my intention to change __vmalloc behavior. If you strongly
> oppose the GFP_NOIO change I can drop it from the patch. It is
> __GFP_REPEAT which I am after.

I am dropping the GFP_NOIO part for this patch but now that I am looking
into the code more closely I completely fail why it is needed in the
first place.

copy_params seems to be called only from the ioctl context which doesn't
hold any locks which would lockup during the direct reclaim AFAICS. The
git log shows that the code has used PF_MEMALLOC before which is even
bigger mystery to me. Could you please clarify why this is GFP_NOIO
restricted context? Maybe it needed to be in the past but I do not see
any reason for it to be now so unless I am missing something the
GFP_KERNEL should be perfectly OK. Also note that GFP_NOIO wouldn't work
properly because there are copy_from_user calls in the same path which
could page fault and do GFP_KERNEL allocations anyway. I can send follow
up cleanups unless I am missing something subtle here.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1387743 — Re: [PATCH 17/19] dm: get rid of superfluous gfp flags

On Fri, 22 Apr 2016, Michal Hocko wrote:

> On Sat 16-04-16 16:31:35, Michal Hocko wrote:
> > On Fri 15-04-16 14:41:29, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Fri, 15 Apr 2016, Michal Hocko wrote:
> > > 
> > > > On Fri 15-04-16 08:29:28, Mikulas Patocka wrote:
> > > > > 
> > > > > 
> > > > > On Mon, 11 Apr 2016, Michal Hocko wrote:
> > > > > 
> > > > > > From: Michal Hocko <mhocko@suse.com>
> > > > > > 
> > > > > > copy_params seems to be little bit confused about which allocation flags
> > > > > > to use. It enforces GFP_NOIO even though it uses
> > > > > > memalloc_noio_{save,restore} which enforces GFP_NOIO at the page
> > > > > 
> > > > > memalloc_noio_{save,restore} is used because __vmalloc is flawed and 
> > > > > doesn't respect GFP_NOIO properly (it doesn't use gfp flags when 
> > > > > allocating pagetables).
> > > > 
> > > > Yes and there are no plans to change __vmalloc to properly propagate gfp
> > > > flags through the whole call chain and that is why we have
> > > > memalloc_noio thingy. If that ever changes later the GFP_NOIO can be
> > > > added in favor of memalloc_noio API. Both are clearly redundant.
> > > > -- 
> > > > Michal Hocko
> > > > SUSE Labs
> > > 
> > > You could move memalloc_noio_{save,restore} to __vmalloc. Something like
> > > 
> > > if (!(gfp_mask & __GFP_IO))
> > > 	noio_flag = memalloc_noio_save();
> > > ...
> > > if (!(gfp_mask & __GFP_IO))
> > > 	memalloc_noio_restore(noio_flag);
> > > 
> > > That would be better than repeating this hack in every __vmalloc caller 
> > > that need GFP_NOIO.
> > 
> > It is not my intention to change __vmalloc behavior. If you strongly
> > oppose the GFP_NOIO change I can drop it from the patch. It is
> > __GFP_REPEAT which I am after.
> 
> I am dropping the GFP_NOIO part for this patch but now that I am looking
> into the code more closely I completely fail why it is needed in the
> first place.
> 
> copy_params seems to be called only from the ioctl context which doesn't
> hold any locks which would lockup during the direct reclaim AFAICS. The
> git log shows that the code has used PF_MEMALLOC before which is even
> bigger mystery to me. Could you please clarify why this is GFP_NOIO
> restricted context? Maybe it needed to be in the past but I do not see
> any reason for it to be now so unless I am missing something the
> GFP_KERNEL should be perfectly OK. Also note that GFP_NOIO wouldn't work
> properly because there are copy_from_user calls in the same path which
> could page fault and do GFP_KERNEL allocations anyway. I can send follow
> up cleanups unless I am missing something subtle here.
> -- 
> Michal Hocko
> SUSE Labs

The LVM tool calls suspend and resume ioctls on device mapper block 
devices.

When a device is suspended, any bio sent to the device is held. If the 
resume ioctl did GFP_KERNEL allocation, the allocation could get stuck 
trying to write some dirty cached pages to the suspended device.

The LVM tool and the dmeventd daemon use mlock to lock its address space, 
so the copy_from_user/copy_to_user call cannot trigger a page fault.

Mikulas

[toc] | [prev] | [next] | [standalone]

#1388784 — Re: [PATCH 17/19] dm: get rid of superfluous gfp flags

[Adding dm-devel@redhat.com to CC]

On Tue 26-04-16 13:20:04, Mikulas Patocka wrote:
> On Fri, 22 Apr 2016, Michal Hocko wrote:
[...]
> > copy_params seems to be called only from the ioctl context which doesn't
> > hold any locks which would lockup during the direct reclaim AFAICS. The
> > git log shows that the code has used PF_MEMALLOC before which is even
> > bigger mystery to me. Could you please clarify why this is GFP_NOIO
> > restricted context? Maybe it needed to be in the past but I do not see
> > any reason for it to be now so unless I am missing something the
> > GFP_KERNEL should be perfectly OK. Also note that GFP_NOIO wouldn't work
> > properly because there are copy_from_user calls in the same path which
> > could page fault and do GFP_KERNEL allocations anyway. I can send follow
> > up cleanups unless I am missing something subtle here.
> 
> The LVM tool calls suspend and resume ioctls on device mapper block 
> devices.
>
> When a device is suspended, any bio sent to the device is held. If the 
> resume ioctl did GFP_KERNEL allocation, the allocation could get stuck 
> trying to write some dirty cached pages to the suspended device.
> 
> The LVM tool and the dmeventd daemon use mlock to lock its address space, 
> so the copy_from_user/copy_to_user call cannot trigger a page fault.

OK, I see, thanks for the clarification! This sounds fragile to me
though. Wouldn't it be better to use the memalloc_noio_save for the
whole copy_params instead? That would force all possible allocations to
not trigger any IO. Something like the following.
---
From dbb2338bb88d2da1ff24cee59cbffd120b119e3b Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Wed, 27 Apr 2016 10:26:13 +0200
Subject: [PATCH] dm: clean up GFP_NIO usage

copy_params uses GFP_NOIO for explicit allocation requests because this
might be called from the suspend path. To quote Mikulas:
: The LVM tool calls suspend and resume ioctls on device mapper block
: devices.
:
: When a device is suspended, any bio sent to the device is held. If the
: resume ioctl did GFP_KERNEL allocation, the allocation could get stuck
: trying to write some dirty cached pages to the suspended device.
:
: The LVM tool and the dmeventd daemon use mlock to lock its address space,
: so the copy_from_user/copy_to_user call cannot trigger a page fault.

Relying on the mlock is quite fragile and we have a better way in kernel
to enfore NOIO which is already used for the vmalloc fallback. Just use
memalloc_noio_{save,restore} around the whole copy_params function which
will force the same also to the page fult paths via copy_{from,to}_user.

While we are there we can also remove __GFP_NOMEMALLOC because copy_params
is never called from MEMALLOC context (e.g. during the reclaim).

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 drivers/md/dm-ioctl.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 2c7ca258c4e4..fe0b57d7573c 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1715,16 +1715,13 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 	 */
 	dmi = NULL;
 	if (param_kernel->data_size <= KMALLOC_MAX_SIZE) {
-		dmi = kmalloc(param_kernel->data_size, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN);
+		dmi = kmalloc(param_kernel->data_size, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
 		if (dmi)
 			*param_flags |= DM_PARAMS_KMALLOC;
 	}
 
 	if (!dmi) {
-		unsigned noio_flag;
-		noio_flag = memalloc_noio_save();
-		dmi = __vmalloc(param_kernel->data_size, GFP_NOIO | __GFP_HIGH | __GFP_HIGHMEM, PAGE_KERNEL);
-		memalloc_noio_restore(noio_flag);
+		dmi = __vmalloc(param_kernel->data_size, GFP_KERNEL | __GFP_HIGH | __GFP_HIGHMEM, PAGE_KERNEL);
 		if (dmi)
 			*param_flags |= DM_PARAMS_VMALLOC;
 	}
@@ -1801,6 +1798,7 @@ static int ctl_ioctl(uint command, struct dm_ioctl __user *user)
 	ioctl_fn fn = NULL;
 	size_t input_param_size;
 	struct dm_ioctl param_kernel;
+	unsigned noio_flag;
 
 	/* only root can play with this */
 	if (!capable(CAP_SYS_ADMIN))
@@ -1832,9 +1830,12 @@ static int ctl_ioctl(uint command, struct dm_ioctl __user *user)
 	}
 
 	/*
-	 * Copy the parameters into kernel space.
+	 * Copy the parameters into kernel space. Make sure that no IO is triggered
+	 * from the allocation paths because this might be called during the suspend.
 	 */
+	noio_flag = memalloc_noio_save();
 	r = copy_params(user, &param_kernel, ioctl_flags, &param, &param_flags);
+	memalloc_noio_restore(noio_flag);
 
 	if (r)
 		return r;
-- 
2.8.0.rc3

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1377774 — CC in git cover letter vs patches (was Re: [PATCH 0/19] get rid of superfluous __GFP_REPORT)

Trimming CC list + CC git folks

Hi Michal,

On Monday 11 April 2016 04:37 PM, Michal Hocko wrote:
> Hi,
> this is the second version of the patchset previously sent [1]

I have a git question if you didn't mind w.r.t. this series. Maybe there's an
obvious answer... I'm using git 2.5.0

I was wondering how you manage to union the individual patch CC in just the cover
letter w/o bombarding everyone with everything.

Thx,
-Vineet

[toc] | [prev] | [next] | [standalone]

#1377947 — Re: CC in git cover letter vs patches (was Re: [PATCH 0/19] get rid of superfluous __GFP_REPORT)

On Wed 13-04-16 16:51:37, Vineet Gupta wrote:
> Trimming CC list + CC git folks
> 
> Hi Michal,
> 
> On Monday 11 April 2016 04:37 PM, Michal Hocko wrote:
> > Hi,
> > this is the second version of the patchset previously sent [1]
> 
> I have a git question if you didn't mind w.r.t. this series. Maybe there's an
> obvious answer... I'm using git 2.5.0
> 
> I was wondering how you manage to union the individual patch CC in just the cover
> letter w/o bombarding everyone with everything.

I am using the following flow:

$ rm *.patch
$ for format-patch range
$ git send-email [--to resp. --cc for all patches] --cc-cmd ./cc-cmd-only-cover.sh --compose *.patch

$ cat ./cc-cmd-only-cover.sh 
#!/bin/bash

# --compose with generate *gitsendemail.msg file
# --cover-letter expects *cover-letter* file
if [[ $1 == *gitsendemail.msg* || $1 == *cover-letter* ]]; then
        grep '<.*@.*>' -h *.patch | sed 's/^.*: //' | sort | uniq
fi

it is a little bit coarse and it would be great if git had a default
option for that but this seems to be working just fine for patch-bombs
where the recipients only have to care about their parts and the cover
for the overal idea of the change.

HTH
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web