[PATCH] kprobe: increase kprobe_hash_table size

[PATCH] kprobe: increase kprobe_hash_table size

[Masami Hiramatsu]

Increase the size of kprobe hash table to 512. It's useful when hundreds
of kprobes were used in the kernel because current size is just 64.

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
Acked-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
---
 kernel/kprobes.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: 2.6.28-rc3/kernel/kprobes.c
===================================================================
--- 2.6.28-rc3.orig/kernel/kprobes.c
+++ 2.6.28-rc3/kernel/kprobes.c
@@ -49,7 +49,7 @@
 #include <asm/errno.h>
 #include <asm/uaccess.h>

-#define KPROBE_HASH_BITS 6
+#define KPROBE_HASH_BITS 9
 #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH] kprobe: increase kprobe_hash_table size

[Andrew Morton]

On Fri, 07 Nov 2008 18:44:30 -0500 Masami Hiramatsu <mhiramat@redhat.com> wrote:

> Increase the size of kprobe hash table to 512. It's useful when hundreds
> of kprobes were used in the kernel because current size is just 64.
> 

"useful" is a bit vague.  How big is the problem which this solves, and
how well did it solve it?

See, someone (me) needs to decide whether to merge this and if so,
whether to merge it into 2.6.29, 2.6.28, 2.6.27.x, 2.6.26.x and
2.6.25.x.  I'll need more information to make that decision, but I do
not have it.

> --- 2.6.28-rc3.orig/kernel/kprobes.c
> +++ 2.6.28-rc3/kernel/kprobes.c
> @@ -49,7 +49,7 @@
>  #include <asm/errno.h>
>  #include <asm/uaccess.h>
> 
> -#define KPROBE_HASH_BITS 6
> +#define KPROBE_HASH_BITS 9
>  #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)

Re: [PATCH] kprobe: increase kprobe_hash_table size

[Masami Hiramatsu]

Hi Andrew,

Andrew Morton wrote:
> On Fri, 07 Nov 2008 18:44:30 -0500 Masami Hiramatsu <mhiramat@redhat.com> wrote:
> 
>> Increase the size of kprobe hash table to 512. It's useful when hundreds
>> of kprobes were used in the kernel because current size is just 64.
>>
> 
> "useful" is a bit vague.  How big is the problem which this solves, and
> how well did it solve it?

For example, when probing enters and exits of syscall-related functions,
we need more than 500 probes. In that case, each hlist would have 8
elements in average. With this patch, the hlist would have 1 element in
average.

I agree that there may be many opinions about what is the best suited size.
Why I chose 512 was that I thought the table (byte) size was less than or
equal 4096 even on 64-bit arch.

> See, someone (me) needs to decide whether to merge this and if so,
> whether to merge it into 2.6.29, 2.6.28, 2.6.27.x, 2.6.26.x and
> 2.6.25.x.  I'll need more information to make that decision, but I do
> not have it.

I think this improves performance just a bit.
So I think it would not be needed for 2.6.27.x or older kernel.

Thank you,

> 
>> --- 2.6.28-rc3.orig/kernel/kprobes.c
>> +++ 2.6.28-rc3/kernel/kprobes.c
>> @@ -49,7 +49,7 @@
>>  #include <asm/errno.h>
>>  #include <asm/uaccess.h>
>>
>> -#define KPROBE_HASH_BITS 6
>> +#define KPROBE_HASH_BITS 9
>>  #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)
> 
> 

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH] kprobe: increase kprobe_hash_table size

[Andrew Morton]

On Fri, 07 Nov 2008 19:18:54 -0500 Masami Hiramatsu <mhiramat@redhat.com> wrote:

> Hi Andrew,
> 
> Andrew Morton wrote:
> > On Fri, 07 Nov 2008 18:44:30 -0500 Masami Hiramatsu <mhiramat@redhat.com> wrote:
> > 
> >> Increase the size of kprobe hash table to 512. It's useful when hundreds
> >> of kprobes were used in the kernel because current size is just 64.
> >>
> > 
> > "useful" is a bit vague.  How big is the problem which this solves, and
> > how well did it solve it?
> 
> For example, when probing enters and exits of syscall-related functions,
> we need more than 500 probes. In that case, each hlist would have 8
> elements in average. With this patch, the hlist would have 1 element in
> average.
> 
> I agree that there may be many opinions about what is the best suited size.
> Why I chose 512 was that I thought the table (byte) size was less than or
> equal 4096 even on 64-bit arch.

Well...

   text    data     bss     dec     hex filename
   7036     744    9380   17160    4308 kernel/kprobes.o
   7048     744   73892   81684   13f14 kernel/kprobes.o

That's 64 kbytes more memory.  It will be kretprobe_table_locks[] which
is hurting here, due to the ____cacheline_aligned.

I expected CONFIG_X86_VSMP=y to make this far worse, but fortunately
that only affects ____cacheline_internodealigned_in_smp.

btw, that array wastes a ton of memory on uniprocessor builds.  Using
____cacheline_aligned_in_smp should fix that.

Please always check these thigns with /usr/bin/size.

btw2, could/should kprobe_table[] and kretprobe_inst_table[] be
aggregated into kretprobe_table_locks[]?  That would save some memory
and might save some cache misses as well?


Anyway, enough pos-facto code review.  Is this change which you're
proposing worth increasing kernel memory usage by 64k?

Re: [PATCH] kprobe: increase kprobe_hash_table size

[Masami Hiramatsu]

Andrew Morton wrote:
>> I agree that there may be many opinions about what is the best suited size.
>> Why I chose 512 was that I thought the table (byte) size was less than or
>> equal 4096 even on 64-bit arch.
> 
> Well...
> 
>    text    data     bss     dec     hex filename
>    7036     744    9380   17160    4308 kernel/kprobes.o
>    7048     744   73892   81684   13f14 kernel/kprobes.o
> 
> That's 64 kbytes more memory.  It will be kretprobe_table_locks[] which
> is hurting here, due to the ____cacheline_aligned.

Oops! It's really bad.


> I expected CONFIG_X86_VSMP=y to make this far worse, but fortunately
> that only affects ____cacheline_internodealigned_in_smp.
> 
> btw, that array wastes a ton of memory on uniprocessor builds.  Using
> ____cacheline_aligned_in_smp should fix that.
> 
> Please always check these thigns with /usr/bin/size.

I see. I'll check that and try to find the best way...

> btw2, could/should kprobe_table[] and kretprobe_inst_table[] be
> aggregated into kretprobe_table_locks[]?  That would save some memory
> and might save some cache misses as well?

Indeed, thank you for good idea.

> Anyway, enough pos-facto code review.  Is this change which you're
> proposing worth increasing kernel memory usage by 64k?

Not really. Hmm, I have to investigate more on this problem.

Thanks a lot.

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH] kprobe: increase kprobe_hash_table size

[Andrew Morton]

On Fri, 07 Nov 2008 21:33:49 -0500 Masami Hiramatsu <mhiramat@redhat.com> wrote:

> Not really. Hmm, I have to investigate more on this problem.

OK ;)

Meanwhile, how does this look?



From: Andrew Morton <akpm@linux-foundation.org>

We only need the cacheline padding on SMP kernels.  Saves 6k:

   text    data     bss     dec     hex filename
   5713     388    2632    8733    221d kernel/kprobes.o
   5713     388    8840   14941    3a5d kernel/kprobes.o

Cc: Masami Hiramatsu <mhiramat@redhat.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/kprobes.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN kernel/kprobes.c~a kernel/kprobes.c
--- a/kernel/kprobes.c~a
+++ a/kernel/kprobes.c
@@ -72,7 +72,7 @@ static bool kprobe_enabled;
 DEFINE_MUTEX(kprobe_mutex);		/* Protects kprobe_table */
 static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
 static struct {
-	spinlock_t lock ____cacheline_aligned;
+	spinlock_t lock ____cacheline_aligned_in_smp;
 } kretprobe_table_locks[KPROBE_TABLE_SIZE];
 
 static spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
_

Re: [PATCH] kprobe: increase kprobe_hash_table size

[Masami Hiramatsu]

Andrew Morton wrote:
> On Fri, 07 Nov 2008 21:33:49 -0500 Masami Hiramatsu <mhiramat@redhat.com> wrote:
> 
>> Not really. Hmm, I have to investigate more on this problem.
> 
> OK ;)
> 
> Meanwhile, how does this look?

Great! That is enough acceptable.
Thank you very much!


> From: Andrew Morton <akpm@linux-foundation.org>
> 
> We only need the cacheline padding on SMP kernels.  Saves 6k:
> 
>    text    data     bss     dec     hex filename
>    5713     388    2632    8733    221d kernel/kprobes.o
>    5713     388    8840   14941    3a5d kernel/kprobes.o
> 
> Cc: Masami Hiramatsu <mhiramat@redhat.com>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Acked-by: Masami Hiramatsu <mhiramat@redhat.com>

> ---
> 
>  kernel/kprobes.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff -puN kernel/kprobes.c~a kernel/kprobes.c
> --- a/kernel/kprobes.c~a
> +++ a/kernel/kprobes.c
> @@ -72,7 +72,7 @@ static bool kprobe_enabled;
>  DEFINE_MUTEX(kprobe_mutex);		/* Protects kprobe_table */
>  static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
>  static struct {
> -	spinlock_t lock ____cacheline_aligned;
> +	spinlock_t lock ____cacheline_aligned_in_smp;
>  } kretprobe_table_locks[KPROBE_TABLE_SIZE];
>  
>  static spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
> _
> 

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [PATCH] kprobe: increase kprobe_hash_table size

[Masami Hiramatsu]

Masami Hiramatsu wrote:
> Andrew Morton wrote:
>> On Fri, 07 Nov 2008 21:33:49 -0500 Masami Hiramatsu <mhiramat@redhat.com> wrote:
>>
>>> Not really. Hmm, I have to investigate more on this problem.
>> OK ;)
>>
>> Meanwhile, how does this look?
> 
> Great! That is enough acceptable.

However, we still have to find reasonable way on SMP...

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com