[PATCH v2] fs: use kmalloc() to allocate fdmem if possible

[PATCH v2] fs: use kmalloc() to allocate fdmem if possible

[Changli Gao]

use kmalloc() to allocate fdmem if possible.

vmalloc() is used as a fallback solution for fdmem allocation. A new helper
function __free_fdtable() is introduced to reduce the lines of code.

A potential bug, vfree() a memory allocated by kmalloc(), is fixed.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
 fs/file.c |   55 ++++++++++++++++++++++---------------------------------
 1 file changed, 22 insertions(+), 33 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 34bb7f7..9af31ae 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -39,28 +39,27 @@ int sysctl_nr_open_max = 1024 * 1024; /* raised later */
  */
 static DEFINE_PER_CPU(struct fdtable_defer, fdtable_defer_list);
 
-static inline void * alloc_fdmem(unsigned int size)
+static inline void *alloc_fdmem(unsigned int size)
 {
-	if (size <= PAGE_SIZE)
-		return kmalloc(size, GFP_KERNEL);
-	else
-		return vmalloc(size);
+	void *data;
+
+	data = kmalloc(size, GFP_KERNEL);
+	if (data != NULL)
+		return data;
+
+	return vmalloc(size);
 }
 
-static inline void free_fdarr(struct fdtable *fdt)
+static inline void free_fdmem(void *ptr)
 {
-	if (fdt->max_fds <= (PAGE_SIZE / sizeof(struct file *)))
-		kfree(fdt->fd);
-	else
-		vfree(fdt->fd);
+	is_vmalloc_addr(ptr) ? vfree(ptr) : kfree(ptr);
 }
 
-static inline void free_fdset(struct fdtable *fdt)
+static inline void __free_fdtable(struct fdtable *fdt)
 {
-	if (fdt->max_fds <= (PAGE_SIZE * BITS_PER_BYTE / 2))
-		kfree(fdt->open_fds);
-	else
-		vfree(fdt->open_fds);
+	free_fdmem(fdt->fd);
+	free_fdmem(fdt->open_fds);
+	kfree(fdt);
 }
 
 static void free_fdtable_work(struct work_struct *work)
@@ -75,9 +74,8 @@ static void free_fdtable_work(struct work_struct *work)
 	spin_unlock_bh(&f->lock);
 	while(fdt) {
 		struct fdtable *next = fdt->next;
-		vfree(fdt->fd);
-		free_fdset(fdt);
-		kfree(fdt);
+
+		__free_fdtable(fdt);
 		fdt = next;
 	}
 }
@@ -184,7 +182,7 @@ static struct fdtable * alloc_fdtable(unsigned int nr)
 	return fdt;
 
 out_arr:
-	free_fdarr(fdt);
+	free_fdmem(fdt->fd);
 out_fdt:
 	kfree(fdt);
 out:
@@ -214,9 +212,7 @@ static int expand_fdtable(struct files_struct *files, int nr)
 	 * caller and alloc_fdtable().  Cheaper to catch it here...
 	 */
 	if (unlikely(new_fdt->max_fds <= nr)) {
-		free_fdarr(new_fdt);
-		free_fdset(new_fdt);
-		kfree(new_fdt);
+		__free_fdtable(new_fdt);
 		return -EMFILE;
 	}
 	/*
@@ -232,9 +228,7 @@ static int expand_fdtable(struct files_struct *files, int nr)
 			free_fdtable(cur_fdt);
 	} else {
 		/* Somebody else expanded, so undo our attempt */
-		free_fdarr(new_fdt);
-		free_fdset(new_fdt);
-		kfree(new_fdt);
+		__free_fdtable(new_fdt);
 	}
 	return 1;
 }
@@ -325,11 +319,8 @@ struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
 	while (unlikely(open_files > new_fdt->max_fds)) {
 		spin_unlock(&oldf->file_lock);
 
-		if (new_fdt != &newf->fdtab) {
-			free_fdarr(new_fdt);
-			free_fdset(new_fdt);
-			kfree(new_fdt);
-		}
+		if (new_fdt != &newf->fdtab)
+			__free_fdtable(new_fdt);
 
 		new_fdt = alloc_fdtable(open_files - 1);
 		if (!new_fdt) {
@@ -339,9 +330,7 @@ struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
 
 		/* beyond sysctl_nr_open; nothing to do */
 		if (unlikely(new_fdt->max_fds < open_files)) {
-			free_fdarr(new_fdt);
-			free_fdset(new_fdt);
-			kfree(new_fdt);
+			__free_fdtable(new_fdt);
 			*errorp = -EMFILE;
 			goto out_release;
 		}

Re: [PATCH v2] fs: use kmalloc() to allocate fdmem if possible

[Andrew Morton]

On Mon,  3 May 2010 03:02:00 +0800
Changli Gao <xiaosuo@gmail.com> wrote:

> use kmalloc() to allocate fdmem if possible.
> 
> vmalloc() is used as a fallback solution for fdmem allocation. A new helper
> function __free_fdtable() is introduced to reduce the lines of code.
> 
> A potential bug, vfree() a memory allocated by kmalloc(), is fixed.
> 

Seems a reasonable thing to do.  It might also be reasonable to make
vmalloc() try kmalloc() first, but that's a separate exercise.

> ----
>  fs/file.c |   55 ++++++++++++++++++++++---------------------------------
>  1 file changed, 22 insertions(+), 33 deletions(-)
> diff --git a/fs/file.c b/fs/file.c
> index 34bb7f7..9af31ae 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -39,28 +39,27 @@ int sysctl_nr_open_max = 1024 * 1024; /* raised later */
>   */
>  static DEFINE_PER_CPU(struct fdtable_defer, fdtable_defer_list);
>  
> -static inline void * alloc_fdmem(unsigned int size)
> +static inline void *alloc_fdmem(unsigned int size)
>  {
> -	if (size <= PAGE_SIZE)
> -		return kmalloc(size, GFP_KERNEL);
> -	else
> -		return vmalloc(size);
> +	void *data;
> +
> +	data = kmalloc(size, GFP_KERNEL);

This most definitely should have __GFP_NOWARN.

> +	if (data != NULL)
> +		return data;
> +
> +	return vmalloc(size);
>  }
>  
> -static inline void free_fdarr(struct fdtable *fdt)
> +static inline void free_fdmem(void *ptr)
>  {
> -	if (fdt->max_fds <= (PAGE_SIZE / sizeof(struct file *)))
> -		kfree(fdt->fd);
> -	else
> -		vfree(fdt->fd);
> +	is_vmalloc_addr(ptr) ? vfree(ptr) : kfree(ptr);
>  }
>  
> -static inline void free_fdset(struct fdtable *fdt)
> +static inline void __free_fdtable(struct fdtable *fdt)
>  {
> -	if (fdt->max_fds <= (PAGE_SIZE * BITS_PER_BYTE / 2))
> -		kfree(fdt->open_fds);
> -	else
> -		vfree(fdt->open_fds);
> +	free_fdmem(fdt->fd);
> +	free_fdmem(fdt->open_fds);
> +	kfree(fdt);
>  }

And these should be uninlined - they're too large to be inlined.

My version of gcc seems to just uninline them anyway, but forcing them
to be inlined with __always_inline indeed causes 70-80 bytes more text,
and we figure that larger text generally causes a slower kernel due to
cache eviction effects.

Re: [PATCH v2] fs: use kmalloc() to allocate fdmem if possible

[Jiri Slaby]

On 05/05/2010 12:27 AM, Andrew Morton wrote:
> On Mon,  3 May 2010 03:02:00 +0800
> Changli Gao <xiaosuo@gmail.com> wrote:
> 
>> use kmalloc() to allocate fdmem if possible.
>>
>> vmalloc() is used as a fallback solution for fdmem allocation. A new helper
>> function __free_fdtable() is introduced to reduce the lines of code.
>>
>> A potential bug, vfree() a memory allocated by kmalloc(), is fixed.
>>
> 
> Seems a reasonable thing to do.

Hi,

So didn't we converge to something like this:
http://lkml.org/lkml/2010/5/3/53
instead of this patch?

This

>  It might also be reasonable to make
> vmalloc() try kmalloc() first, but that's a separate exercise.

and this

> This most definitely should have __GFP_NOWARN.

is discussed there too btw.

thanks,
-- 
js
suse labs

Re: [PATCH v2] fs: use kmalloc() to allocate fdmem if possible

[Eric Dumazet]

Le mercredi 05 mai 2010 à 09:18 +0200, Jiri Slaby a écrit :
> On 05/05/2010 12:27 AM, Andrew Morton wrote:
> > On Mon,  3 May 2010 03:02:00 +0800
> > Changli Gao <xiaosuo@gmail.com> wrote:
> > 
> >> use kmalloc() to allocate fdmem if possible.
> >>
> >> vmalloc() is used as a fallback solution for fdmem allocation. A new helper
> >> function __free_fdtable() is introduced to reduce the lines of code.
> >>
> >> A potential bug, vfree() a memory allocated by kmalloc(), is fixed.
> >>
> > 
> > Seems a reasonable thing to do.
> 
> Hi,
> 
> So didn't we converge to something like this:
> http://lkml.org/lkml/2010/5/3/53
> instead of this patch?
> 
> This
> 
> >  It might also be reasonable to make
> > vmalloc() try kmalloc() first, but that's a separate exercise.
> 
> and this
> 
> > This most definitely should have __GFP_NOWARN.
> 
> is discussed there too btw.
> 
> thanks,

Its a start, I guess we also need some NUMA capability as well.

For example, iptables could use such an allocator only if it is NUMA
enabled.



--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html