kprobe: increase kprobe_hash_table size

PATCH In review Masami Hiramatsu 2 participants 8 messages Nov 7, 2008 – Nov 8, 2008 kprobes

Share & tools

Masami Hiramatsu

author

2008-11-07 23:45 UTC #0 lore raw

PATCH 1/1 kprobe: increase kprobe_hash_table size

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

kernel/kprobes.c +1 -1

Index: 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)

Signature · 7 lines

Masami Hiramatsu

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

e-mail: [email protected]

Andrew Morton

2008-11-07 23:58 UTC #1 lore raw

Quoted 3 lines · 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.

Quoted 9 lines

kernel/kprobes.c +1 -1

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

author

2008-11-08 00:19 UTC #2 lore raw

Hi Andrew,

Quoted 8 lines · Andrew Morton

Quoted 3 lines · 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.

Quoted 4 lines

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,

Quoted 12 lines

Quoted 10 lines

kernel/kprobes.c +1 -1

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

Signature · 7 lines

Masami Hiramatsu

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

e-mail: [email protected]

Andrew Morton

2008-11-08 01:04 UTC #3 lore raw

Quoted 12 lines · On Fri, 07 Nov 2008 19:18:54 -0500 Masami Hiramatsu <[email protected]> wrote:

Hi Andrew,
Quoted 8 lines · Andrew Morton
Quoted 3 lines · 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?

Quoted 8 lines

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?

Masami Hiramatsu

author

2008-11-08 02:34 UTC #4 lore raw

Quoted 4 lines · Andrew Morton

Quoted 3 lines
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.

Quoted 8 lines

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.

Quoted 7 lines

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

Quoted 3 lines

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.

Quoted 2 lines

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.

Signature · 7 lines

Masami Hiramatsu

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

e-mail: [email protected]

Andrew Morton

2008-11-08 02:47 UTC #5 lore raw

Quoted 1 line · 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(-)

kernel/kprobes.c renamed from kernel/kprobes.c~a +1 -1

diff -puN kernel/kprobes.c~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

author Acked-by

2008-11-08 02:52 UTC #6 lore raw

Quoted 7 lines · Andrew Morton

Quoted 1 line · 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!

Quoted 11 lines

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

Quoted 19 lines

---

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

diff -puN kernel/kprobes.c~a kernel/kprobes.c

kernel/kprobes.c renamed from kernel/kprobes.c~a +1 -1

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

Signature · 7 lines

Masami Hiramatsu

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

e-mail: [email protected]

Masami Hiramatsu

author

2008-11-08 02:54 UTC #7 lore raw

Quoted 9 lines · Masami Hiramatsu

Quoted 6 lines · Andrew Morton
Quoted 1 line · 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,

Signature · 7 lines

Masami Hiramatsu

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

e-mail: [email protected]