* [-mm PATCH] kprobes: fix build break in 2.6.15-rc5-mm3
@ 2005-12-20 9:54 Ananth N Mavinakayanahalli
2006-01-04 2:55 ` Andrew Morton
0 siblings, 1 reply; 3+ messages in thread
From: Ananth N Mavinakayanahalli @ 2005-12-20 9:54 UTC (permalink / raw)
To: linux-kernel; +Cc: akpm, prasanna, anil.s.keshavamurthy, davem
From: Ananth N Mavinkayanahalli <ananth@in.ibm.com>
The following patch (against 2.6.15-rc5-mm3) fixes a kprobes build
break due to changes introduced in the kprobe locking in
2.6.15-rc5-mm3. In addition, the patch reverts back the open-coding
of kprobe_mutex.
Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Acked-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
---
arch/powerpc/kernel/kprobes.c | 6 +++---
arch/x86_64/kernel/kprobes.c | 6 +++---
include/asm-i386/kprobes.h | 2 +-
include/asm-ia64/kprobes.h | 2 +-
include/asm-powerpc/kprobes.h | 3 ++-
include/asm-sparc64/kprobes.h | 2 +-
include/asm-x86_64/kprobes.h | 3 ++-
include/linux/kprobes.h | 1 +
kernel/kprobes.c | 4 ++--
9 files changed, 16 insertions(+), 13 deletions(-)
Index: linux-2.6.15-rc5/arch/powerpc/kernel/kprobes.c
===================================================================
--- linux-2.6.15-rc5.orig/arch/powerpc/kernel/kprobes.c
+++ linux-2.6.15-rc5/arch/powerpc/kernel/kprobes.c
@@ -80,11 +80,11 @@ void __kprobes arch_disarm_kprobe(struct
(unsigned long) p->addr + sizeof(kprobe_opcode_t));
}
-void __kprobes arch_remove_kprobe(struct kprobe *p, struct semaphore *s)
+void __kprobes arch_remove_kprobe(struct kprobe *p)
{
- down(s);
+ down(&kprobe_mutex);
free_insn_slot(p->ainsn.insn);
- up(s);
+ up(&kprobe_mutex);
}
static inline void prepare_singlestep(struct kprobe *p, struct pt_regs *regs)
Index: linux-2.6.15-rc5/arch/x86_64/kernel/kprobes.c
===================================================================
--- linux-2.6.15-rc5.orig/arch/x86_64/kernel/kprobes.c
+++ linux-2.6.15-rc5/arch/x86_64/kernel/kprobes.c
@@ -220,11 +220,11 @@ void __kprobes arch_disarm_kprobe(struct
(unsigned long) p->addr + sizeof(kprobe_opcode_t));
}
-void __kprobes arch_remove_kprobe(struct kprobe *p, struct semaphore *s)
+void __kprobes arch_remove_kprobe(struct kprobe *p)
{
- down(s);
+ down(&kprobe_mutex);
free_insn_slot(p->ainsn.insn);
- up(s);
+ up(&kprobe_mutex);
}
static inline void save_previous_kprobe(struct kprobe_ctlblk *kcb)
Index: linux-2.6.15-rc5/include/linux/kprobes.h
===================================================================
--- linux-2.6.15-rc5.orig/include/linux/kprobes.h
+++ linux-2.6.15-rc5/include/linux/kprobes.h
@@ -149,6 +149,7 @@ struct kretprobe_instance {
};
extern spinlock_t kretprobe_lock;
+extern struct semaphore kprobe_mutex;
extern int arch_prepare_kprobe(struct kprobe *p);
extern void arch_arm_kprobe(struct kprobe *p);
extern void arch_disarm_kprobe(struct kprobe *p);
Index: linux-2.6.15-rc5/kernel/kprobes.c
===================================================================
--- linux-2.6.15-rc5.orig/kernel/kprobes.c
+++ linux-2.6.15-rc5/kernel/kprobes.c
@@ -48,7 +48,7 @@
static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
-static DECLARE_MUTEX(kprobe_mutex); /* Protects kprobe_table */
+DECLARE_MUTEX(kprobe_mutex); /* Protects kprobe_table */
DEFINE_SPINLOCK(kretprobe_lock); /* Protects kretprobe_inst_table */
static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
@@ -532,7 +532,7 @@ valid_p:
list_del_rcu(&p->list);
kfree(old_p);
}
- arch_remove_kprobe(p, &kprobe_mutex);
+ arch_remove_kprobe(p);
}
}
Index: linux-2.6.15-rc5/include/asm-powerpc/kprobes.h
===================================================================
--- linux-2.6.15-rc5.orig/include/asm-powerpc/kprobes.h
+++ linux-2.6.15-rc5/include/asm-powerpc/kprobes.h
@@ -32,6 +32,7 @@
#define __ARCH_WANT_KPROBES_INSN_SLOT
struct pt_regs;
+struct kprobe;
typedef unsigned int kprobe_opcode_t;
#define BREAKPOINT_INSTRUCTION 0x7fe00008 /* trap */
@@ -49,7 +50,7 @@ typedef unsigned int kprobe_opcode_t;
#define ARCH_SUPPORTS_KRETPROBES
void kretprobe_trampoline(void);
-extern void arch_remove_kprobe(struct kprobe *p, struct semaphore *s);
+extern void arch_remove_kprobe(struct kprobe *p);
/* Architecture specific copy of original instruction */
struct arch_specific_insn {
Index: linux-2.6.15-rc5/include/asm-x86_64/kprobes.h
===================================================================
--- linux-2.6.15-rc5.orig/include/asm-x86_64/kprobes.h
+++ linux-2.6.15-rc5/include/asm-x86_64/kprobes.h
@@ -30,6 +30,7 @@
#define __ARCH_WANT_KPROBES_INSN_SLOT
struct pt_regs;
+struct kprobe;
typedef u8 kprobe_opcode_t;
#define BREAKPOINT_INSTRUCTION 0xcc
@@ -44,6 +45,7 @@ typedef u8 kprobe_opcode_t;
#define ARCH_SUPPORTS_KRETPROBES
void kretprobe_trampoline(void);
+extern void arch_remove_kprobe(struct kprobe *p);
/* Architecture specific copy of original instruction*/
struct arch_specific_insn {
@@ -78,7 +80,6 @@ static inline void restore_interrupts(st
local_irq_enable();
}
-extern void arch_remove_kprobe(struct kprobe *p, struct semaphore *s);
extern int post_kprobe_handler(struct pt_regs *regs);
extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr);
extern int kprobe_handler(struct pt_regs *regs);
Index: linux-2.6.15-rc5/include/asm-i386/kprobes.h
===================================================================
--- linux-2.6.15-rc5.orig/include/asm-i386/kprobes.h
+++ linux-2.6.15-rc5/include/asm-i386/kprobes.h
@@ -40,7 +40,7 @@ typedef u8 kprobe_opcode_t;
#define JPROBE_ENTRY(pentry) (kprobe_opcode_t *)pentry
#define ARCH_SUPPORTS_KRETPROBES
-#define arch_remove_kprobe(p, s) do { } while(0)
+#define arch_remove_kprobe(p) do {} while (0)
void kretprobe_trampoline(void);
Index: linux-2.6.15-rc5/include/asm-ia64/kprobes.h
===================================================================
--- linux-2.6.15-rc5.orig/include/asm-ia64/kprobes.h
+++ linux-2.6.15-rc5/include/asm-ia64/kprobes.h
@@ -89,7 +89,7 @@ struct kprobe_ctlblk {
#define IP_RELATIVE_PREDICT_OPCODE (7)
#define LONG_BRANCH_OPCODE (0xC)
#define LONG_CALL_OPCODE (0xD)
-#define arch_remove_kprobe(p, s) do { } while(0)
+#define arch_remove_kprobe(p) do {} while (0)
typedef struct kprobe_opcode {
bundle_t bundle;
Index: linux-2.6.15-rc5/include/asm-sparc64/kprobes.h
===================================================================
--- linux-2.6.15-rc5.orig/include/asm-sparc64/kprobes.h
+++ linux-2.6.15-rc5/include/asm-sparc64/kprobes.h
@@ -12,7 +12,7 @@ typedef u32 kprobe_opcode_t;
#define MAX_INSN_SIZE 2
#define JPROBE_ENTRY(pentry) (kprobe_opcode_t *)pentry
-#define arch_remove_kprobe(p, s) do { } while(0)
+#define arch_remove_kprobe(p) do {} while (0)
/* Architecture specific copy of original instruction*/
struct arch_specific_insn {
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [-mm PATCH] kprobes: fix build break in 2.6.15-rc5-mm3
2005-12-20 9:54 [-mm PATCH] kprobes: fix build break in 2.6.15-rc5-mm3 Ananth N Mavinakayanahalli
@ 2006-01-04 2:55 ` Andrew Morton
2006-01-04 4:51 ` Ananth N Mavinakayanahalli
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Morton @ 2006-01-04 2:55 UTC (permalink / raw)
To: ananth; +Cc: linux-kernel, prasanna, anil.s.keshavamurthy, davem
Ananth N Mavinakayanahalli <ananth@in.ibm.com> wrote:
>
> The following patch (against 2.6.15-rc5-mm3) fixes a kprobes build
> break due to changes introduced in the kprobe locking in
> 2.6.15-rc5-mm3. In addition, the patch reverts back the open-coding
> of kprobe_mutex.
Complaints:
a) Your changelog failed to describe what the build breakage was. It helps.
b) the changelog fails to describe _why_ we've reverted the locking
c) The patch does multiple things.
See, what I would _like_ to do is to fold the fixes in this patch into the
patches which are already in -mm. That way, the patches which hit Linus's
tree will be neater and won't introduce build breakage at any point.
And they won't add stuff and then immediately take it away again. That's
for git losers ;)
But the patch which you've sent doesn't have a hope of applying anywhere
except at the end of the patches which I already have.
The net result is that we'll hit Linus's tree with a bunch of patches, and
then a followup patch which fixes those patches. Which is a dumb way in
which to present the permanent kernel record, given that we have an
opportunity to get it right first time, no?
Here's the bottom line: please never ever ever ever ever ever do more than
one thing in a single patch. Ever. Did I mention "ever"? There are soooo
many reasons for this....
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [-mm PATCH] kprobes: fix build break in 2.6.15-rc5-mm3
2006-01-04 2:55 ` Andrew Morton
@ 2006-01-04 4:51 ` Ananth N Mavinakayanahalli
0 siblings, 0 replies; 3+ messages in thread
From: Ananth N Mavinakayanahalli @ 2006-01-04 4:51 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, prasanna, anil.s.keshavamurthy, davem
On Tue, Jan 03, 2006 at 06:55:08PM -0800, Andrew Morton wrote:
> Ananth N Mavinakayanahalli <ananth@in.ibm.com> wrote:
> >
> > The following patch (against 2.6.15-rc5-mm3) fixes a kprobes build
> > break due to changes introduced in the kprobe locking in
> > 2.6.15-rc5-mm3. In addition, the patch reverts back the open-coding
> > of kprobe_mutex.
>
Andrew,
> Complaints:
Sorry for the goofups..
> a) Your changelog failed to describe what the build breakage was. It helps.
The arch_remove_kprobe() prototype declaration in
include/asm-*/kprobes.h needs a forward declaration of "struct kprobe"
due to the order in which the kprobe.h files (include/linux/kprobe.h and
the arch specific ones) are included. Though other archs build fine,
the powerpc compiler throws out an error:
include/asm/kprobes.h:53: warning: its scope is only this definition or
declaration, which is probably not what you want
arch/powerpc/kernel/kprobes.c:84: error: conflicting types for
`arch_remove_kprobe'
include/asm/kprobes.h:53: error: previous declaration of
`arch_remove_kprobe'
make[1]: *** [arch/powerpc/kernel/kprobes.o] Error 1
make: *** [arch/powerpc/kernel] Error 2
> b) the changelog fails to describe _why_ we've reverted the locking
The patch that introduced kprobe_mutex instead of the spinlock had an
undesirable portion that passed a struct semaphore * as a parameter.
This obfuscates locking and will lead to hard to maintain code.
> c) The patch does multiple things.
>
> See, what I would _like_ to do is to fold the fixes in this patch into the
> patches which are already in -mm. That way, the patches which hit Linus's
> tree will be neater and won't introduce build breakage at any point.
>
> And they won't add stuff and then immediately take it away again. That's
> for git losers ;)
>
> But the patch which you've sent doesn't have a hope of applying anywhere
> except at the end of the patches which I already have.
My understanding was that the preferred method is incremental patches over
the released -mm.
> The net result is that we'll hit Linus's tree with a bunch of patches, and
> then a followup patch which fixes those patches. Which is a dumb way in
> which to present the permanent kernel record, given that we have an
> opportunity to get it right first time, no?
You are right. The patch in question fixes a build break and other issues
in the kprobe spinlock to mutex patch. And it is indeed better to redo the
patch with these fixes included. Does that sound reasonable?
> Here's the bottom line: please never ever ever ever ever ever do more than
> one thing in a single patch. Ever. Did I mention "ever"? There are soooo
> many reasons for this....
Point taken :)
Ananth
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2006-01-04 4:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-12-20 9:54 [-mm PATCH] kprobes: fix build break in 2.6.15-rc5-mm3 Ananth N Mavinakayanahalli
2006-01-04 2:55 ` Andrew Morton
2006-01-04 4:51 ` Ananth N Mavinakayanahalli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox