[PATCH] binder: Use kmem_cache for binder_thread

[PATCH] binder: Use kmem_cache for binder_thread

[Peikan Tsai]

Hi,

The allocated size for each binder_thread is 512 bytes by kzalloc.
Because the size of binder_thread is fixed and it's only 304 bytes.
It will save 208 bytes per binder_thread when use create a kmem_cache
for the binder_thread.

Signed-off-by: Peikan Tsai <peikantsai@gmail.com>
---
 drivers/android/binder.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index dc1c83eafc22..043e0ebd0fe7 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -87,6 +87,8 @@ static struct dentry *binder_debugfs_dir_entry_root;
 static struct dentry *binder_debugfs_dir_entry_proc;
 static atomic_t binder_last_id;

+static struct kmem_cache *binder_thread_cachep;
+
 static int proc_show(struct seq_file *m, void *unused);
 DEFINE_SHOW_ATTRIBUTE(proc);

@@ -4696,14 +4698,15 @@ static struct binder_thread *binder_get_thread(struct binder_proc *proc)
 	thread = binder_get_thread_ilocked(proc, NULL);
 	binder_inner_proc_unlock(proc);
 	if (!thread) {
-		new_thread = kzalloc(sizeof(*thread), GFP_KERNEL);
+		new_thread = kmem_cache_zalloc(binder_thread_cachep,
+					       GFP_KERNEL);
 		if (new_thread == NULL)
 			return NULL;
 		binder_inner_proc_lock(proc);
 		thread = binder_get_thread_ilocked(proc, new_thread);
 		binder_inner_proc_unlock(proc);
 		if (thread != new_thread)
-			kfree(new_thread);
+			kmem_cache_free(binder_thread_cachep, new_thread);
 	}
 	return thread;
 }
@@ -4723,7 +4726,7 @@ static void binder_free_thread(struct binder_thread *thread)
 	BUG_ON(!list_empty(&thread->todo));
 	binder_stats_deleted(BINDER_STAT_THREAD);
 	binder_proc_dec_tmpref(thread->proc);
-	kfree(thread);
+	kmem_cache_free(binder_thread_cachep, thread);
 }

 static int binder_thread_release(struct binder_proc *proc,
@@ -6095,6 +6098,12 @@ static int __init binder_init(void)
 	if (ret)
 		return ret;

+	binder_thread_cachep = kmem_cache_create("binder_thread",
+						 sizeof(struct binder_thread),
+						 0, 0, NULL);
+	if (!binder_thread_cachep)
+		return -ENOMEM;
+
 	atomic_set(&binder_transaction_log.cur, ~0U);
 	atomic_set(&binder_transaction_log_failed.cur, ~0U);

@@ -6167,6 +6176,7 @@ static int __init binder_init(void)

 err_alloc_device_names_failed:
 	debugfs_remove_recursive(binder_debugfs_dir_entry_root);
+	kmem_cache_destroy(binder_thread_cachep);

 	return ret;
 }
--
2.17.1

Re: [PATCH] binder: Use kmem_cache for binder_thread

[Greg KH]

On Thu, Aug 29, 2019 at 01:49:53PM +0800, Peikan Tsai wrote:
> Hi,

No need for that in a changelog text :)

> The allocated size for each binder_thread is 512 bytes by kzalloc.
> Because the size of binder_thread is fixed and it's only 304 bytes.
> It will save 208 bytes per binder_thread when use create a kmem_cache
> for the binder_thread.

Are you _sure_ it really will save that much memory?  You want to do
allocations based on a nice alignment for lots of good reasons,
especially for something that needs quick accesses.

Did you test your change on a system that relies on binder and find any
speed improvement or decrease, and any actual memory savings?

If so, can you post your results?

thanks,

greg k-h

Re: [PATCH] binder: Use kmem_cache for binder_thread

[Joel Fernandes]

On Thu, Aug 29, 2019 at 08:42:29AM +0200, Greg KH wrote:
> On Thu, Aug 29, 2019 at 01:49:53PM +0800, Peikan Tsai wrote:
[snip] 
> > The allocated size for each binder_thread is 512 bytes by kzalloc.
> > Because the size of binder_thread is fixed and it's only 304 bytes.
> > It will save 208 bytes per binder_thread when use create a kmem_cache
> > for the binder_thread.
> 
> Are you _sure_ it really will save that much memory?  You want to do
> allocations based on a nice alignment for lots of good reasons,
> especially for something that needs quick accesses.

Alignment can be done for slab allocations, kmem_cache_create() takes an
align argument. I am not sure what the default alignment of objects is
though (probably no default alignment). What is an optimal alignment in your
view?

> Did you test your change on a system that relies on binder and find any
> speed improvement or decrease, and any actual memory savings?
> 
> If so, can you post your results?

That's certainly worth it and I thought of asking for the same, but spoke too
soon!

Independent note: In general I find the internal fragmentation with large
kmalloc()s troubling in the kernel :-(. Say you have a 5000 objects of 512
allocation, each 300 bytes. 212 * 5000 is around 1MB. Which is arguably not
neglible on a small memory system, right?

thanks,

 - Joel

Re: [PATCH] binder: Use kmem_cache for binder_thread

[Christian Brauner]

On Thu, Aug 29, 2019 at 09:53:59AM -0400, Joel Fernandes wrote:
> On Thu, Aug 29, 2019 at 08:42:29AM +0200, Greg KH wrote:
> > On Thu, Aug 29, 2019 at 01:49:53PM +0800, Peikan Tsai wrote:
> [snip] 
> > > The allocated size for each binder_thread is 512 bytes by kzalloc.
> > > Because the size of binder_thread is fixed and it's only 304 bytes.
> > > It will save 208 bytes per binder_thread when use create a kmem_cache
> > > for the binder_thread.
> > 
> > Are you _sure_ it really will save that much memory?  You want to do
> > allocations based on a nice alignment for lots of good reasons,
> > especially for something that needs quick accesses.
> 
> Alignment can be done for slab allocations, kmem_cache_create() takes an
> align argument. I am not sure what the default alignment of objects is
> though (probably no default alignment). What is an optimal alignment in your
> view?

Probably SLAB_HWCACHE_ALIGN would make most sense.

> 
> > Did you test your change on a system that relies on binder and find any
> > speed improvement or decrease, and any actual memory savings?
> > 
> > If so, can you post your results?
> 
> That's certainly worth it and I thought of asking for the same, but spoke too
> soon!

Yeah, it'd be interesting to see what difference this actually makes. 

Christian

Re: [PATCH] binder: Use kmem_cache for binder_thread

[Peikan Tsai]

On Thu, Aug 29, 2019 at 05:27:22PM +0200, Christian Brauner wrote:
> On Thu, Aug 29, 2019 at 09:53:59AM -0400, Joel Fernandes wrote:
> > On Thu, Aug 29, 2019 at 08:42:29AM +0200, Greg KH wrote:
> > > On Thu, Aug 29, 2019 at 01:49:53PM +0800, Peikan Tsai wrote:
> > [snip] 
> > > > The allocated size for each binder_thread is 512 bytes by kzalloc.
> > > > Because the size of binder_thread is fixed and it's only 304 bytes.
> > > > It will save 208 bytes per binder_thread when use create a kmem_cache
> > > > for the binder_thread.
> > > 
> > > Are you _sure_ it really will save that much memory?  You want to do
> > > allocations based on a nice alignment for lots of good reasons,
> > > especially for something that needs quick accesses.
> > 
> > Alignment can be done for slab allocations, kmem_cache_create() takes an
> > align argument. I am not sure what the default alignment of objects is
> > though (probably no default alignment). What is an optimal alignment in your
> > view?
> 
> Probably SLAB_HWCACHE_ALIGN would make most sense.
> 

Agree. Thanks for yours comments and suggestions.
I'll put SLAB_HWCACHE_ALIGN it in patch v2.

> > 
> > > Did you test your change on a system that relies on binder and find any
> > > speed improvement or decrease, and any actual memory savings?
> > > 
> > > If so, can you post your results?
> > 
> > That's certainly worth it and I thought of asking for the same, but spoke too
> > soon!
> 
> Yeah, it'd be interesting to see what difference this actually makes. 
> 
> Christian

I tested this change on an Android device(arm) with AOSP kernel 4.19 and
observed
memory usage of binder_thread. But I didn't do binder benchmark yet.

On my platform the memory usage of binder_thread reduce about 90 KB as
the
following result.
        nr obj          obj size        total
	before: 624             512             319488 bytes
	after:  728             312             227136 bytes

Re: [PATCH] binder: Use kmem_cache for binder_thread

[joel]

On August 29, 2019 2:59:01 PM EDT, Peikan Tsai <peikantsai@gmail.com> wrote:
>On Thu, Aug 29, 2019 at 05:27:22PM +0200, Christian Brauner wrote:
>> On Thu, Aug 29, 2019 at 09:53:59AM -0400, Joel Fernandes wrote:
>> > On Thu, Aug 29, 2019 at 08:42:29AM +0200, Greg KH wrote:
>> > > On Thu, Aug 29, 2019 at 01:49:53PM +0800, Peikan Tsai wrote:
>> > [snip] 
>> > > > The allocated size for each binder_thread is 512 bytes by
>kzalloc.
>> > > > Because the size of binder_thread is fixed and it's only 304
>bytes.
>> > > > It will save 208 bytes per binder_thread when use create a
>kmem_cache
>> > > > for the binder_thread.
>> > > 
>> > > Are you _sure_ it really will save that much memory?  You want to
>do
>> > > allocations based on a nice alignment for lots of good reasons,
>> > > especially for something that needs quick accesses.
>> > 
>> > Alignment can be done for slab allocations, kmem_cache_create()
>takes an
>> > align argument. I am not sure what the default alignment of objects
>is
>> > though (probably no default alignment). What is an optimal
>alignment in your
>> > view?
>> 
>> Probably SLAB_HWCACHE_ALIGN would make most sense.
>> 
>
>Agree. Thanks for yours comments and suggestions.
>I'll put SLAB_HWCACHE_ALIGN it in patch v2.
>
>> > 
>> > > Did you test your change on a system that relies on binder and
>find any
>> > > speed improvement or decrease, and any actual memory savings?
>> > > 
>> > > If so, can you post your results?
>> > 
>> > That's certainly worth it and I thought of asking for the same, but
>spoke too
>> > soon!
>> 
>> Yeah, it'd be interesting to see what difference this actually makes.
>
>> 
>> Christian
>
>I tested this change on an Android device(arm) with AOSP kernel 4.19
>and
>observed
>memory usage of binder_thread. But I didn't do binder benchmark yet.
>
>On my platform the memory usage of binder_thread reduce about 90 KB as
>the
>following result.
>        nr obj          obj size        total
>	before: 624             512             319488 bytes
>	after:  728             312             227136 bytes

And add this to the changelog as well. Curious- why is nrobj higher with the patch?

Please don't use my reviewed-by tag yet and I will review the new patch and provide tag separately.

Thank you.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] binder: Use kmem_cache for binder_thread

[Greg KH]

On Fri, Aug 30, 2019 at 02:59:01AM +0800, Peikan Tsai wrote:
> On Thu, Aug 29, 2019 at 05:27:22PM +0200, Christian Brauner wrote:
> > On Thu, Aug 29, 2019 at 09:53:59AM -0400, Joel Fernandes wrote:
> > > On Thu, Aug 29, 2019 at 08:42:29AM +0200, Greg KH wrote:
> > > > On Thu, Aug 29, 2019 at 01:49:53PM +0800, Peikan Tsai wrote:
> > > [snip] 
> > > > > The allocated size for each binder_thread is 512 bytes by kzalloc.
> > > > > Because the size of binder_thread is fixed and it's only 304 bytes.
> > > > > It will save 208 bytes per binder_thread when use create a kmem_cache
> > > > > for the binder_thread.
> > > > 
> > > > Are you _sure_ it really will save that much memory?  You want to do
> > > > allocations based on a nice alignment for lots of good reasons,
> > > > especially for something that needs quick accesses.
> > > 
> > > Alignment can be done for slab allocations, kmem_cache_create() takes an
> > > align argument. I am not sure what the default alignment of objects is
> > > though (probably no default alignment). What is an optimal alignment in your
> > > view?
> > 
> > Probably SLAB_HWCACHE_ALIGN would make most sense.
> > 
> 
> Agree. Thanks for yours comments and suggestions.
> I'll put SLAB_HWCACHE_ALIGN it in patch v2.
> 
> > > 
> > > > Did you test your change on a system that relies on binder and find any
> > > > speed improvement or decrease, and any actual memory savings?
> > > > 
> > > > If so, can you post your results?
> > > 
> > > That's certainly worth it and I thought of asking for the same, but spoke too
> > > soon!
> > 
> > Yeah, it'd be interesting to see what difference this actually makes. 
> > 
> > Christian
> 
> I tested this change on an Android device(arm) with AOSP kernel 4.19 and
> observed
> memory usage of binder_thread. But I didn't do binder benchmark yet.
> 
> On my platform the memory usage of binder_thread reduce about 90 KB as
> the
> following result.
>         nr obj          obj size        total
> 	before: 624             512             319488 bytes
> 	after:  728             312             227136 bytes

You have more objects???

Re: [PATCH] binder: Use kmem_cache for binder_thread

[Peikan Tsai]

On Fri, Aug 30, 2019 at 08:39:43AM +0200, Greg KH wrote:
> On Fri, Aug 30, 2019 at 02:59:01AM +0800, Peikan Tsai wrote:
> > On Thu, Aug 29, 2019 at 05:27:22PM +0200, Christian Brauner wrote:
> > > On Thu, Aug 29, 2019 at 09:53:59AM -0400, Joel Fernandes wrote:
> > > > On Thu, Aug 29, 2019 at 08:42:29AM +0200, Greg KH wrote:
> > > > > On Thu, Aug 29, 2019 at 01:49:53PM +0800, Peikan Tsai wrote:
> > > > [snip] 
> > > > > > The allocated size for each binder_thread is 512 bytes by kzalloc.
> > > > > > Because the size of binder_thread is fixed and it's only 304 bytes.
> > > > > > It will save 208 bytes per binder_thread when use create a kmem_cache
> > > > > > for the binder_thread.
> > > > > 
> > > > > Are you _sure_ it really will save that much memory?  You want to do
> > > > > allocations based on a nice alignment for lots of good reasons,
> > > > > especially for something that needs quick accesses.
> > > > 
> > > > Alignment can be done for slab allocations, kmem_cache_create() takes an
> > > > align argument. I am not sure what the default alignment of objects is
> > > > though (probably no default alignment). What is an optimal alignment in your
> > > > view?
> > > 
> > > Probably SLAB_HWCACHE_ALIGN would make most sense.
> > > 
> > 
> > Agree. Thanks for yours comments and suggestions.
> > I'll put SLAB_HWCACHE_ALIGN it in patch v2.
> > 
> > > > 
> > > > > Did you test your change on a system that relies on binder and find any
> > > > > speed improvement or decrease, and any actual memory savings?
> > > > > 
> > > > > If so, can you post your results?
> > > > 
> > > > That's certainly worth it and I thought of asking for the same, but spoke too
> > > > soon!
> > > 
> > > Yeah, it'd be interesting to see what difference this actually makes. 
> > > 
> > > Christian
> > 
> > I tested this change on an Android device(arm) with AOSP kernel 4.19 and
> > observed
> > memory usage of binder_thread. But I didn't do binder benchmark yet.
> > 
> > On my platform the memory usage of binder_thread reduce about 90 KB as
> > the
> > following result.
> >         nr obj          obj size        total
> > 	before: 624             512             319488 bytes
> > 	after:  728             312             227136 bytes
> 
> You have more objects???
> 

Sorry, it's total objects which include some inactive objects ...
And because I tested it on an Android platform so there may be some noise.

So I try 'adb stop' and 'echo 3 > /proc/sys/vm/drop_caches' before starting
test to reduce the noise, and the result are as following.

                    objs
kzalloc              220  (kmalloc-512 alloc by binder_get_thread)

             active_objs  total objs   objperslab  slabdata
kmem_cache           194         403           13        31

Seems there are more objects when use kmemcache for binder_thread...
But as I understand it, those inactive objects can be free by kmemcahe shrink?

Also, I tested the throughput by using performace test of Android VTS.

size(bytes)	kzalloc(byte/ns)	kmemcache(byte/ns)
4		0.17			0.17
8		0.33			0.32
16		0.66			0.66
32		1.36			1.42
64		2.66			2.61
128		5.4			5.26
256		10.29			10.77
512		21.51			21.36
1k		41			40.26
2k		82.12			80.28
4k		149.24			146.95
8k		262.34			256
16k		417.96			422.2
32k		596.66			590.23
64k		600.84			601.25

Re: [PATCH] binder: Use kmem_cache for binder_thread

[Greg KH]

On Thu, Aug 29, 2019 at 05:27:22PM +0200, Christian Brauner wrote:
> On Thu, Aug 29, 2019 at 09:53:59AM -0400, Joel Fernandes wrote:
> > On Thu, Aug 29, 2019 at 08:42:29AM +0200, Greg KH wrote:
> > > On Thu, Aug 29, 2019 at 01:49:53PM +0800, Peikan Tsai wrote:
> > [snip] 
> > > > The allocated size for each binder_thread is 512 bytes by kzalloc.
> > > > Because the size of binder_thread is fixed and it's only 304 bytes.
> > > > It will save 208 bytes per binder_thread when use create a kmem_cache
> > > > for the binder_thread.
> > > 
> > > Are you _sure_ it really will save that much memory?  You want to do
> > > allocations based on a nice alignment for lots of good reasons,
> > > especially for something that needs quick accesses.
> > 
> > Alignment can be done for slab allocations, kmem_cache_create() takes an
> > align argument. I am not sure what the default alignment of objects is
> > though (probably no default alignment). What is an optimal alignment in your
> > view?
> 
> Probably SLAB_HWCACHE_ALIGN would make most sense.

This isn't memory accessing hardware, so I don't think it would, right?

Anyway, some actual performance tests need to be run to see if any of
this make any difference at all please...

thanks,

greg k-h


> 
> > 
> > > Did you test your change on a system that relies on binder and find any
> > > speed improvement or decrease, and any actual memory savings?
> > > 
> > > If so, can you post your results?
> > 
> > That's certainly worth it and I thought of asking for the same, but spoke too
> > soon!
> 
> Yeah, it'd be interesting to see what difference this actually makes. 
> 
> Christian

Re: [PATCH] binder: Use kmem_cache for binder_thread

[Christian Brauner]

On Fri, Aug 30, 2019 at 08:38:51AM +0200, Greg KH wrote:
> On Thu, Aug 29, 2019 at 05:27:22PM +0200, Christian Brauner wrote:
> > On Thu, Aug 29, 2019 at 09:53:59AM -0400, Joel Fernandes wrote:
> > > On Thu, Aug 29, 2019 at 08:42:29AM +0200, Greg KH wrote:
> > > > On Thu, Aug 29, 2019 at 01:49:53PM +0800, Peikan Tsai wrote:
> > > [snip] 
> > > > > The allocated size for each binder_thread is 512 bytes by kzalloc.
> > > > > Because the size of binder_thread is fixed and it's only 304 bytes.
> > > > > It will save 208 bytes per binder_thread when use create a kmem_cache
> > > > > for the binder_thread.
> > > > 
> > > > Are you _sure_ it really will save that much memory?  You want to do
> > > > allocations based on a nice alignment for lots of good reasons,
> > > > especially for something that needs quick accesses.
> > > 
> > > Alignment can be done for slab allocations, kmem_cache_create() takes an
> > > align argument. I am not sure what the default alignment of objects is
> > > though (probably no default alignment). What is an optimal alignment in your
> > > view?
> > 
> > Probably SLAB_HWCACHE_ALIGN would make most sense.
> 
> This isn't memory accessing hardware, so I don't think it would, right?

I was more thinking of cacheline bouncing under contention. But maybe
that's not worth it in this case...

Re: [PATCH] binder: Use kmem_cache for binder_thread

[Joel Fernandes]

On Thu, Aug 29, 2019 at 01:49:53PM +0800, Peikan Tsai wrote:
> Hi,
> 
> The allocated size for each binder_thread is 512 bytes by kzalloc.
> Because the size of binder_thread is fixed and it's only 304 bytes.
> It will save 208 bytes per binder_thread when use create a kmem_cache
> for the binder_thread.

Awesome change and observation!!!

Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

(Another thought: how did you discover this? Are you using some tools to look
into slab fragmentation?).

thanks,

 - Joel

> Signed-off-by: Peikan Tsai <peikantsai@gmail.com>
> ---
>  drivers/android/binder.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index dc1c83eafc22..043e0ebd0fe7 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -87,6 +87,8 @@ static struct dentry *binder_debugfs_dir_entry_root;
>  static struct dentry *binder_debugfs_dir_entry_proc;
>  static atomic_t binder_last_id;
> 
> +static struct kmem_cache *binder_thread_cachep;
> +
>  static int proc_show(struct seq_file *m, void *unused);
>  DEFINE_SHOW_ATTRIBUTE(proc);
> 
> @@ -4696,14 +4698,15 @@ static struct binder_thread *binder_get_thread(struct binder_proc *proc)
>  	thread = binder_get_thread_ilocked(proc, NULL);
>  	binder_inner_proc_unlock(proc);
>  	if (!thread) {
> -		new_thread = kzalloc(sizeof(*thread), GFP_KERNEL);
> +		new_thread = kmem_cache_zalloc(binder_thread_cachep,
> +					       GFP_KERNEL);
>  		if (new_thread == NULL)
>  			return NULL;
>  		binder_inner_proc_lock(proc);
>  		thread = binder_get_thread_ilocked(proc, new_thread);
>  		binder_inner_proc_unlock(proc);
>  		if (thread != new_thread)
> -			kfree(new_thread);
> +			kmem_cache_free(binder_thread_cachep, new_thread);
>  	}
>  	return thread;
>  }
> @@ -4723,7 +4726,7 @@ static void binder_free_thread(struct binder_thread *thread)
>  	BUG_ON(!list_empty(&thread->todo));
>  	binder_stats_deleted(BINDER_STAT_THREAD);
>  	binder_proc_dec_tmpref(thread->proc);
> -	kfree(thread);
> +	kmem_cache_free(binder_thread_cachep, thread);
>  }
> 
>  static int binder_thread_release(struct binder_proc *proc,
> @@ -6095,6 +6098,12 @@ static int __init binder_init(void)
>  	if (ret)
>  		return ret;
> 
> +	binder_thread_cachep = kmem_cache_create("binder_thread",
> +						 sizeof(struct binder_thread),
> +						 0, 0, NULL);
> +	if (!binder_thread_cachep)
> +		return -ENOMEM;
> +
>  	atomic_set(&binder_transaction_log.cur, ~0U);
>  	atomic_set(&binder_transaction_log_failed.cur, ~0U);
> 
> @@ -6167,6 +6176,7 @@ static int __init binder_init(void)
> 
>  err_alloc_device_names_failed:
>  	debugfs_remove_recursive(binder_debugfs_dir_entry_root);
> +	kmem_cache_destroy(binder_thread_cachep);
> 
>  	return ret;
>  }
> --
> 2.17.1
>