[PATCH 17/20] dm: get rid of superfluous gfp flags

[PATCH 17/20] dm: get rid of superfluous gfp flags

[Michal Hocko]

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. Let's just drop the __GFP_REPEAT and leave the further
cleanup to later changes.

Cc: Shaohua Li <shli@kernel.org>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: dm-devel@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..2c7ca258c4e4 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_NOIO | __GFP_HIGH | __GFP_HIGHMEM, PAGE_KERNEL);
 		memalloc_noio_restore(noio_flag);
 		if (dmi)
 			*param_flags |= DM_PARAMS_VMALLOC;
-- 
2.8.0.rc3

[PATCH 18/20] dm: clean up GFP_NIO usage

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

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

Cc: Shaohua Li <shli@kernel.org>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: dm-devel@redhat.com
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

[PATCH 19/20] md: simplify free_params for kmalloc vs vmalloc fallback

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

Use kvfree rather than DM_PARAMS_[KV]MALLOC specific param flags.

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

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index fe0b57d7573c..2b48c49774bc 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1670,19 +1670,14 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 	return r;
 }
 
-#define DM_PARAMS_KMALLOC	0x0001	/* Params alloced with kmalloc */
-#define DM_PARAMS_VMALLOC	0x0002	/* Params alloced with vmalloc */
-#define DM_WIPE_BUFFER		0x0010	/* Wipe input buffer before returning from ioctl */
+#define DM_WIPE_BUFFER		0x0001	/* Wipe input buffer before returning from ioctl */
 
 static void free_params(struct dm_ioctl *param, size_t param_size, int param_flags)
 {
 	if (param_flags & DM_WIPE_BUFFER)
 		memset(param, 0, param_size);
 
-	if (param_flags & DM_PARAMS_KMALLOC)
-		kfree(param);
-	if (param_flags & DM_PARAMS_VMALLOC)
-		vfree(param);
+	kvfree(param);
 }
 
 static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kernel,
@@ -1714,17 +1709,11 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 	 * Use kmalloc() rather than vmalloc() when we can.
 	 */
 	dmi = NULL;
-	if (param_kernel->data_size <= KMALLOC_MAX_SIZE) {
+	if (param_kernel->data_size <= KMALLOC_MAX_SIZE)
 		dmi = kmalloc(param_kernel->data_size, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
-		if (dmi)
-			*param_flags |= DM_PARAMS_KMALLOC;
-	}
 
-	if (!dmi) {
+	if (!dmi)
 		dmi = __vmalloc(param_kernel->data_size, GFP_KERNEL | __GFP_HIGH | __GFP_HIGHMEM, PAGE_KERNEL);
-		if (dmi)
-			*param_flags |= DM_PARAMS_VMALLOC;
-	}
 
 	if (!dmi) {
 		if (secure_data && clear_user(user, param_kernel->data_size))
-- 
2.8.0.rc3

Re: [PATCH 18/20] dm: clean up GFP_NIO usage

[Mikulas Patocka]

On Thu, 28 Apr 2016, Michal Hocko wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> 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.

The userspace memory is locked, so we don't need to use memalloc_noio_save 
around copy_from_user. If the memory weren't locked, memalloc_noio_save 
wouldn't help us to prevent the IO.

We don't need this change (unless you show that it fixes real bug).

Mikulas

> While we are there we can also remove __GFP_NOMEMALLOC because copy_params
> is never called from MEMALLOC context (e.g. during the reclaim).
> 
> Cc: Shaohua Li <shli@kernel.org>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: dm-devel@redhat.com
> 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
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 18/20] dm: clean up GFP_NIO usage

[Michal Hocko]

On Thu 28-04-16 10:20:09, Mikulas Patocka wrote:
> 
> 
> On Thu, 28 Apr 2016, Michal Hocko wrote:
> 
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > 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.
> 
> The userspace memory is locked, so we don't need to use memalloc_noio_save 
> around copy_from_user. If the memory weren't locked, memalloc_noio_save 
> wouldn't help us to prevent the IO.

OK, you are right. Got your point. You would have to read from disk to
fault memory in so this is not just about not performing IO during the
reclaim.

So scratch this patch then.

Thanks!
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] md: simplify free_params for kmalloc vs vmalloc fallback

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

Use kvfree rather than DM_PARAMS_[KV]MALLOC specific param flags.

Cc: Shaohua Li <shli@kernel.org>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: dm-devel@redhat.com
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
Hi,
this is a rebase on top of dropped "dm: clean up GFP_NIO usage" which
should be dropped as per the feedback from Mikulas.

 drivers/md/dm-ioctl.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
index 2c7ca258c4e4..e66e5b43bc18 100644
--- a/drivers/md/dm-ioctl.c
+++ b/drivers/md/dm-ioctl.c
@@ -1670,19 +1670,14 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
 	return r;
 }
 
-#define DM_PARAMS_KMALLOC	0x0001	/* Params alloced with kmalloc */
-#define DM_PARAMS_VMALLOC	0x0002	/* Params alloced with vmalloc */
-#define DM_WIPE_BUFFER		0x0010	/* Wipe input buffer before returning from ioctl */
+#define DM_WIPE_BUFFER		0x0001	/* Wipe input buffer before returning from ioctl */
 
 static void free_params(struct dm_ioctl *param, size_t param_size, int param_flags)
 {
 	if (param_flags & DM_WIPE_BUFFER)
 		memset(param, 0, param_size);
 
-	if (param_flags & DM_PARAMS_KMALLOC)
-		kfree(param);
-	if (param_flags & DM_PARAMS_VMALLOC)
-		vfree(param);
+	kvfree(param);
 }
 
 static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kernel,
@@ -1714,19 +1709,14 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
 	 * Use kmalloc() rather than vmalloc() when we can.
 	 */
 	dmi = NULL;
-	if (param_kernel->data_size <= KMALLOC_MAX_SIZE) {
+	if (param_kernel->data_size <= KMALLOC_MAX_SIZE)
 		dmi = kmalloc(param_kernel->data_size, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __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);
-		if (dmi)
-			*param_flags |= DM_PARAMS_VMALLOC;
 	}
 
 	if (!dmi) {
-- 
2.8.0.rc3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] md: simplify free_params for kmalloc vs vmalloc fallback

[Mikulas Patocka]

Acked-by: Mikulas Patocka <mpatocka@redhat.com>

BTW. we could also use kvmalloc to complement kvfree, proposed here: 
https://www.redhat.com/archives/dm-devel/2015-July/msg00046.html

Mikulas

On Thu, 28 Apr 2016, Michal Hocko wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> Use kvfree rather than DM_PARAMS_[KV]MALLOC specific param flags.
> 
> Cc: Shaohua Li <shli@kernel.org>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: dm-devel@redhat.com
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> Hi,
> this is a rebase on top of dropped "dm: clean up GFP_NIO usage" which
> should be dropped as per the feedback from Mikulas.
> 
>  drivers/md/dm-ioctl.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 2c7ca258c4e4..e66e5b43bc18 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1670,19 +1670,14 @@ static int check_version(unsigned int cmd, struct dm_ioctl __user *user)
>  	return r;
>  }
>  
> -#define DM_PARAMS_KMALLOC	0x0001	/* Params alloced with kmalloc */
> -#define DM_PARAMS_VMALLOC	0x0002	/* Params alloced with vmalloc */
> -#define DM_WIPE_BUFFER		0x0010	/* Wipe input buffer before returning from ioctl */
> +#define DM_WIPE_BUFFER		0x0001	/* Wipe input buffer before returning from ioctl */
>  
>  static void free_params(struct dm_ioctl *param, size_t param_size, int param_flags)
>  {
>  	if (param_flags & DM_WIPE_BUFFER)
>  		memset(param, 0, param_size);
>  
> -	if (param_flags & DM_PARAMS_KMALLOC)
> -		kfree(param);
> -	if (param_flags & DM_PARAMS_VMALLOC)
> -		vfree(param);
> +	kvfree(param);
>  }
>  
>  static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kernel,
> @@ -1714,19 +1709,14 @@ static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kern
>  	 * Use kmalloc() rather than vmalloc() when we can.
>  	 */
>  	dmi = NULL;
> -	if (param_kernel->data_size <= KMALLOC_MAX_SIZE) {
> +	if (param_kernel->data_size <= KMALLOC_MAX_SIZE)
>  		dmi = kmalloc(param_kernel->data_size, GFP_NOIO | __GFP_NORETRY | __GFP_NOMEMALLOC | __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);
> -		if (dmi)
> -			*param_flags |= DM_PARAMS_VMALLOC;
>  	}
>  
>  	if (!dmi) {
> -- 
> 2.8.0.rc3
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] md: simplify free_params for kmalloc vs vmalloc fallback

[Michal Hocko]

On Thu 28-04-16 11:04:05, Mikulas Patocka wrote:
> Acked-by: Mikulas Patocka <mpatocka@redhat.com>

Thanks!

> BTW. we could also use kvmalloc to complement kvfree, proposed here: 
> https://www.redhat.com/archives/dm-devel/2015-July/msg00046.html

If there are sufficient users (I haven't checked other than quick git
grep on KMALLOC_MAX_SIZE and there do not seem that many) who are
sharing the same fallback strategy then why not. But I suspect that some
would rather fallback earlier and even do not attempt larger than e.g.
order-1 requests.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 19/20] md: simplify free_params for kmalloc vs vmalloc fallback

[Mike Snitzer]

On Thu, Apr 28 2016 at  9:24am -0400,
Michal Hocko <mhocko@kernel.org> wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> Use kvfree rather than DM_PARAMS_[KV]MALLOC specific param flags.
> 
> Cc: Shaohua Li <shli@kernel.org>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: dm-devel@redhat.com
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Nack, seriously, this is the 3rd time this patch has been attempted.
Did you actually test the change?  It'll crash very quickly, see:

https://www.redhat.com/archives/dm-devel/2016-April/msg00103.html

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] md: simplify free_params for kmalloc vs vmalloc fallback

[Mikulas Patocka]

On Thu, 28 Apr 2016, Michal Hocko wrote:

> On Thu 28-04-16 11:04:05, Mikulas Patocka wrote:
> > Acked-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> Thanks!
> 
> > BTW. we could also use kvmalloc to complement kvfree, proposed here: 
> > https://www.redhat.com/archives/dm-devel/2015-July/msg00046.html
> 
> If there are sufficient users (I haven't checked other than quick git
> grep on KMALLOC_MAX_SIZE

the problem is that kmallocs with large sizes near KMALLOC_MAX_SIZE are 
unreliable, they'll randomly fail if memory is too fragmented.

> and there do not seem that many) who are
> sharing the same fallback strategy then why not. But I suspect that some
> would rather fallback earlier and even do not attempt larger than e.g.
> order-1 requests.
> -- 
> Michal Hocko
> SUSE Labs

There are many users that use one of these patterns:

	if (size <= some_threshold)
		p = kmalloc(size);
	else
		p = vmalloc(size);

or

	p = kmalloc(size);
	if (!p)
		p = vmalloc(size);


For example: alloc_fdmem, seq_buf_alloc, setxattr, getxattr, ipc_alloc, 
pidlist_allocate, get_pages_array, alloc_bucket_locks, 
frame_vector_create. If you grep the kernel for vmalloc, you'll find this 
pattern over and over again.

In alloc_large_system_hash, there is
	table = __vmalloc(size, GFP_ATOMIC, PAGE_KERNEL);
- that is clearly wrong because __vmalloc doesn't respect GFP_ATOMIC

Mikulas

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 19/20] md: simplify free_params for kmalloc vs vmalloc fallback

[Michal Hocko]

On Thu 28-04-16 11:37:31, Mike Snitzer wrote:
> On Thu, Apr 28 2016 at  9:24am -0400,
> Michal Hocko <mhocko@kernel.org> wrote:
> 
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > Use kvfree rather than DM_PARAMS_[KV]MALLOC specific param flags.
> > 
> > Cc: Shaohua Li <shli@kernel.org>
> > Cc: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: dm-devel@redhat.com
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Nack, seriously, this is the 3rd time this patch has been attempted.
> Did you actually test the change?  It'll crash very quickly, see:
> 
> https://www.redhat.com/archives/dm-devel/2016-April/msg00103.html

You are right! My bad I should have checked the other callers!

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] md: simplify free_params for kmalloc vs vmalloc fallback

[Michal Hocko]

On Thu 28-04-16 11:40:59, Mikulas Patocka wrote:
[...]
> There are many users that use one of these patterns:
> 
> 	if (size <= some_threshold)
> 		p = kmalloc(size);
> 	else
> 		p = vmalloc(size);
> 
> or
> 
> 	p = kmalloc(size);
> 	if (!p)
> 		p = vmalloc(size);
> 
> 
> For example: alloc_fdmem, seq_buf_alloc, setxattr, getxattr, ipc_alloc, 
> pidlist_allocate, get_pages_array, alloc_bucket_locks, 
> frame_vector_create. If you grep the kernel for vmalloc, you'll find this 
> pattern over and over again.

It is certainly good to address a common pattern by a helper if it makes
to code easier to follo IMHO.

> 
> In alloc_large_system_hash, there is
> 	table = __vmalloc(size, GFP_ATOMIC, PAGE_KERNEL);
> - that is clearly wrong because __vmalloc doesn't respect GFP_ATOMIC

I have seen this code some time already. I guess it was Al complaining
about it but then I just forgot about it. I have no idea why GFP_ATOMIC
was used there. This predates git times but it should be
https://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.10/2.6.10-mm1/broken-out/alloc_large_system_hash-numa-interleaving.patch
The changelog is quite verbose but no mention about this ugliness.

So I do agree that the above should be fixed and a common helper might
be interesting but I am afraid we are getting off topic here.

Thanks!
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 17/20] dm: get rid of superfluous gfp flags

[Mike Snitzer]

On Thu, Apr 28 2016 at  9:24am -0400,
Michal Hocko <mhocko@kernel.org> 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
> 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. Let's just drop the __GFP_REPEAT and leave the further
> cleanup to later changes.
> 
> Cc: Shaohua Li <shli@kernel.org>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: dm-devel@redhat.com
> Signed-off-by: Michal Hocko <mhocko@suse.com>

I've taken this patch for 4.7 but editted the header, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.7&id=0222c76e96163355620224625c1cd80991086dc7

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 17/20] dm: get rid of superfluous gfp flags

[Michal Hocko]

On Fri 29-04-16 14:54:52, Mike Snitzer wrote:
> On Thu, Apr 28 2016 at  9:24am -0400,
> Michal Hocko <mhocko@kernel.org> 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
> > 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. Let's just drop the __GFP_REPEAT and leave the further
> > cleanup to later changes.
> > 
> > Cc: Shaohua Li <shli@kernel.org>
> > Cc: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: dm-devel@redhat.com
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> I've taken this patch for 4.7 but editted the header, see:
> https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.7&id=0222c76e96163355620224625c1cd80991086dc7

Thanks!

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>