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 <[email protected]>
Acked-by: Ananth N Mavinakayanahalli <[email protected]>
---
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: [email protected]
On Fri, 07 Nov 2008 18:44:30 -0500 Masami Hiramatsu <[email protected]> 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)
Hi Andrew,
Andrew Morton wrote:
> On Fri, 07 Nov 2008 18:44:30 -0500 Masami Hiramatsu <[email protected]> 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: [email protected]
On Fri, 07 Nov 2008 19:18:54 -0500 Masami Hiramatsu <[email protected]> wrote:
> Hi Andrew,
>
> Andrew Morton wrote:
> > On Fri, 07 Nov 2008 18:44:30 -0500 Masami Hiramatsu <[email protected]> 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?
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: [email protected]
On Fri, 07 Nov 2008 21:33:49 -0500 Masami Hiramatsu <[email protected]> wrote:
> Not really. Hmm, I have to investigate more on this problem.
OK ;)
Meanwhile, how does this look?
From: Andrew Morton <[email protected]>
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 <[email protected]>
Cc: Ananth N Mavinakayanahalli <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
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)
_
Andrew Morton wrote:
> On Fri, 07 Nov 2008 21:33:49 -0500 Masami Hiramatsu <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
> Cc: Ananth N Mavinakayanahalli <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
Acked-by: Masami Hiramatsu <[email protected]>
> ---
>
> 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: [email protected]
Masami Hiramatsu wrote:
> Andrew Morton wrote:
>> On Fri, 07 Nov 2008 21:33:49 -0500 Masami Hiramatsu <[email protected]> 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: [email protected]