linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] fs: use kmalloc() to allocate fdmem if possible
       [not found] <Alexander Viro <viro@zeniv.linux.org.uk>
@ 2010-05-02 16:46 ` Changli Gao
  2010-05-02 17:31   ` Avi Kivity
  0 siblings, 1 reply; 18+ messages in thread
From: Changli Gao @ 2010-05-02 16:46 UTC (permalink / raw)
  Cc: Jiri Slaby, Andrew Morton, Paul E. McKenney, Alexey Dobriyan,
	Ingo Molnar, Peter Zijlstra, linux-fsdevel, linux-kernel,
	Changli Gao

use kmalloc() to allocate fdmem if possible.

vmalloc() is used as a fallback solution for fdmem allocation. Two members are
added into the hole of the structure fdtable to indicate if vmalloc() is used
or not.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
 fs/file.c               |   33 ++++++++++++++++++++-------------
 include/linux/fdtable.h |    2 ++
 2 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 34bb7f7..f71dd85 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -39,28 +39,34 @@ 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, unsigned short *use_vmalloc)
 {
-	if (size <= PAGE_SIZE)
-		return kmalloc(size, GFP_KERNEL);
-	else
-		return vmalloc(size);
+	void *data;
+
+	data = kmalloc(size, GFP_KERNEL);
+	if (data != NULL) {
+		*use_vmalloc = 0;
+		return data;
+	}
+	*use_vmalloc = 1;
+
+	return vmalloc(size);
 }
 
 static inline void free_fdarr(struct fdtable *fdt)
 {
-	if (fdt->max_fds <= (PAGE_SIZE / sizeof(struct file *)))
-		kfree(fdt->fd);
-	else
+	if (fdt->fdarr_vmalloc)
 		vfree(fdt->fd);
+	else
+		kfree(fdt->fd);
 }
 
 static inline void free_fdset(struct fdtable *fdt)
 {
-	if (fdt->max_fds <= (PAGE_SIZE * BITS_PER_BYTE / 2))
-		kfree(fdt->open_fds);
-	else
+	if (fdt->fdset_vmalloc)
 		vfree(fdt->open_fds);
+	else
+		kfree(fdt->open_fds);
 }
 
 static void free_fdtable_work(struct work_struct *work)
@@ -167,12 +173,13 @@ static struct fdtable * alloc_fdtable(unsigned int nr)
 	if (!fdt)
 		goto out;
 	fdt->max_fds = nr;
-	data = alloc_fdmem(nr * sizeof(struct file *));
+	data = alloc_fdmem(nr * sizeof(struct file *), &fdt->fdarr_vmalloc);
 	if (!data)
 		goto out_fdt;
 	fdt->fd = (struct file **)data;
 	data = alloc_fdmem(max_t(unsigned int,
-				 2 * nr / BITS_PER_BYTE, L1_CACHE_BYTES));
+				 2 * nr / BITS_PER_BYTE, L1_CACHE_BYTES),
+			   &fdt->fdset_vmalloc);
 	if (!data)
 		goto out_arr;
 	fdt->open_fds = (fd_set *)data;
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 013dc52..1e730bc 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -30,6 +30,8 @@ struct embedded_fd_set {
 
 struct fdtable {
 	unsigned int max_fds;
+	unsigned short fdarr_vmalloc;
+	unsigned short fdset_vmalloc;
 	struct file ** fd;      /* current fd array */
 	fd_set *close_on_exec;
 	fd_set *open_fds;

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-02 16:46 ` [PATCH] fs: use kmalloc() to allocate fdmem if possible Changli Gao
@ 2010-05-02 17:31   ` Avi Kivity
  2010-05-03  0:15     ` Tetsuo Handa
  0 siblings, 1 reply; 18+ messages in thread
From: Avi Kivity @ 2010-05-02 17:31 UTC (permalink / raw)
  To: Changli Gao
  Cc: Jiri Slaby, Andrew Morton, Paul E. McKenney, Alexey Dobriyan,
	Ingo Molnar, Peter Zijlstra, linux-fsdevel, linux-kernel

On 05/02/2010 07:46 PM, Changli Gao wrote:
> use kmalloc() to allocate fdmem if possible.
>
> vmalloc() is used as a fallback solution for fdmem allocation. Two members are
> added into the hole of the structure fdtable to indicate if vmalloc() is used
> or not.
>
>
> diff --git a/fs/file.c b/fs/file.c
> index 34bb7f7..f71dd85 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -39,28 +39,34 @@ 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, unsigned short *use_vmalloc)
>   {
> -	if (size<= PAGE_SIZE)
> -		return kmalloc(size, GFP_KERNEL);
> -	else
> -		return vmalloc(size);
> +	void *data;
> +
> +	data = kmalloc(size, GFP_KERNEL);
> +	if (data != NULL) {
> +		*use_vmalloc = 0;
> +		return data;
> +	}
> +	*use_vmalloc = 1;
> +
> +	return vmalloc(size);
>   }
>    

Perhaps vmalloc() should do this by itself?  vfree() can examine the 
pointer and call kfree() if it isn't within the vmalloc address range.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-02 17:31   ` Avi Kivity
@ 2010-05-03  0:15     ` Tetsuo Handa
  2010-05-03  6:06       ` Avi Kivity
  0 siblings, 1 reply; 18+ messages in thread
From: Tetsuo Handa @ 2010-05-03  0:15 UTC (permalink / raw)
  To: avi, xiaosuo
  Cc: jslaby, akpm, paulmck, adobriyan, mingo, peterz, linux-fsdevel,
	linux-kernel

Avi Kivity wrote:
> On 05/02/2010 07:46 PM, Changli Gao wrote:
> > use kmalloc() to allocate fdmem if possible.
> >
> > vmalloc() is used as a fallback solution for fdmem allocation. Two members are
> > added into the hole of the structure fdtable to indicate if vmalloc() is used
> > or not.
> >
> >
> > diff --git a/fs/file.c b/fs/file.c
> > index 34bb7f7..f71dd85 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -39,28 +39,34 @@ 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, unsigned short *use_vmalloc)
> >   {
> > -	if (size<= PAGE_SIZE)
> > -		return kmalloc(size, GFP_KERNEL);
> > -	else
> > -		return vmalloc(size);
> > +	void *data;
> > +
> > +	data = kmalloc(size, GFP_KERNEL);
> > +	if (data != NULL) {
> > +		*use_vmalloc = 0;
> > +		return data;
> > +	}
> > +	*use_vmalloc = 1;
> > +
> > +	return vmalloc(size);
> >   }
> >    
> 
> Perhaps vmalloc() should do this by itself?  vfree() can examine the 
> pointer and call kfree() if it isn't within the vmalloc address range.

You can use is_vmalloc_addr().

	if (is_vmalloc_addr(p))
		vfree(p);
	else
		kfree(p);

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  0:15     ` Tetsuo Handa
@ 2010-05-03  6:06       ` Avi Kivity
  2010-05-03  7:05         ` Changli Gao
  0 siblings, 1 reply; 18+ messages in thread
From: Avi Kivity @ 2010-05-03  6:06 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: xiaosuo, jslaby, akpm, paulmck, adobriyan, mingo, peterz,
	linux-fsdevel, linux-kernel

On 05/03/2010 03:15 AM, Tetsuo Handa wrote:
> Avi Kivity wrote:
>    
>> On 05/02/2010 07:46 PM, Changli Gao wrote:
>>      
>>> use kmalloc() to allocate fdmem if possible.
>>>
>>> vmalloc() is used as a fallback solution for fdmem allocation. Two members are
>>> added into the hole of the structure fdtable to indicate if vmalloc() is used
>>> or not.
>>>
>>>
>>> diff --git a/fs/file.c b/fs/file.c
>>> index 34bb7f7..f71dd85 100644
>>> --- a/fs/file.c
>>> +++ b/fs/file.c
>>> @@ -39,28 +39,34 @@ 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, unsigned short *use_vmalloc)
>>>    {
>>> -	if (size<= PAGE_SIZE)
>>> -		return kmalloc(size, GFP_KERNEL);
>>> -	else
>>> -		return vmalloc(size);
>>> +	void *data;
>>> +
>>> +	data = kmalloc(size, GFP_KERNEL);
>>> +	if (data != NULL) {
>>> +		*use_vmalloc = 0;
>>> +		return data;
>>> +	}
>>> +	*use_vmalloc = 1;
>>> +
>>> +	return vmalloc(size);
>>>    }
>>>
>>>        
>> Perhaps vmalloc() should do this by itself?  vfree() can examine the
>> pointer and call kfree() if it isn't within the vmalloc address range.
>>      
> You can use is_vmalloc_addr().
>
> 	if (is_vmalloc_addr(p))
> 		vfree(p);
> 	else
> 		kfree(p);
>    


My point is, vmalloc() and vfree should do this, not their callers:

vmalloc(size):
     if (size <= PAGE_SIZE)
         return kmalloc(size, GFP_KERNEL);
     ...

vfree(p):
     if (!is_vmalloc_addr(p) {
         kfree(p);
         return;
     }
     ...

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  6:06       ` Avi Kivity
@ 2010-05-03  7:05         ` Changli Gao
  2010-05-03  7:42           ` Avi Kivity
  0 siblings, 1 reply; 18+ messages in thread
From: Changli Gao @ 2010-05-03  7:05 UTC (permalink / raw)
  To: Avi Kivity
  Cc: Tetsuo Handa, jslaby, akpm, paulmck, adobriyan, mingo, peterz,
	linux-fsdevel, linux-kernel

On Mon, May 3, 2010 at 2:06 PM, Avi Kivity <avi@redhat.com> wrote:
>
> My point is, vmalloc() and vfree should do this, not their callers:
>
> vmalloc(size):
>    if (size <= PAGE_SIZE)
>        return kmalloc(size, GFP_KERNEL);
>    ...
>
> vfree(p):
>    if (!is_vmalloc_addr(p) {
>        kfree(p);
>        return;
>    }
>    ...

I think we should not change vmalloc/vfree, and you can invent new
memory APIs, such as malloc()/free().

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)
--
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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  7:05         ` Changli Gao
@ 2010-05-03  7:42           ` Avi Kivity
  2010-05-03  7:52             ` Changli Gao
  0 siblings, 1 reply; 18+ messages in thread
From: Avi Kivity @ 2010-05-03  7:42 UTC (permalink / raw)
  To: Changli Gao
  Cc: Tetsuo Handa, jslaby, akpm, paulmck, adobriyan, mingo, peterz,
	linux-fsdevel, linux-kernel

On 05/03/2010 10:05 AM, Changli Gao wrote:
> On Mon, May 3, 2010 at 2:06 PM, Avi Kivity<avi@redhat.com>  wrote:
>    
>> My point is, vmalloc() and vfree should do this, not their callers:
>>
>> vmalloc(size):
>>     if (size<= PAGE_SIZE)
>>         return kmalloc(size, GFP_KERNEL);
>>     ...
>>
>> vfree(p):
>>     if (!is_vmalloc_addr(p) {
>>         kfree(p);
>>         return;
>>     }
>>     ...
>>      
> I think we should not change vmalloc/vfree, and you can invent new
> memory APIs, such as malloc()/free().
>    

Why?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  7:42           ` Avi Kivity
@ 2010-05-03  7:52             ` Changli Gao
  2010-05-03  8:49               ` Avi Kivity
  0 siblings, 1 reply; 18+ messages in thread
From: Changli Gao @ 2010-05-03  7:52 UTC (permalink / raw)
  To: Avi Kivity
  Cc: Tetsuo Handa, jslaby, akpm, paulmck, adobriyan, mingo, peterz,
	linux-fsdevel, linux-kernel

On Mon, May 3, 2010 at 3:42 PM, Avi Kivity <avi@redhat.com> wrote:
> On 05/03/2010 10:05 AM, Changli Gao wrote:
>>
>> On Mon, May 3, 2010 at 2:06 PM, Avi Kivity<avi@redhat.com>  wrote:
>>
>>>
>>> My point is, vmalloc() and vfree should do this, not their callers:
>>>
>>> vmalloc(size):
>>>    if (size<= PAGE_SIZE)
>>>        return kmalloc(size, GFP_KERNEL);
>>>    ...
>>>
>>> vfree(p):
>>>    if (!is_vmalloc_addr(p) {
>>>        kfree(p);
>>>        return;
>>>    }
>>>    ...
>>>
>>
>> I think we should not change vmalloc/vfree, and you can invent new
>> memory APIs, such as malloc()/free().
>>
>
> Why?
>

Because vmalloc is used to allocate virtually contiguous memory. v in
vmalloc means virtually.


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)
--
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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  7:52             ` Changli Gao
@ 2010-05-03  8:49               ` Avi Kivity
  2010-05-03  9:03                 ` Jiri Slaby
  2010-05-03  9:13                 ` Eric Dumazet
  0 siblings, 2 replies; 18+ messages in thread
From: Avi Kivity @ 2010-05-03  8:49 UTC (permalink / raw)
  To: Changli Gao
  Cc: Tetsuo Handa, jslaby, akpm, paulmck, adobriyan, mingo, peterz,
	linux-fsdevel, linux-kernel

On 05/03/2010 10:52 AM, Changli Gao wrote:
> On Mon, May 3, 2010 at 3:42 PM, Avi Kivity<avi@redhat.com>  wrote:
>    
>> On 05/03/2010 10:05 AM, Changli Gao wrote:
>>      
>>> On Mon, May 3, 2010 at 2:06 PM, Avi Kivity<avi@redhat.com>    wrote:
>>>
>>>        
>>>> My point is, vmalloc() and vfree should do this, not their callers:
>>>>
>>>> vmalloc(size):
>>>>     if (size<= PAGE_SIZE)
>>>>         return kmalloc(size, GFP_KERNEL);
>>>>     ...
>>>>
>>>> vfree(p):
>>>>     if (!is_vmalloc_addr(p) {
>>>>         kfree(p);
>>>>         return;
>>>>     }
>>>>     ...
>>>>
>>>>          
>>> I think we should not change vmalloc/vfree, and you can invent new
>>> memory APIs, such as malloc()/free().
>>>
>>>        
>> Why?
>>
>>      
> Because vmalloc is used to allocate virtually contiguous memory. v in
> vmalloc means virtually.
>
>    

A kmalloc()ed page is virtually contiguous, satisfying your requirement.

-- 
error compiling committee.c: too many arguments to function


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  8:49               ` Avi Kivity
@ 2010-05-03  9:03                 ` Jiri Slaby
  2010-05-03  9:18                   ` Avi Kivity
  2010-05-03  9:13                 ` Eric Dumazet
  1 sibling, 1 reply; 18+ messages in thread
From: Jiri Slaby @ 2010-05-03  9:03 UTC (permalink / raw)
  To: Avi Kivity
  Cc: Changli Gao, Tetsuo Handa, akpm, paulmck, adobriyan, mingo,
	peterz, linux-fsdevel, linux-kernel

On 05/03/2010 10:49 AM, Avi Kivity wrote:
> On 05/03/2010 10:52 AM, Changli Gao wrote:
>> On Mon, May 3, 2010 at 3:42 PM, Avi Kivity<avi@redhat.com>  wrote:
>>   
>>> On 05/03/2010 10:05 AM, Changli Gao wrote:
>>>     
>>>> On Mon, May 3, 2010 at 2:06 PM, Avi Kivity<avi@redhat.com>    wrote:
>>>>
>>>>       
>>>>> My point is, vmalloc() and vfree should do this, not their callers:
>>>>>
>>>>> vmalloc(size):
>>>>>     if (size<= PAGE_SIZE)
>>>>>         return kmalloc(size, GFP_KERNEL);
>>>>>     ...
>>>>>
>>>>> vfree(p):
>>>>>     if (!is_vmalloc_addr(p) {
>>>>>         kfree(p);
>>>>>         return;
>>>>>     }
>>>>>     ...
>>>>>
>>>>>          
>>>> I think we should not change vmalloc/vfree, and you can invent new
>>>> memory APIs, such as malloc()/free().
>>>>
>>>>        
>>> Why?
>>>
>>>      
>> Because vmalloc is used to allocate virtually contiguous memory. v in
>> vmalloc means virtually.
>>
>>    
> 
> A kmalloc()ed page is virtually contiguous, satisfying your requirement.

But it won't work well for vmalloc_to_{page,pfn} and similar. Some code
may expect vmalloc result to be in the vmalloc area and page-aligned
(both in position and size).

Not that it won't be possible to inspect the callers, but in my eyes it
would definitely be better to introduce kmalloc_or_vmalloc-alike where
the caller explicitly doesn't care about the resulting position and size.

regards,
-- 
js
suse labs

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  8:49               ` Avi Kivity
  2010-05-03  9:03                 ` Jiri Slaby
@ 2010-05-03  9:13                 ` Eric Dumazet
  2010-05-03  9:19                   ` Jiri Slaby
  2010-05-04  4:20                   ` Changli Gao
  1 sibling, 2 replies; 18+ messages in thread
From: Eric Dumazet @ 2010-05-03  9:13 UTC (permalink / raw)
  To: Avi Kivity, Changli Gao
  Cc: Tetsuo Handa, jslaby, akpm, paulmck, adobriyan, mingo, peterz,
	linux-fsdevel, linux-kernel

Le lundi 03 mai 2010 à 11:49 +0300, Avi Kivity a écrit :

> A kmalloc()ed page is virtually contiguous, satisfying your requirement.
> 

Still, a different function pair is needed, at least for documentation
reasons.

Changli, _many_ patches like yours were suggested and refused in the
past for the reason it should be generic. Maybe its time...

So first introduce a pair of funtions, then we can use them.

At free time we can use is_vmalloc_addr() so that no extra bit is needed
to tell if a zone was vmalloced() or kmalloced()


One remark :

+       data = kmalloc(size, GFP_KERNEL);

If allocation fails (because of fragmentation), we would have a kernel
log.
Still, vmalloc() might be OK.
So you should add a __GFP_NOWARN flag.




/*
 * Warning: if size is not a power of two, kmalloc() might
 * waste memory because of its requirements.
 */
void *kvmalloc(size_t size)
{
	void *ptr = kmalloc(size, GFP_KERNEL | __GFP_NOWARN);

	if (ptr)
		return ptr;
	return vmalloc(size);
}

void kvfree(void *ptr)
{
	BUG_ON(in_interrupt());
	if (is_vmalloc_addr(ptr))
		vfree(ptr);
	else
		kfree(ptr);
}

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  9:03                 ` Jiri Slaby
@ 2010-05-03  9:18                   ` Avi Kivity
  2010-05-03  9:22                     ` Jiri Slaby
  0 siblings, 1 reply; 18+ messages in thread
From: Avi Kivity @ 2010-05-03  9:18 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: Changli Gao, Tetsuo Handa, akpm, paulmck, adobriyan, mingo,
	peterz, linux-fsdevel, linux-kernel

On 05/03/2010 12:03 PM, Jiri Slaby wrote:
>
>    
>>> Because vmalloc is used to allocate virtually contiguous memory. v in
>>> vmalloc means virtually.
>>>
>>>
>>>        
>> A kmalloc()ed page is virtually contiguous, satisfying your requirement.
>>      
> But it won't work well for vmalloc_to_{page,pfn} and similar.

Modify vmalloc_to_{page,pfn} accordingly.

> Some code
> may expect vmalloc result to be in the vmalloc area and page-aligned
> (both in position and size).
>    

Both would be a bug IMO.  vmalloc() follows kmalloc() and malloc() which 
only guarantee natural alignment.

> Not that it won't be possible to inspect the callers, but in my eyes it
> would definitely be better to introduce kmalloc_or_vmalloc-alike where
> the caller explicitly doesn't care about the resulting position and size.
>    

You're forcing every user to make a choice about what's essentially a 
micro optimization.

If it's really needed to have a vmalloc-in-vmalloc-space, that should be 
a special function, while the ordinary vmalloc should make the optimization.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  9:13                 ` Eric Dumazet
@ 2010-05-03  9:19                   ` Jiri Slaby
  2010-05-03  9:29                     ` Eric Dumazet
  2010-05-04  4:20                   ` Changli Gao
  1 sibling, 1 reply; 18+ messages in thread
From: Jiri Slaby @ 2010-05-03  9:19 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Avi Kivity, Changli Gao, Tetsuo Handa, akpm, paulmck, adobriyan,
	mingo, peterz, linux-fsdevel, linux-kernel

On 05/03/2010 11:13 AM, Eric Dumazet wrote:
> /*
>  * Warning: if size is not a power of two, kmalloc() might
>  * waste memory because of its requirements.
>  */
> void *kvmalloc(size_t size)
> {
> 	void *ptr = kmalloc(size, GFP_KERNEL | __GFP_NOWARN);

This is kind of waste of high-order pages, isn't it? Wouldn't it be
better to have a watermark where we skip kmalloc completely?

> 
> 	if (ptr)
> 		return ptr;
> 	return vmalloc(size);
> }



-- 
js
suse labs

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  9:18                   ` Avi Kivity
@ 2010-05-03  9:22                     ` Jiri Slaby
  2010-05-03  9:25                       ` Jiri Slaby
  2010-05-03  9:28                       ` Avi Kivity
  0 siblings, 2 replies; 18+ messages in thread
From: Jiri Slaby @ 2010-05-03  9:22 UTC (permalink / raw)
  To: Avi Kivity
  Cc: Changli Gao, Tetsuo Handa, akpm, paulmck, adobriyan, mingo,
	peterz, linux-fsdevel, linux-kernel

On 05/03/2010 11:18 AM, Avi Kivity wrote:
> On 05/03/2010 12:03 PM, Jiri Slaby wrote:
>>>> Because vmalloc is used to allocate virtually contiguous memory. v in
>>>> vmalloc means virtually.
>>>>
>>>>
>>>>        
>>> A kmalloc()ed page is virtually contiguous, satisfying your requirement.
>>>      
>> But it won't work well for vmalloc_to_{page,pfn} and similar.
> 
> Modify vmalloc_to_{page,pfn} accordingly.

When you get a slub in the middle of page, how?

>> Some code
>> may expect vmalloc result to be in the vmalloc area and page-aligned
>> (both in position and size).
>>    
> 
> Both would be a bug IMO.  vmalloc() follows kmalloc() and malloc() which
> only guarantee natural alignment.

Nope, from what I understand, vmalloc aligns (uses page allocator). Even
for purposes of vmalloc_to_*.

regards,
-- 
js
suse labs

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  9:22                     ` Jiri Slaby
@ 2010-05-03  9:25                       ` Jiri Slaby
  2010-05-03  9:28                       ` Avi Kivity
  1 sibling, 0 replies; 18+ messages in thread
From: Jiri Slaby @ 2010-05-03  9:25 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: Avi Kivity, Changli Gao, Tetsuo Handa, akpm, paulmck, adobriyan,
	mingo, peterz, linux-fsdevel, linux-kernel

On 05/03/2010 11:22 AM, Jiri Slaby wrote:
> When you get a slub in the middle of page

What I was thinking about? I meant when you get a piece of memory from
slab allocator which is in the middle of a page.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  9:22                     ` Jiri Slaby
  2010-05-03  9:25                       ` Jiri Slaby
@ 2010-05-03  9:28                       ` Avi Kivity
  1 sibling, 0 replies; 18+ messages in thread
From: Avi Kivity @ 2010-05-03  9:28 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: Changli Gao, Tetsuo Handa, akpm, paulmck, adobriyan, mingo,
	peterz, linux-fsdevel, linux-kernel

On 05/03/2010 12:22 PM, Jiri Slaby wrote:
> On 05/03/2010 11:18 AM, Avi Kivity wrote:
>    
>> On 05/03/2010 12:03 PM, Jiri Slaby wrote:
>>      
>>>>> Because vmalloc is used to allocate virtually contiguous memory. v in
>>>>> vmalloc means virtually.
>>>>>
>>>>>
>>>>>
>>>>>            
>>>> A kmalloc()ed page is virtually contiguous, satisfying your requirement.
>>>>
>>>>          
>>> But it won't work well for vmalloc_to_{page,pfn} and similar.
>>>        
>> Modify vmalloc_to_{page,pfn} accordingly.
>>      
> When you get a slub in the middle of page, how?
>    

Er, right.  So a separate API is needed for users who are interested in 
memory, not pages.

>>> Some code
>>> may expect vmalloc result to be in the vmalloc area and page-aligned
>>> (both in position and size).
>>>
>>>        
>> Both would be a bug IMO.  vmalloc() follows kmalloc() and malloc() which
>> only guarantee natural alignment.
>>      
> Nope, from what I understand, vmalloc aligns (uses page allocator). Even
> for purposes of vmalloc_to_*.
>    

Correct, my mistake.

-- 
error compiling committee.c: too many arguments to function

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  9:19                   ` Jiri Slaby
@ 2010-05-03  9:29                     ` Eric Dumazet
  0 siblings, 0 replies; 18+ messages in thread
From: Eric Dumazet @ 2010-05-03  9:29 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: Avi Kivity, Changli Gao, Tetsuo Handa, akpm, paulmck, adobriyan,
	mingo, peterz, linux-fsdevel, linux-kernel

Le lundi 03 mai 2010 à 11:19 +0200, Jiri Slaby a écrit :
> On 05/03/2010 11:13 AM, Eric Dumazet wrote:
> > /*
> >  * Warning: if size is not a power of two, kmalloc() might
> >  * waste memory because of its requirements.
> >  */
> > void *kvmalloc(size_t size)
> > {
> > 	void *ptr = kmalloc(size, GFP_KERNEL | __GFP_NOWARN);
> 
> This is kind of waste of high-order pages, isn't it? Wouldn't it be
> better to have a watermark where we skip kmalloc completely?

> 
> 

There are many drawbacks using kmalloc() instead of vmalloc().

(NUMA comes to mind, if you want a zone spreaded on all available nodes)

So this kvmalloc() is not meant to be a generic replacement of all
vmalloc() users, only selected ones.

In some spots, allocating 8192 bytes in a driver setup for example, it
should be fine. Using a high order page for this is perfect, as long as
we can have a fallback to vmalloc()

grep bnx2_alloc_mem /proc/vmallocinfo
0xf8690000-0xf8692000    8192 bnx2_alloc_mem+0x111/0x6d0 pages=1 vmalloc


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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-03  9:13                 ` Eric Dumazet
  2010-05-03  9:19                   ` Jiri Slaby
@ 2010-05-04  4:20                   ` Changli Gao
  2010-05-04  5:33                     ` Eric Dumazet
  1 sibling, 1 reply; 18+ messages in thread
From: Changli Gao @ 2010-05-04  4:20 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Avi Kivity, Tetsuo Handa, jslaby, akpm, paulmck, adobriyan, mingo,
	peterz, linux-fsdevel, linux-kernel

On Mon, May 3, 2010 at 5:13 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> /*
>  * Warning: if size is not a power of two, kmalloc() might
>  * waste memory because of its requirements.
>  */
> void *kvmalloc(size_t size)
> {
>        void *ptr = kmalloc(size, GFP_KERNEL | __GFP_NOWARN);
>
>        if (ptr)
>                return ptr;
>        return vmalloc(size);
> }

we can use alloc_pages_exact()/free_pages_exact() when size is larger
than PAGE_SIZE, then size isn't needed to be a power of two.

void *kvmalloc(size_t size)
{
      void *ptr;

      if (size < PAGE_SIZE)
          return kmalloc(size, GFP_KERNEL);
      ptr = alloc_pages_exact(size, GFP_KERNEL | __GFP_NOWARN);
      if (ptr)
          return ptr;
      return vmalloc(size);
}

void *kvfree(void *ptr, size_t size)
{
       BUG_ON(in_interrupt());
       if (size < PAGE_SIZE)
            kfree(ptr);
       else if (is_vmalloc_addr(ptr))
            vfree(ptr);
       else
            free_pages_exact(ptr, size);
}

>
> void kvfree(void *ptr)
> {
>        BUG_ON(in_interrupt());
>        if (is_vmalloc_addr(ptr))
>                vfree(ptr);
>        else
>                kfree(ptr);
> }
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs: use kmalloc() to allocate fdmem if possible
  2010-05-04  4:20                   ` Changli Gao
@ 2010-05-04  5:33                     ` Eric Dumazet
  0 siblings, 0 replies; 18+ messages in thread
From: Eric Dumazet @ 2010-05-04  5:33 UTC (permalink / raw)
  To: Changli Gao
  Cc: Avi Kivity, Tetsuo Handa, jslaby, akpm, paulmck, adobriyan, mingo,
	peterz, linux-fsdevel, linux-kernel

Le mardi 04 mai 2010 à 12:20 +0800, Changli Gao a écrit :

> we can use alloc_pages_exact()/free_pages_exact() when size is larger
> than PAGE_SIZE, then size isn't needed to be a power of two.
> 
> void *kvmalloc(size_t size)
> {
>       void *ptr;
> 
>       if (size < PAGE_SIZE)
>           return kmalloc(size, GFP_KERNEL);
>       ptr = alloc_pages_exact(size, GFP_KERNEL | __GFP_NOWARN);
>       if (ptr)
>           return ptr;
>       return vmalloc(size);
> }
> 
> void *kvfree(void *ptr, size_t size)
> {
>        BUG_ON(in_interrupt());
>        if (size < PAGE_SIZE)
>             kfree(ptr);
>        else if (is_vmalloc_addr(ptr))
>             vfree(ptr);
>        else
>             free_pages_exact(ptr, size);
> }

Yes, this has the drawback to need 'size' in kvfree(), but it should not
be a problem for users of this code.

alloc_pages_exact() also fragments high order page, but I guess it's not
worse than kmalloc() in this context.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2010-05-04  5:33 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <Alexander Viro <viro@zeniv.linux.org.uk>
2010-05-02 16:46 ` [PATCH] fs: use kmalloc() to allocate fdmem if possible Changli Gao
2010-05-02 17:31   ` Avi Kivity
2010-05-03  0:15     ` Tetsuo Handa
2010-05-03  6:06       ` Avi Kivity
2010-05-03  7:05         ` Changli Gao
2010-05-03  7:42           ` Avi Kivity
2010-05-03  7:52             ` Changli Gao
2010-05-03  8:49               ` Avi Kivity
2010-05-03  9:03                 ` Jiri Slaby
2010-05-03  9:18                   ` Avi Kivity
2010-05-03  9:22                     ` Jiri Slaby
2010-05-03  9:25                       ` Jiri Slaby
2010-05-03  9:28                       ` Avi Kivity
2010-05-03  9:13                 ` Eric Dumazet
2010-05-03  9:19                   ` Jiri Slaby
2010-05-03  9:29                     ` Eric Dumazet
2010-05-04  4:20                   ` Changli Gao
2010-05-04  5:33                     ` Eric Dumazet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).