[PATCH 4/7] dm: introduce dm_kvmalloc

[PATCH 4/7] dm: introduce dm_kvmalloc

[Mikulas Patocka]

Introduce the functions dm_kvmalloc and dm_kvmalloc_node. These functions
attempt to do allocation with kmalloc and if it fails, use vmalloc. Memory
allocated with these functions should be freed with kvfree (that function
is already present in the Linux kernel).

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

---
 drivers/md/dm-ioctl.c         |   26 +++++---------------------
 drivers/md/dm-table.c         |   37 +++++++++++++++++++++++++++++++++++++
 include/linux/device-mapper.h |    2 ++
 3 files changed, 44 insertions(+), 21 deletions(-)

Index: linux-4.1/drivers/md/dm-ioctl.c
===================================================================
--- linux-4.1.orig/drivers/md/dm-ioctl.c	2015-07-02 19:21:15.000000000 +0200
+++ linux-4.1/drivers/md/dm-ioctl.c	2015-07-02 19:21:21.000000000 +0200
@@ -1676,12 +1676,8 @@ static void free_params(struct dm_ioctl 
 	if (param_flags & DM_WIPE_BUFFER)
 		memset(param, 0, param_size);
 
-	if (param_flags & DM_PARAMS_ALLOC) {
-		if (is_vmalloc_addr(param))
-			vfree(param);
-		else
-			kfree(param);
-	}
+	if (param_flags & DM_PARAMS_ALLOC)
+		kvfree(param);
 }
 
 static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kernel,
@@ -1712,21 +1708,7 @@ static int copy_params(struct dm_ioctl _
 	 * Try to avoid low memory issues when a device is suspended.
 	 * Use kmalloc() rather than vmalloc() when we can.
 	 */
-	dmi = NULL;
-	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_ALLOC;
-	}
-
-	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);
-		memalloc_noio_restore(noio_flag);
-		if (dmi)
-			*param_flags |= DM_PARAMS_ALLOC;
-	}
+	dmi = dm_kvmalloc(param_kernel->data_size, GFP_NOIO);
 
 	if (!dmi) {
 		if (secure_data && clear_user(user, param_kernel->data_size))
@@ -1734,6 +1716,8 @@ static int copy_params(struct dm_ioctl _
 		return -ENOMEM;
 	}
 
+	*param_flags |= DM_PARAMS_ALLOC;
+
 	if (copy_from_user(dmi, user, param_kernel->data_size))
 		goto bad;
 
Index: linux-4.1/include/linux/device-mapper.h
===================================================================
--- linux-4.1.orig/include/linux/device-mapper.h	2015-07-02 19:21:16.000000000 +0200
+++ linux-4.1/include/linux/device-mapper.h	2015-07-02 19:21:21.000000000 +0200
@@ -475,6 +475,8 @@ struct dm_table *dm_swap_table(struct ma
 /*
  * A wrapper around vmalloc.
  */
+void *dm_kvmalloc_node(size_t size, gfp_t gfp, int node);
+void *dm_kvmalloc(size_t size, gfp_t gfp);
 void *dm_vcalloc(unsigned long nmemb, unsigned long elem_size);
 
 /*-----------------------------------------------------------------
Index: linux-4.1/drivers/md/dm-table.c
===================================================================
--- linux-4.1.orig/drivers/md/dm-table.c	2015-07-02 19:21:16.000000000 +0200
+++ linux-4.1/drivers/md/dm-table.c	2015-07-02 19:28:18.000000000 +0200
@@ -131,6 +131,43 @@ static int setup_btree_index(unsigned in
 	return 0;
 }
 
+void *dm_kvmalloc_node(size_t size, gfp_t gfp, int node)
+{
+	void *p;
+	unsigned uninitialized_var(noio_flag);
+
+	/* vmalloc doesn't support no-wait allocations */
+	WARN_ON(!(gfp & __GFP_WAIT));
+
+	if (likely(size <= KMALLOC_MAX_SIZE)) {
+		p = kmalloc_node(size, gfp | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN, node);
+		if (likely(p != NULL))
+			return p;
+	}
+	if ((gfp & (__GFP_IO | __GFP_FS)) != (__GFP_IO | __GFP_FS)) {
+		/*
+		 * vmalloc allocates page tables with GFP_KERNEL, regardless
+		 * of GFP flags passed to it. If we are no GFP_NOIO context,
+		 * we call memalloc_noio_save, so that all allocations are
+		 * implicitly done with GFP_NOIO.
+		 */
+		noio_flag = memalloc_noio_save();
+		gfp |= __GFP_HIGH;
+	}
+	p = __vmalloc_node_flags(size, node, gfp | __GFP_REPEAT | __GFP_HIGHMEM);
+	if ((gfp & (__GFP_IO | __GFP_FS)) != (__GFP_IO | __GFP_FS)) {
+		memalloc_noio_restore(noio_flag);
+	}
+	return p;
+}
+EXPORT_SYMBOL(dm_kvmalloc_node);
+
+void *dm_kvmalloc(size_t size, gfp_t gfp)
+{
+	return dm_kvmalloc_node(size, gfp, NUMA_NO_NODE);
+}
+EXPORT_SYMBOL(dm_kvmalloc);
+
 void *dm_vcalloc(unsigned long nmemb, unsigned long elem_size)
 {
 	unsigned long size;

Re: [PATCH 4/7] dm: introduce dm_kvmalloc

[Vivek Goyal]

On Fri, Jul 03, 2015 at 04:59:54PM -0400, Mikulas Patocka wrote:
[..]
> Index: linux-4.1/drivers/md/dm-table.c
> ===================================================================
> --- linux-4.1.orig/drivers/md/dm-table.c	2015-07-02 19:21:16.000000000 +0200
> +++ linux-4.1/drivers/md/dm-table.c	2015-07-02 19:28:18.000000000 +0200
> @@ -131,6 +131,43 @@ static int setup_btree_index(unsigned in
>  	return 0;
>  }
>  
> +void *dm_kvmalloc_node(size_t size, gfp_t gfp, int node)
> +{
> +	void *p;
> +	unsigned uninitialized_var(noio_flag);
> +
> +	/* vmalloc doesn't support no-wait allocations */
> +	WARN_ON(!(gfp & __GFP_WAIT));
> +

Should this WARN_ON() be moved after kmalloc(). This should be triggered
only if we use vmalloc().

Thanks
Vivek

Re: [PATCH 4/7] dm: introduce dm_kvmalloc

[Mikulas Patocka]

On Mon, 6 Jul 2015, Vivek Goyal wrote:

> On Fri, Jul 03, 2015 at 04:59:54PM -0400, Mikulas Patocka wrote:
> [..]
> > Index: linux-4.1/drivers/md/dm-table.c
> > ===================================================================
> > --- linux-4.1.orig/drivers/md/dm-table.c	2015-07-02 19:21:16.000000000 +0200
> > +++ linux-4.1/drivers/md/dm-table.c	2015-07-02 19:28:18.000000000 +0200
> > @@ -131,6 +131,43 @@ static int setup_btree_index(unsigned in
> >  	return 0;
> >  }
> >  
> > +void *dm_kvmalloc_node(size_t size, gfp_t gfp, int node)
> > +{
> > +	void *p;
> > +	unsigned uninitialized_var(noio_flag);
> > +
> > +	/* vmalloc doesn't support no-wait allocations */
> > +	WARN_ON(!(gfp & __GFP_WAIT));
> > +
> 
> Should this WARN_ON() be moved after kmalloc(). This should be triggered
> only if we use vmalloc().
> 
> Thanks
> Vivek

I think not - it would just hide the problematic usage. Warning always is 
better than warning with some low probability when kmalloc fails.

Mikulas

Re: [PATCH 4/7] dm: introduce dm_kvmalloc

[Vivek Goyal]

On Mon, Jul 06, 2015 at 09:04:34AM -0400, Mikulas Patocka wrote:
> 
> 
> On Mon, 6 Jul 2015, Vivek Goyal wrote:
> 
> > On Fri, Jul 03, 2015 at 04:59:54PM -0400, Mikulas Patocka wrote:
> > [..]
> > > Index: linux-4.1/drivers/md/dm-table.c
> > > ===================================================================
> > > --- linux-4.1.orig/drivers/md/dm-table.c	2015-07-02 19:21:16.000000000 +0200
> > > +++ linux-4.1/drivers/md/dm-table.c	2015-07-02 19:28:18.000000000 +0200
> > > @@ -131,6 +131,43 @@ static int setup_btree_index(unsigned in
> > >  	return 0;
> > >  }
> > >  
> > > +void *dm_kvmalloc_node(size_t size, gfp_t gfp, int node)
> > > +{
> > > +	void *p;
> > > +	unsigned uninitialized_var(noio_flag);
> > > +
> > > +	/* vmalloc doesn't support no-wait allocations */
> > > +	WARN_ON(!(gfp & __GFP_WAIT));
> > > +
> > 
> > Should this WARN_ON() be moved after kmalloc(). This should be triggered
> > only if we use vmalloc().
> > 
> > Thanks
> > Vivek
> 
> I think not - it would just hide the problematic usage. Warning always is 
> better than warning with some low probability when kmalloc fails.

Ok, so you don't want this function to be used if __GFP_WAIT is not
specified and caller needs to call kmalloc() directly in that case. If
that's the intent, it is fine.

Thanks
Vivek

Re: [PATCH 4/7] dm: introduce dm_kvmalloc

[Mikulas Patocka]

On Mon, 6 Jul 2015, Vivek Goyal wrote:

> On Mon, Jul 06, 2015 at 09:04:34AM -0400, Mikulas Patocka wrote:
> > 
> > 
> > On Mon, 6 Jul 2015, Vivek Goyal wrote:
> > 
> > > On Fri, Jul 03, 2015 at 04:59:54PM -0400, Mikulas Patocka wrote:
> > > [..]
> > > > Index: linux-4.1/drivers/md/dm-table.c
> > > > ===================================================================
> > > > --- linux-4.1.orig/drivers/md/dm-table.c	2015-07-02 19:21:16.000000000 +0200
> > > > +++ linux-4.1/drivers/md/dm-table.c	2015-07-02 19:28:18.000000000 +0200
> > > > @@ -131,6 +131,43 @@ static int setup_btree_index(unsigned in
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +void *dm_kvmalloc_node(size_t size, gfp_t gfp, int node)
> > > > +{
> > > > +	void *p;
> > > > +	unsigned uninitialized_var(noio_flag);
> > > > +
> > > > +	/* vmalloc doesn't support no-wait allocations */
> > > > +	WARN_ON(!(gfp & __GFP_WAIT));
> > > > +
> > > 
> > > Should this WARN_ON() be moved after kmalloc(). This should be triggered
> > > only if we use vmalloc().
> > > 
> > > Thanks
> > > Vivek
> > 
> > I think not - it would just hide the problematic usage. Warning always is 
> > better than warning with some low probability when kmalloc fails.
> 
> Ok, so you don't want this function to be used if __GFP_WAIT is not
> specified and caller needs to call kmalloc() directly in that case. If
> that's the intent, it is fine.

Yes, if the caller doesn't specify __GFP_WAIT, he should use kmalloc 
directly.

Mikulas

> Thanks
> Vivek
> 

Re: [PATCH 4/7] dm: introduce dm_kvmalloc

[Mike Snitzer]

On Fri, Jul 03 2015 at  4:59pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Introduce the functions dm_kvmalloc and dm_kvmalloc_node. These functions
> attempt to do allocation with kmalloc and if it fails, use vmalloc. Memory
> allocated with these functions should be freed with kvfree (that function
> is already present in the Linux kernel).
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm-ioctl.c         |   26 +++++---------------------
>  drivers/md/dm-table.c         |   37 +++++++++++++++++++++++++++++++++++++
>  include/linux/device-mapper.h |    2 ++
>  3 files changed, 44 insertions(+), 21 deletions(-)
> 
> Index: linux-4.1/drivers/md/dm-ioctl.c
> ===================================================================
> --- linux-4.1.orig/drivers/md/dm-ioctl.c	2015-07-02 19:21:15.000000000 +0200
> +++ linux-4.1/drivers/md/dm-ioctl.c	2015-07-02 19:21:21.000000000 +0200
> @@ -1676,12 +1676,8 @@ static void free_params(struct dm_ioctl 
>  	if (param_flags & DM_WIPE_BUFFER)
>  		memset(param, 0, param_size);
>  
> -	if (param_flags & DM_PARAMS_ALLOC) {
> -		if (is_vmalloc_addr(param))
> -			vfree(param);
> -		else
> -			kfree(param);
> -	}
> +	if (param_flags & DM_PARAMS_ALLOC)
> +		kvfree(param);
>  }
>  
>  static int copy_params(struct dm_ioctl __user *user, struct dm_ioctl *param_kernel,
> @@ -1712,21 +1708,7 @@ static int copy_params(struct dm_ioctl _
>  	 * Try to avoid low memory issues when a device is suspended.
>  	 * Use kmalloc() rather than vmalloc() when we can.
>  	 */
> -	dmi = NULL;
> -	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_ALLOC;
> -	}
> -
> -	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);
> -		memalloc_noio_restore(noio_flag);
> -		if (dmi)
> -			*param_flags |= DM_PARAMS_ALLOC;
> -	}
> +	dmi = dm_kvmalloc(param_kernel->data_size, GFP_NOIO);
>  
>  	if (!dmi) {
>  		if (secure_data && clear_user(user, param_kernel->data_size))
> @@ -1734,6 +1716,8 @@ static int copy_params(struct dm_ioctl _
>  		return -ENOMEM;
>  	}
>  
> +	*param_flags |= DM_PARAMS_ALLOC;
> +
>  	if (copy_from_user(dmi, user, param_kernel->data_size))
>  		goto bad;
>  
> Index: linux-4.1/include/linux/device-mapper.h
> ===================================================================
> --- linux-4.1.orig/include/linux/device-mapper.h	2015-07-02 19:21:16.000000000 +0200
> +++ linux-4.1/include/linux/device-mapper.h	2015-07-02 19:21:21.000000000 +0200
> @@ -475,6 +475,8 @@ struct dm_table *dm_swap_table(struct ma
>  /*
>   * A wrapper around vmalloc.
>   */
> +void *dm_kvmalloc_node(size_t size, gfp_t gfp, int node);
> +void *dm_kvmalloc(size_t size, gfp_t gfp);
>  void *dm_vcalloc(unsigned long nmemb, unsigned long elem_size);
>  
>  /*-----------------------------------------------------------------
> Index: linux-4.1/drivers/md/dm-table.c
> ===================================================================
> --- linux-4.1.orig/drivers/md/dm-table.c	2015-07-02 19:21:16.000000000 +0200
> +++ linux-4.1/drivers/md/dm-table.c	2015-07-02 19:28:18.000000000 +0200
> @@ -131,6 +131,43 @@ static int setup_btree_index(unsigned in
>  	return 0;
>  }
>  
> +void *dm_kvmalloc_node(size_t size, gfp_t gfp, int node)
> +{
> +	void *p;
> +	unsigned uninitialized_var(noio_flag);
> +
> +	/* vmalloc doesn't support no-wait allocations */
> +	WARN_ON(!(gfp & __GFP_WAIT));
> +
> +	if (likely(size <= KMALLOC_MAX_SIZE)) {
> +		p = kmalloc_node(size, gfp | __GFP_NORETRY | __GFP_NOMEMALLOC | __GFP_NOWARN, node);
> +		if (likely(p != NULL))
> +			return p;
> +	}
> +	if ((gfp & (__GFP_IO | __GFP_FS)) != (__GFP_IO | __GFP_FS)) {
> +		/*
> +		 * vmalloc allocates page tables with GFP_KERNEL, regardless
> +		 * of GFP flags passed to it. If we are no GFP_NOIO context,
> +		 * we call memalloc_noio_save, so that all allocations are
> +		 * implicitly done with GFP_NOIO.
> +		 */
> +		noio_flag = memalloc_noio_save();
> +		gfp |= __GFP_HIGH;
> +	}
> +	p = __vmalloc_node_flags(size, node, gfp | __GFP_REPEAT | __GFP_HIGHMEM);
> +	if ((gfp & (__GFP_IO | __GFP_FS)) != (__GFP_IO | __GFP_FS)) {
> +		memalloc_noio_restore(noio_flag);
> +	}
> +	return p;
> +}
> +EXPORT_SYMBOL(dm_kvmalloc_node);
> +
> +void *dm_kvmalloc(size_t size, gfp_t gfp)
> +{
> +	return dm_kvmalloc_node(size, gfp, NUMA_NO_NODE);
> +}
> +EXPORT_SYMBOL(dm_kvmalloc);
> +
>  void *dm_vcalloc(unsigned long nmemb, unsigned long elem_size)
>  {
>  	unsigned long size;
> 

In general I like what you've done with this patchset _except_ I'm not
seeing why dm_kvmalloc() should be in DM at all.  It should probably be
elevated to an kvmalloc() export from mm/util.c and include/linux/mm.h
along-side kvfree().

David and/or Andrew, what do you think?

FYI, full patchset starts here:
https://www.redhat.com/archives/dm-devel/2015-July/msg00004.html

(but Mikulas didn't chain the reply so the 7 patches aren't properly
threaded/navigated, you can dig out the patches toward the top of the
list here: https://patchwork.kernel.org/project/dm-devel/list/ )

Re: [PATCH 4/7] dm: introduce dm_kvmalloc

[Mikulas Patocka]

On Mon, 6 Jul 2015, Mike Snitzer wrote:

> In general I like what you've done with this patchset _except_ I'm not
> seeing why dm_kvmalloc() should be in DM at all.  It should probably be
> elevated to an kvmalloc() export from mm/util.c and include/linux/mm.h
> along-side kvfree().

If memory management maintainers agree to put it into mm core, it would be 
nice. If not, put it into device mapper.

Mikulas

> David and/or Andrew, what do you think?
> 
> FYI, full patchset starts here:
> https://www.redhat.com/archives/dm-devel/2015-July/msg00004.html
> 
> (but Mikulas didn't chain the reply so the 7 patches aren't properly
> threaded/navigated, you can dig out the patches toward the top of the
> list here: https://patchwork.kernel.org/project/dm-devel/list/ )
> 

Re: [PATCH 4/7] dm: introduce dm_kvmalloc

[Mike Snitzer]

On Tue, Jul 07 2015 at  8:50am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Mon, 6 Jul 2015, Mike Snitzer wrote:
> 
> > In general I like what you've done with this patchset _except_ I'm not
> > seeing why dm_kvmalloc() should be in DM at all.  It should probably be
> > elevated to an kvmalloc() export from mm/util.c and include/linux/mm.h
> > along-side kvfree().
> 
> If memory management maintainers agree to put it into mm core, it would be 
> nice. If not, put it into device mapper.

Please rework your patches so that the mm changes are split out into
clean patch(es) at the start of the series.

Please cc Andrew and David on posting v2.  Also, please chain the reply
so that the patchset is properly threaded.