[tip:perf/uprobes] uprobes/core: Clean up, refactor and improve the code

[tip:perf/uprobes] uprobes/core: Clean up, refactor and improve the code

[tip-bot for Ingo Molnar]

Commit-ID:  7b2d81d48a2d8e37efb6ce7b4d5ef58822b30d89
Gitweb:     http://git.kernel.org/tip/7b2d81d48a2d8e37efb6ce7b4d5ef58822b30d89
Author:     Ingo Molnar <mingo@elte.hu>
AuthorDate: Fri, 17 Feb 2012 09:27:41 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 17 Feb 2012 10:18:07 +0100

uprobes/core: Clean up, refactor and improve the code

Make the uprobes code readable to me:

 - improve the Kconfig text so that a mere mortal gets some idea
   what CONFIG_UPROBES=y is really about

 - do trivial renames to standardize around the uprobes_*() namespace

 - clean up and simplify various code flow details

 - separate basic blocks of functionality

 - line break artifact and white space related removal

 - use standard local varible definition blocks

 - use vertical spacing to make things more readable

 - remove unnecessary volatile

 - restructure comment blocks to make them more uniform and
   more readable in general

Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Cc: Jim Keniston <jkenisto@us.ibm.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Arnaldo Carvalho de Melo <acme@infradead.org>
Cc: Anton Arapov <anton@redhat.com>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Link: http://lkml.kernel.org/n/tip-ewbwhb8o6navvllsauu7k07p@git.kernel.org
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/Kconfig                   |   14 ++-
 arch/x86/include/asm/uprobes.h |   17 ++--
 arch/x86/kernel/uprobes.c      |  129 ++++++++++++-----------
 include/linux/uprobes.h        |   28 +++---
 kernel/uprobes.c               |  219 +++++++++++++++++++++++-----------------
 mm/mmap.c                      |   12 +-
 6 files changed, 233 insertions(+), 186 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 284f589..cca5b54 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -66,13 +66,19 @@ config OPTPROBES
 	depends on !PREEMPT
 
 config UPROBES
-	bool "User-space probes (EXPERIMENTAL)"
+	bool "Transparent user-space probes (EXPERIMENTAL)"
 	depends on ARCH_SUPPORTS_UPROBES
 	default n
 	help
-	  Uprobes enables kernel subsystems to establish probepoints
-	  in user applications and execute handler functions when
-	  the probepoints are hit.
+	  Uprobes is the user-space counterpart to kprobes: they
+	  enable instrumentation applications (such as 'perf probe')
+	  to establish unintrusive probes in user-space binaries and
+	  libraries, by executing handler functions when the probes
+	  are hit by user-space applications.
+
+	  ( These probes come in the form of single-byte breakpoints,
+	    managed by the kernel and kept transparent to the probed
+	    application. )
 
 	  If in doubt, say "N".
 
diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 8208234..072df39 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -1,7 +1,7 @@
 #ifndef _ASM_UPROBES_H
 #define _ASM_UPROBES_H
 /*
- * Userspace Probes (UProbes) for x86
+ * User-space Probes (UProbes) for x86
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -24,19 +24,20 @@
  */
 
 typedef u8 uprobe_opcode_t;
-#define MAX_UINSN_BYTES 16
-#define UPROBES_XOL_SLOT_BYTES	128	/* to keep it cache aligned */
 
-#define UPROBES_BKPT_INSN 0xcc
-#define UPROBES_BKPT_INSN_SIZE 1
+#define MAX_UINSN_BYTES			  16
+#define UPROBES_XOL_SLOT_BYTES		 128	/* to keep it cache aligned */
+
+#define UPROBES_BKPT_INSN		0xcc
+#define UPROBES_BKPT_INSN_SIZE		   1
 
 struct uprobe_arch_info {
-	u16			fixups;
+	u16				fixups;
 #ifdef CONFIG_X86_64
-	unsigned long rip_rela_target_address;
+	unsigned long			rip_rela_target_address;
 #endif
 };
 
 struct uprobe;
-extern int analyze_insn(struct mm_struct *mm, struct uprobe *uprobe);
+extern int arch_uprobes_analyze_insn(struct mm_struct *mm, struct uprobe *uprobe);
 #endif	/* _ASM_UPROBES_H */
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 2a301bb..cf2a184 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -1,5 +1,5 @@
 /*
- * Userspace Probes (UProbes) for x86
+ * User-space Probes (UProbes) for x86
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -20,7 +20,6 @@
  *	Srikar Dronamraju
  *	Jim Keniston
  */
-
 #include <linux/kernel.h>
 #include <linux/sched.h>
 #include <linux/ptrace.h>
@@ -42,10 +41,10 @@
 #define UPROBES_FIX_RIP_CX	0x4000
 
 /* Adaptations for mhiramat x86 decoder v14. */
-#define OPCODE1(insn) ((insn)->opcode.bytes[0])
-#define OPCODE2(insn) ((insn)->opcode.bytes[1])
-#define OPCODE3(insn) ((insn)->opcode.bytes[2])
-#define MODRM_REG(insn) X86_MODRM_REG(insn->modrm.value)
+#define OPCODE1(insn)		((insn)->opcode.bytes[0])
+#define OPCODE2(insn)		((insn)->opcode.bytes[1])
+#define OPCODE3(insn)		((insn)->opcode.bytes[2])
+#define MODRM_REG(insn)		X86_MODRM_REG(insn->modrm.value)
 
 #define W(row, b0, b1, b2, b3, b4, b5, b6, b7, b8, b9, ba, bb, bc, bd, be, bf)\
 	(((b0##UL << 0x0)|(b1##UL << 0x1)|(b2##UL << 0x2)|(b3##UL << 0x3) |   \
@@ -55,7 +54,7 @@
 	 << (row % 32))
 
 #ifdef CONFIG_X86_64
-static volatile u32 good_insns_64[256 / 32] = {
+static u32 good_insns_64[256 / 32] = {
 	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
 	/*      ----------------------------------------------         */
 	W(0x00, 1, 1, 1, 1, 1, 1, 0, 0, 1, 1, 1, 1, 1, 1, 0, 0) | /* 00 */
@@ -81,7 +80,7 @@ static volatile u32 good_insns_64[256 / 32] = {
 
 /* Good-instruction tables for 32-bit apps */
 
-static volatile u32 good_insns_32[256 / 32] = {
+static u32 good_insns_32[256 / 32] = {
 	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
 	/*      ----------------------------------------------         */
 	W(0x00, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 0) | /* 00 */
@@ -105,7 +104,7 @@ static volatile u32 good_insns_32[256 / 32] = {
 };
 
 /* Using this for both 64-bit and 32-bit apps */
-static volatile u32 good_2byte_insns[256 / 32] = {
+static u32 good_2byte_insns[256 / 32] = {
 	/*      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f         */
 	/*      ----------------------------------------------         */
 	W(0x00, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1) | /* 00 */
@@ -132,42 +131,47 @@ static volatile u32 good_2byte_insns[256 / 32] = {
 
 /*
  * opcodes we'll probably never support:
- * 6c-6d, e4-e5, ec-ed - in
- * 6e-6f, e6-e7, ee-ef - out
- * cc, cd - int3, int
- * cf - iret
- * d6 - illegal instruction
- * f1 - int1/icebp
- * f4 - hlt
- * fa, fb - cli, sti
- * 0f - lar, lsl, syscall, clts, sysret, sysenter, sysexit, invd, wbinvd, ud2
+ *
+ *  6c-6d, e4-e5, ec-ed - in
+ *  6e-6f, e6-e7, ee-ef - out
+ *  cc, cd - int3, int
+ *  cf - iret
+ *  d6 - illegal instruction
+ *  f1 - int1/icebp
+ *  f4 - hlt
+ *  fa, fb - cli, sti
+ *  0f - lar, lsl, syscall, clts, sysret, sysenter, sysexit, invd, wbinvd, ud2
  *
  * invalid opcodes in 64-bit mode:
- * 06, 0e, 16, 1e, 27, 2f, 37, 3f, 60-62, 82, c4-c5, d4-d5
  *
- * 63 - we support this opcode in x86_64 but not in i386.
+ *  06, 0e, 16, 1e, 27, 2f, 37, 3f, 60-62, 82, c4-c5, d4-d5
+ *  63 - we support this opcode in x86_64 but not in i386.
  *
  * opcodes we may need to refine support for:
- * 0f - 2-byte instructions: For many of these instructions, the validity
- * depends on the prefix and/or the reg field.  On such instructions, we
- * just consider the opcode combination valid if it corresponds to any
- * valid instruction.
- * 8f - Group 1 - only reg = 0 is OK
- * c6-c7 - Group 11 - only reg = 0 is OK
- * d9-df - fpu insns with some illegal encodings
- * f2, f3 - repnz, repz prefixes.  These are also the first byte for
- * certain floating-point instructions, such as addsd.
- * fe - Group 4 - only reg = 0 or 1 is OK
- * ff - Group 5 - only reg = 0-6 is OK
+ *
+ *  0f - 2-byte instructions: For many of these instructions, the validity
+ *  depends on the prefix and/or the reg field.  On such instructions, we
+ *  just consider the opcode combination valid if it corresponds to any
+ *  valid instruction.
+ *
+ *  8f - Group 1 - only reg = 0 is OK
+ *  c6-c7 - Group 11 - only reg = 0 is OK
+ *  d9-df - fpu insns with some illegal encodings
+ *  f2, f3 - repnz, repz prefixes.  These are also the first byte for
+ *  certain floating-point instructions, such as addsd.
+ *
+ *  fe - Group 4 - only reg = 0 or 1 is OK
+ *  ff - Group 5 - only reg = 0-6 is OK
  *
  * others -- Do we need to support these?
- * 0f - (floating-point?) prefetch instructions
- * 07, 17, 1f - pop es, pop ss, pop ds
- * 26, 2e, 36, 3e - es:, cs:, ss:, ds: segment prefixes --
+ *
+ *  0f - (floating-point?) prefetch instructions
+ *  07, 17, 1f - pop es, pop ss, pop ds
+ *  26, 2e, 36, 3e - es:, cs:, ss:, ds: segment prefixes --
  *	but 64 and 65 (fs: and gs:) seem to be used, so we support them
- * 67 - addr16 prefix
- * ce - into
- * f0 - lock prefix
+ *  67 - addr16 prefix
+ *  ce - into
+ *  f0 - lock prefix
  */
 
 /*
@@ -182,11 +186,11 @@ static bool is_prefix_bad(struct insn *insn)
 
 	for (i = 0; i < insn->prefixes.nbytes; i++) {
 		switch (insn->prefixes.bytes[i]) {
-		case 0x26:	/*INAT_PFX_ES   */
-		case 0x2E:	/*INAT_PFX_CS   */
-		case 0x36:	/*INAT_PFX_DS   */
-		case 0x3E:	/*INAT_PFX_SS   */
-		case 0xF0:	/*INAT_PFX_LOCK */
+		case 0x26:	/* INAT_PFX_ES   */
+		case 0x2E:	/* INAT_PFX_CS   */
+		case 0x36:	/* INAT_PFX_DS   */
+		case 0x3E:	/* INAT_PFX_SS   */
+		case 0xF0:	/* INAT_PFX_LOCK */
 			return true;
 		}
 	}
@@ -201,12 +205,15 @@ static int validate_insn_32bits(struct uprobe *uprobe, struct insn *insn)
 	insn_get_opcode(insn);
 	if (is_prefix_bad(insn))
 		return -ENOTSUPP;
+
 	if (test_bit(OPCODE1(insn), (unsigned long *)good_insns_32))
 		return 0;
+
 	if (insn->opcode.nbytes == 2) {
 		if (test_bit(OPCODE2(insn), (unsigned long *)good_2byte_insns))
 			return 0;
 	}
+
 	return -ENOTSUPP;
 }
 
@@ -282,12 +289,12 @@ static void prepare_fixups(struct uprobe *uprobe, struct insn *insn)
  * disastrous.
  *
  * Some useful facts about rip-relative instructions:
- * - There's always a modrm byte.
- * - There's never a SIB byte.
- * - The displacement is always 4 bytes.
+ *
+ *  - There's always a modrm byte.
+ *  - There's never a SIB byte.
+ *  - The displacement is always 4 bytes.
  */
-static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe,
-							struct insn *insn)
+static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
 {
 	u8 *cursor;
 	u8 reg;
@@ -342,13 +349,12 @@ static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe,
 	}
 
 	/* Target address = address of next instruction + (signed) offset */
-	uprobe->arch_info.rip_rela_target_address = (long)insn->length
-					+ insn->displacement.value;
+	uprobe->arch_info.rip_rela_target_address = (long)insn->length + insn->displacement.value;
+
 	/* Displacement field is gone; slide immediate field (if any) over. */
 	if (insn->immediate.nbytes) {
 		cursor++;
-		memmove(cursor, cursor + insn->displacement.nbytes,
-						insn->immediate.nbytes);
+		memmove(cursor, cursor + insn->displacement.nbytes, insn->immediate.nbytes);
 	}
 	return;
 }
@@ -361,8 +367,10 @@ static int validate_insn_64bits(struct uprobe *uprobe, struct insn *insn)
 	insn_get_opcode(insn);
 	if (is_prefix_bad(insn))
 		return -ENOTSUPP;
+
 	if (test_bit(OPCODE1(insn), (unsigned long *)good_insns_64))
 		return 0;
+
 	if (insn->opcode.nbytes == 2) {
 		if (test_bit(OPCODE2(insn), (unsigned long *)good_2byte_insns))
 			return 0;
@@ -370,34 +378,31 @@ static int validate_insn_64bits(struct uprobe *uprobe, struct insn *insn)
 	return -ENOTSUPP;
 }
 
-static int validate_insn_bits(struct mm_struct *mm, struct uprobe *uprobe,
-				struct insn *insn)
+static int validate_insn_bits(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
 {
 	if (mm->context.ia32_compat)
 		return validate_insn_32bits(uprobe, insn);
 	return validate_insn_64bits(uprobe, insn);
 }
-#else
-static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe,
-							struct insn *insn)
+#else /* 32-bit: */
+static void handle_riprel_insn(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
 {
-	return;
+	/* No RIP-relative addressing on 32-bit */
 }
 
-static int validate_insn_bits(struct mm_struct *mm, struct uprobe *uprobe,
-				struct insn *insn)
+static int validate_insn_bits(struct mm_struct *mm, struct uprobe *uprobe, struct insn *insn)
 {
 	return validate_insn_32bits(uprobe, insn);
 }
 #endif /* CONFIG_X86_64 */
 
 /**
- * analyze_insn - instruction analysis including validity and fixups.
+ * arch_uprobes_analyze_insn - instruction analysis including validity and fixups.
  * @mm: the probed address space.
  * @uprobe: the probepoint information.
  * Return 0 on success or a -ve number on error.
  */
-int analyze_insn(struct mm_struct *mm, struct uprobe *uprobe)
+int arch_uprobes_analyze_insn(struct mm_struct *mm, struct uprobe *uprobe)
 {
 	int ret;
 	struct insn insn;
@@ -406,7 +411,9 @@ int analyze_insn(struct mm_struct *mm, struct uprobe *uprobe)
 	ret = validate_insn_bits(mm, uprobe, &insn);
 	if (ret != 0)
 		return ret;
+
 	handle_riprel_insn(mm, uprobe, &insn);
 	prepare_fixups(uprobe, &insn);
+
 	return 0;
 }
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index f1d13fd..64e45f1 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -1,7 +1,7 @@
 #ifndef _LINUX_UPROBES_H
 #define _LINUX_UPROBES_H
 /*
- * Userspace Probes (UProbes)
+ * User-space Probes (UProbes)
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -40,8 +40,10 @@ struct uprobe_arch_info {};
 #define uprobe_opcode_sz sizeof(uprobe_opcode_t)
 
 /* flags that denote/change uprobes behaviour */
+
 /* Have a copy of original instruction */
 #define UPROBES_COPY_INSN	0x1
+
 /* Dont run handlers when first register/ last unregister in progress*/
 #define UPROBES_RUN_HANDLER	0x2
 
@@ -70,27 +72,23 @@ struct uprobe {
 };
 
 #ifdef CONFIG_UPROBES
-extern int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe,
-							unsigned long vaddr);
-extern int __weak set_orig_insn(struct mm_struct *mm, struct uprobe *uprobe,
-					unsigned long vaddr, bool verify);
+extern int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr);
+extern int __weak set_orig_insn(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr, bool verify);
 extern bool __weak is_bkpt_insn(uprobe_opcode_t *insn);
-extern int register_uprobe(struct inode *inode, loff_t offset,
-				struct uprobe_consumer *consumer);
-extern void unregister_uprobe(struct inode *inode, loff_t offset,
-				struct uprobe_consumer *consumer);
-extern int mmap_uprobe(struct vm_area_struct *vma);
+extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *consumer);
+extern void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *consumer);
+extern int uprobe_mmap(struct vm_area_struct *vma);
 #else /* CONFIG_UPROBES is not defined */
-static inline int register_uprobe(struct inode *inode, loff_t offset,
-				struct uprobe_consumer *consumer)
+static inline int
+uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *consumer)
 {
 	return -ENOSYS;
 }
-static inline void unregister_uprobe(struct inode *inode, loff_t offset,
-				struct uprobe_consumer *consumer)
+static inline void
+uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *consumer)
 {
 }
-static inline int mmap_uprobe(struct vm_area_struct *vma)
+static inline int uprobe_mmap(struct vm_area_struct *vma)
 {
 	return 0;
 }
diff --git a/kernel/uprobes.c b/kernel/uprobes.c
index 72e8bb3..884817f 100644
--- a/kernel/uprobes.c
+++ b/kernel/uprobes.c
@@ -1,5 +1,5 @@
 /*
- * Userspace Probes (UProbes)
+ * User-space Probes (UProbes)
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -29,24 +29,26 @@
 #include <linux/rmap.h>		/* anon_vma_prepare */
 #include <linux/mmu_notifier.h>	/* set_pte_at_notify */
 #include <linux/swap.h>		/* try_to_free_swap */
+
 #include <linux/uprobes.h>
 
 static struct rb_root uprobes_tree = RB_ROOT;
+
 static DEFINE_SPINLOCK(uprobes_treelock);	/* serialize rbtree access */
 
 #define UPROBES_HASH_SZ	13
+
 /* serialize (un)register */
 static struct mutex uprobes_mutex[UPROBES_HASH_SZ];
-#define uprobes_hash(v)	(&uprobes_mutex[((unsigned long)(v)) %\
-						UPROBES_HASH_SZ])
+
+#define uprobes_hash(v)		(&uprobes_mutex[((unsigned long)(v)) % UPROBES_HASH_SZ])
 
 /* serialize uprobe->pending_list */
 static struct mutex uprobes_mmap_mutex[UPROBES_HASH_SZ];
-#define uprobes_mmap_hash(v)	(&uprobes_mmap_mutex[((unsigned long)(v)) %\
-						UPROBES_HASH_SZ])
+#define uprobes_mmap_hash(v)	(&uprobes_mmap_mutex[((unsigned long)(v)) % UPROBES_HASH_SZ])
 
 /*
- * uprobe_events allows us to skip the mmap_uprobe if there are no uprobe
+ * uprobe_events allows us to skip the uprobe_mmap if there are no uprobe
  * events active at this time.  Probably a fine grained per inode count is
  * better?
  */
@@ -58,9 +60,9 @@ static atomic_t uprobe_events = ATOMIC_INIT(0);
  * vm_area_struct wasnt recommended.
  */
 struct vma_info {
-	struct list_head probe_list;
-	struct mm_struct *mm;
-	loff_t vaddr;
+	struct list_head	probe_list;
+	struct mm_struct	*mm;
+	loff_t			vaddr;
 };
 
 /*
@@ -79,8 +81,7 @@ static bool valid_vma(struct vm_area_struct *vma, bool is_register)
 	if (!is_register)
 		return true;
 
-	if ((vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)) ==
-						(VM_READ|VM_EXEC))
+	if ((vma->vm_flags & (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)) == (VM_READ|VM_EXEC))
 		return true;
 
 	return false;
@@ -92,6 +93,7 @@ static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
 
 	vaddr = vma->vm_start + offset;
 	vaddr -= vma->vm_pgoff << PAGE_SHIFT;
+
 	return vaddr;
 }
 
@@ -105,8 +107,7 @@ static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
  *
  * Returns 0 on success, -EFAULT on failure.
  */
-static int __replace_page(struct vm_area_struct *vma, struct page *page,
-					struct page *kpage)
+static int __replace_page(struct vm_area_struct *vma, struct page *page, struct page *kpage)
 {
 	struct mm_struct *mm = vma->vm_mm;
 	pgd_t *pgd;
@@ -163,7 +164,7 @@ out:
  */
 bool __weak is_bkpt_insn(uprobe_opcode_t *insn)
 {
-	return (*insn == UPROBES_BKPT_INSN);
+	return *insn == UPROBES_BKPT_INSN;
 }
 
 /*
@@ -203,6 +204,7 @@ static int write_opcode(struct mm_struct *mm, struct uprobe *uprobe,
 	ret = get_user_pages(NULL, mm, vaddr, 1, 0, 0, &old_page, &vma);
 	if (ret <= 0)
 		return ret;
+
 	ret = -EINVAL;
 
 	/*
@@ -239,6 +241,7 @@ static int write_opcode(struct mm_struct *mm, struct uprobe *uprobe,
 	vaddr_new = kmap_atomic(new_page);
 
 	memcpy(vaddr_new, vaddr_old, PAGE_SIZE);
+
 	/* poke the new insn in, ASSUMES we don't cross page boundary */
 	vaddr &= ~PAGE_MASK;
 	BUG_ON(vaddr + uprobe_opcode_sz > PAGE_SIZE);
@@ -260,7 +263,8 @@ unlock_out:
 	page_cache_release(new_page);
 
 put_out:
-	put_page(old_page);	/* we did a get_page in the beginning */
+	put_page(old_page);
+
 	return ret;
 }
 
@@ -276,8 +280,7 @@ put_out:
  * For mm @mm, read the opcode at @vaddr and store it in @opcode.
  * Return 0 (success) or a negative errno.
  */
-static int read_opcode(struct mm_struct *mm, unsigned long vaddr,
-						uprobe_opcode_t *opcode)
+static int read_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t *opcode)
 {
 	struct page *page;
 	void *vaddr_new;
@@ -293,15 +296,18 @@ static int read_opcode(struct mm_struct *mm, unsigned long vaddr,
 	memcpy(opcode, vaddr_new + vaddr, uprobe_opcode_sz);
 	kunmap_atomic(vaddr_new);
 	unlock_page(page);
-	put_page(page);		/* we did a get_user_pages in the beginning */
+
+	put_page(page);
+
 	return 0;
 }
 
 static int is_bkpt_at_addr(struct mm_struct *mm, unsigned long vaddr)
 {
 	uprobe_opcode_t opcode;
-	int result = read_opcode(mm, vaddr, &opcode);
+	int result;
 
+	result = read_opcode(mm, vaddr, &opcode);
 	if (result)
 		return result;
 
@@ -320,11 +326,11 @@ static int is_bkpt_at_addr(struct mm_struct *mm, unsigned long vaddr)
  * For mm @mm, store the breakpoint instruction at @vaddr.
  * Return 0 (success) or a negative errno.
  */
-int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe,
-						unsigned long vaddr)
+int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr)
 {
-	int result = is_bkpt_at_addr(mm, vaddr);
+	int result;
 
+	result = is_bkpt_at_addr(mm, vaddr);
 	if (result == 1)
 		return -EEXIST;
 
@@ -344,35 +350,35 @@ int __weak set_bkpt(struct mm_struct *mm, struct uprobe *uprobe,
  * For mm @mm, restore the original opcode (opcode) at @vaddr.
  * Return 0 (success) or a negative errno.
  */
-int __weak set_orig_insn(struct mm_struct *mm, struct uprobe *uprobe,
-					unsigned long vaddr, bool verify)
+int __weak
+set_orig_insn(struct mm_struct *mm, struct uprobe *uprobe, unsigned long vaddr, bool verify)
 {
 	if (verify) {
-		int result = is_bkpt_at_addr(mm, vaddr);
+		int result;
 
+		result = is_bkpt_at_addr(mm, vaddr);
 		if (!result)
 			return -EINVAL;
 
 		if (result != 1)
 			return result;
 	}
-	return write_opcode(mm, uprobe, vaddr,
-				*(uprobe_opcode_t *)uprobe->insn);
+	return write_opcode(mm, uprobe, vaddr, *(uprobe_opcode_t *)uprobe->insn);
 }
 
 static int match_uprobe(struct uprobe *l, struct uprobe *r)
 {
 	if (l->inode < r->inode)
 		return -1;
+
 	if (l->inode > r->inode)
 		return 1;
-	else {
-		if (l->offset < r->offset)
-			return -1;
 
-		if (l->offset > r->offset)
-			return 1;
-	}
+	if (l->offset < r->offset)
+		return -1;
+
+	if (l->offset > r->offset)
+		return 1;
 
 	return 0;
 }
@@ -391,6 +397,7 @@ static struct uprobe *__find_uprobe(struct inode *inode, loff_t offset)
 			atomic_inc(&uprobe->ref);
 			return uprobe;
 		}
+
 		if (match < 0)
 			n = n->rb_left;
 		else
@@ -411,6 +418,7 @@ static struct uprobe *find_uprobe(struct inode *inode, loff_t offset)
 	spin_lock_irqsave(&uprobes_treelock, flags);
 	uprobe = __find_uprobe(inode, offset);
 	spin_unlock_irqrestore(&uprobes_treelock, flags);
+
 	return uprobe;
 }
 
@@ -436,16 +444,18 @@ static struct uprobe *__insert_uprobe(struct uprobe *uprobe)
 			p = &parent->rb_right;
 
 	}
+
 	u = NULL;
 	rb_link_node(&uprobe->rb_node, parent, p);
 	rb_insert_color(&uprobe->rb_node, &uprobes_tree);
 	/* get access + creation ref */
 	atomic_set(&uprobe->ref, 2);
+
 	return u;
 }
 
 /*
- * Acquires uprobes_treelock.
+ * Acquire uprobes_treelock.
  * Matching uprobe already exists in rbtree;
  *	increment (access refcount) and return the matching uprobe.
  *
@@ -460,6 +470,7 @@ static struct uprobe *insert_uprobe(struct uprobe *uprobe)
 	spin_lock_irqsave(&uprobes_treelock, flags);
 	u = __insert_uprobe(uprobe);
 	spin_unlock_irqrestore(&uprobes_treelock, flags);
+
 	return u;
 }
 
@@ -490,19 +501,22 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset)
 		kfree(uprobe);
 		uprobe = cur_uprobe;
 		iput(inode);
-	} else
+	} else {
 		atomic_inc(&uprobe_events);
+	}
+
 	return uprobe;
 }
 
 /* Returns the previous consumer */
-static struct uprobe_consumer *add_consumer(struct uprobe *uprobe,
-				struct uprobe_consumer *consumer)
+static struct uprobe_consumer *
+consumer_add(struct uprobe *uprobe, struct uprobe_consumer *consumer)
 {
 	down_write(&uprobe->consumer_rwsem);
 	consumer->next = uprobe->consumers;
 	uprobe->consumers = consumer;
 	up_write(&uprobe->consumer_rwsem);
+
 	return consumer->next;
 }
 
@@ -511,8 +525,7 @@ static struct uprobe_consumer *add_consumer(struct uprobe *uprobe,
  * Return true if the @consumer is deleted successfully
  * or return false.
  */
-static bool del_consumer(struct uprobe *uprobe,
-				struct uprobe_consumer *consumer)
+static bool consumer_del(struct uprobe *uprobe, struct uprobe_consumer *consumer)
 {
 	struct uprobe_consumer **con;
 	bool ret = false;
@@ -526,6 +539,7 @@ static bool del_consumer(struct uprobe *uprobe,
 		}
 	}
 	up_write(&uprobe->consumer_rwsem);
+
 	return ret;
 }
 
@@ -557,15 +571,15 @@ static int __copy_insn(struct address_space *mapping,
 	memcpy(insn, vaddr + off1, nbytes);
 	kunmap_atomic(vaddr);
 	page_cache_release(page);
+
 	return 0;
 }
 
-static int copy_insn(struct uprobe *uprobe, struct vm_area_struct *vma,
-					unsigned long addr)
+static int copy_insn(struct uprobe *uprobe, struct vm_area_struct *vma, unsigned long addr)
 {
 	struct address_space *mapping;
-	int bytes;
 	unsigned long nbytes;
+	int bytes;
 
 	addr &= ~PAGE_MASK;
 	nbytes = PAGE_SIZE - addr;
@@ -605,6 +619,7 @@ static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe,
 		return -EEXIST;
 
 	addr = (unsigned long)vaddr;
+
 	if (!(uprobe->flags & UPROBES_COPY_INSN)) {
 		ret = copy_insn(uprobe, vma, addr);
 		if (ret)
@@ -613,7 +628,7 @@ static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe,
 		if (is_bkpt_insn((uprobe_opcode_t *)uprobe->insn))
 			return -EEXIST;
 
-		ret = analyze_insn(mm, uprobe);
+		ret = arch_uprobes_analyze_insn(mm, uprobe);
 		if (ret)
 			return ret;
 
@@ -624,8 +639,7 @@ static int install_breakpoint(struct mm_struct *mm, struct uprobe *uprobe,
 	return ret;
 }
 
-static void remove_breakpoint(struct mm_struct *mm, struct uprobe *uprobe,
-							loff_t vaddr)
+static void remove_breakpoint(struct mm_struct *mm, struct uprobe *uprobe, loff_t vaddr)
 {
 	set_orig_insn(mm, uprobe, (unsigned long)vaddr, true);
 }
@@ -649,9 +663,11 @@ static struct vma_info *__find_next_vma_info(struct list_head *head,
 	struct prio_tree_iter iter;
 	struct vm_area_struct *vma;
 	struct vma_info *tmpvi;
-	loff_t vaddr;
-	unsigned long pgoff = offset >> PAGE_SHIFT;
+	unsigned long pgoff;
 	int existing_vma;
+	loff_t vaddr;
+
+	pgoff = offset >> PAGE_SHIFT;
 
 	vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {
 		if (!valid_vma(vma, is_register))
@@ -659,6 +675,7 @@ static struct vma_info *__find_next_vma_info(struct list_head *head,
 
 		existing_vma = 0;
 		vaddr = vma_address(vma, offset);
+
 		list_for_each_entry(tmpvi, head, probe_list) {
 			if (tmpvi->mm == vma->vm_mm && tmpvi->vaddr == vaddr) {
 				existing_vma = 1;
@@ -670,14 +687,15 @@ static struct vma_info *__find_next_vma_info(struct list_head *head,
 		 * Another vma needs a probe to be installed. However skip
 		 * installing the probe if the vma is about to be unlinked.
 		 */
-		if (!existing_vma &&
-				atomic_inc_not_zero(&vma->vm_mm->mm_users)) {
+		if (!existing_vma && atomic_inc_not_zero(&vma->vm_mm->mm_users)) {
 			vi->mm = vma->vm_mm;
 			vi->vaddr = vaddr;
 			list_add(&vi->probe_list, head);
+
 			return vi;
 		}
 	}
+
 	return NULL;
 }
 
@@ -685,11 +703,12 @@ static struct vma_info *__find_next_vma_info(struct list_head *head,
  * Iterate in the rmap prio tree  and find a vma where a probe has not
  * yet been inserted.
  */
-static struct vma_info *find_next_vma_info(struct list_head *head,
-			loff_t offset, struct address_space *mapping,
-			bool is_register)
+static struct vma_info *
+find_next_vma_info(struct list_head *head, loff_t offset, struct address_space *mapping,
+		   bool is_register)
 {
 	struct vma_info *vi, *retvi;
+
 	vi = kzalloc(sizeof(struct vma_info), GFP_KERNEL);
 	if (!vi)
 		return ERR_PTR(-ENOMEM);
@@ -700,6 +719,7 @@ static struct vma_info *find_next_vma_info(struct list_head *head,
 
 	if (!retvi)
 		kfree(vi);
+
 	return retvi;
 }
 
@@ -711,16 +731,23 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 	struct vma_info *vi, *tmpvi;
 	struct mm_struct *mm;
 	loff_t vaddr;
-	int ret = 0;
+	int ret;
 
 	mapping = uprobe->inode->i_mapping;
 	INIT_LIST_HEAD(&try_list);
-	while ((vi = find_next_vma_info(&try_list, uprobe->offset,
-					mapping, is_register)) != NULL) {
+
+	ret = 0;
+
+	for (;;) {
+		vi = find_next_vma_info(&try_list, uprobe->offset, mapping, is_register);
+		if (!vi)
+			break;
+
 		if (IS_ERR(vi)) {
 			ret = PTR_ERR(vi);
 			break;
 		}
+
 		mm = vi->mm;
 		down_read(&mm->mmap_sem);
 		vma = find_vma(mm, (unsigned long)vi->vaddr);
@@ -755,19 +782,21 @@ static int register_for_each_vma(struct uprobe *uprobe, bool is_register)
 				break;
 		}
 	}
+
 	list_for_each_entry_safe(vi, tmpvi, &try_list, probe_list) {
 		list_del(&vi->probe_list);
 		kfree(vi);
 	}
+
 	return ret;
 }
 
-static int __register_uprobe(struct uprobe *uprobe)
+static int __uprobe_register(struct uprobe *uprobe)
 {
 	return register_for_each_vma(uprobe, true);
 }
 
-static void __unregister_uprobe(struct uprobe *uprobe)
+static void __uprobe_unregister(struct uprobe *uprobe)
 {
 	if (!register_for_each_vma(uprobe, false))
 		delete_uprobe(uprobe);
@@ -776,15 +805,15 @@ static void __unregister_uprobe(struct uprobe *uprobe)
 }
 
 /*
- * register_uprobe - register a probe
+ * uprobe_register - register a probe
  * @inode: the file in which the probe has to be placed.
  * @offset: offset from the start of the file.
  * @consumer: information on howto handle the probe..
  *
- * Apart from the access refcount, register_uprobe() takes a creation
+ * Apart from the access refcount, uprobe_register() takes a creation
  * refcount (thro alloc_uprobe) if and only if this @uprobe is getting
  * inserted into the rbtree (i.e first consumer for a @inode:@offset
- * tuple).  Creation refcount stops unregister_uprobe from freeing the
+ * tuple).  Creation refcount stops uprobe_unregister from freeing the
  * @uprobe even before the register operation is complete. Creation
  * refcount is released when the last @consumer for the @uprobe
  * unregisters.
@@ -792,28 +821,29 @@ static void __unregister_uprobe(struct uprobe *uprobe)
  * Return errno if it cannot successully install probes
  * else return 0 (success)
  */
-int register_uprobe(struct inode *inode, loff_t offset,
-				struct uprobe_consumer *consumer)
+int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *consumer)
 {
 	struct uprobe *uprobe;
-	int ret = -EINVAL;
+	int ret;
 
 	if (!inode || !consumer || consumer->next)
-		return ret;
+		return -EINVAL;
 
 	if (offset > i_size_read(inode))
-		return ret;
+		return -EINVAL;
 
 	ret = 0;
 	mutex_lock(uprobes_hash(inode));
 	uprobe = alloc_uprobe(inode, offset);
-	if (uprobe && !add_consumer(uprobe, consumer)) {
-		ret = __register_uprobe(uprobe);
+
+	if (uprobe && !consumer_add(uprobe, consumer)) {
+		ret = __uprobe_register(uprobe);
 		if (ret) {
 			uprobe->consumers = NULL;
-			__unregister_uprobe(uprobe);
-		} else
+			__uprobe_unregister(uprobe);
+		} else {
 			uprobe->flags |= UPROBES_RUN_HANDLER;
+		}
 	}
 
 	mutex_unlock(uprobes_hash(inode));
@@ -823,15 +853,14 @@ int register_uprobe(struct inode *inode, loff_t offset,
 }
 
 /*
- * unregister_uprobe - unregister a already registered probe.
+ * uprobe_unregister - unregister a already registered probe.
  * @inode: the file in which the probe has to be removed.
  * @offset: offset from the start of the file.
  * @consumer: identify which probe if multiple probes are colocated.
  */
-void unregister_uprobe(struct inode *inode, loff_t offset,
-				struct uprobe_consumer *consumer)
+void uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer *consumer)
 {
-	struct uprobe *uprobe = NULL;
+	struct uprobe *uprobe;
 
 	if (!inode || !consumer)
 		return;
@@ -841,15 +870,14 @@ void unregister_uprobe(struct inode *inode, loff_t offset,
 		return;
 
 	mutex_lock(uprobes_hash(inode));
-	if (!del_consumer(uprobe, consumer))
-		goto unreg_out;
 
-	if (!uprobe->consumers) {
-		__unregister_uprobe(uprobe);
-		uprobe->flags &= ~UPROBES_RUN_HANDLER;
+	if (consumer_del(uprobe, consumer)) {
+		if (!uprobe->consumers) {
+			__uprobe_unregister(uprobe);
+			uprobe->flags &= ~UPROBES_RUN_HANDLER;
+		}
 	}
 
-unreg_out:
 	mutex_unlock(uprobes_hash(inode));
 	if (uprobe)
 		put_uprobe(uprobe);
@@ -870,6 +898,7 @@ static struct rb_node *find_least_offset_node(struct inode *inode)
 	while (n) {
 		uprobe = rb_entry(n, struct uprobe, rb_node);
 		match = match_uprobe(&u, uprobe);
+
 		if (uprobe->inode == inode)
 			close_node = n;
 
@@ -881,6 +910,7 @@ static struct rb_node *find_least_offset_node(struct inode *inode)
 		else
 			n = n->rb_right;
 	}
+
 	return close_node;
 }
 
@@ -890,11 +920,13 @@ static struct rb_node *find_least_offset_node(struct inode *inode)
 static void build_probe_list(struct inode *inode, struct list_head *head)
 {
 	struct uprobe *uprobe;
-	struct rb_node *n;
 	unsigned long flags;
+	struct rb_node *n;
 
 	spin_lock_irqsave(&uprobes_treelock, flags);
+
 	n = find_least_offset_node(inode);
+
 	for (; n; n = rb_next(n)) {
 		uprobe = rb_entry(n, struct uprobe, rb_node);
 		if (uprobe->inode != inode)
@@ -903,6 +935,7 @@ static void build_probe_list(struct inode *inode, struct list_head *head)
 		list_add(&uprobe->pending_list, head);
 		atomic_inc(&uprobe->ref);
 	}
+
 	spin_unlock_irqrestore(&uprobes_treelock, flags);
 }
 
@@ -912,42 +945,44 @@ static void build_probe_list(struct inode *inode, struct list_head *head)
  *
  * Return -ve no if we fail to insert probes and we cannot
  * bail-out.
- * Return 0 otherwise. i.e :
+ * Return 0 otherwise. i.e:
+ *
  *	- successful insertion of probes
  *	- (or) no possible probes to be inserted.
  *	- (or) insertion of probes failed but we can bail-out.
  */
-int mmap_uprobe(struct vm_area_struct *vma)
+int uprobe_mmap(struct vm_area_struct *vma)
 {
 	struct list_head tmp_list;
 	struct uprobe *uprobe, *u;
 	struct inode *inode;
-	int ret = 0;
+	int ret;
 
 	if (!atomic_read(&uprobe_events) || !valid_vma(vma, true))
-		return ret;	/* Bail-out */
+		return 0;
 
 	inode = vma->vm_file->f_mapping->host;
 	if (!inode)
-		return ret;
+		return 0;
 
 	INIT_LIST_HEAD(&tmp_list);
 	mutex_lock(uprobes_mmap_hash(inode));
 	build_probe_list(inode, &tmp_list);
+
+	ret = 0;
+
 	list_for_each_entry_safe(uprobe, u, &tmp_list, pending_list) {
 		loff_t vaddr;
 
 		list_del(&uprobe->pending_list);
 		if (!ret) {
 			vaddr = vma_address(vma, uprobe->offset);
-			if (vaddr < vma->vm_start || vaddr >= vma->vm_end) {
-				put_uprobe(uprobe);
-				continue;
+			if (vaddr >= vma->vm_start && vaddr < vma->vm_end) {
+				ret = install_breakpoint(vma->vm_mm, uprobe, vma, vaddr);
+				/* Ignore double add: */
+				if (ret == -EEXIST)
+					ret = 0;
 			}
-			ret = install_breakpoint(vma->vm_mm, uprobe, vma,
-								vaddr);
-			if (ret == -EEXIST)
-				ret = 0;
 		}
 		put_uprobe(uprobe);
 	}
diff --git a/mm/mmap.c b/mm/mmap.c
index 1aed183..5a863d3 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -618,10 +618,10 @@ again:			remove_next = 1 + (end > next->vm_end);
 		mutex_unlock(&mapping->i_mmap_mutex);
 
 	if (root) {
-		mmap_uprobe(vma);
+		uprobe_mmap(vma);
 
 		if (adjust_next)
-			mmap_uprobe(next);
+			uprobe_mmap(next);
 	}
 
 	if (remove_next) {
@@ -646,7 +646,7 @@ again:			remove_next = 1 + (end > next->vm_end);
 		}
 	}
 	if (insert && file)
-		mmap_uprobe(insert);
+		uprobe_mmap(insert);
 
 	validate_mm(mm);
 
@@ -1340,7 +1340,7 @@ out:
 	} else if ((flags & MAP_POPULATE) && !(flags & MAP_NONBLOCK))
 		make_pages_present(addr, addr + len);
 
-	if (file && mmap_uprobe(vma))
+	if (file && uprobe_mmap(vma))
 		/* matching probes but cannot insert */
 		goto unmap_and_free_vma;
 
@@ -2301,7 +2301,7 @@ int insert_vm_struct(struct mm_struct * mm, struct vm_area_struct * vma)
 	     security_vm_enough_memory_mm(mm, vma_pages(vma)))
 		return -ENOMEM;
 
-	if (vma->vm_file && mmap_uprobe(vma))
+	if (vma->vm_file && uprobe_mmap(vma))
 		return -EINVAL;
 
 	vma_link(mm, vma, prev, rb_link, rb_parent);
@@ -2374,7 +2374,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
 			if (new_vma->vm_file) {
 				get_file(new_vma->vm_file);
 
-				if (mmap_uprobe(new_vma))
+				if (uprobe_mmap(new_vma))
 					goto out_free_mempol;
 
 				if (vma->vm_flags & VM_EXECUTABLE)

Re: [tip:perf/uprobes] uprobes/core: Clean up, refactor and improve the code

[Ingo Molnar]

Srikar,

* tip-bot for Ingo Molnar <mingo@elte.hu> wrote:

> Commit-ID:  7b2d81d48a2d8e37efb6ce7b4d5ef58822b30d89
> Gitweb:     http://git.kernel.org/tip/7b2d81d48a2d8e37efb6ce7b4d5ef58822b30d89
> Author:     Ingo Molnar <mingo@elte.hu>
> AuthorDate: Fri, 17 Feb 2012 09:27:41 +0100
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Fri, 17 Feb 2012 10:18:07 +0100
> 
> uprobes/core: Clean up, refactor and improve the code
> 
> Make the uprobes code readable to me:
> 
>  - improve the Kconfig text so that a mere mortal gets some idea
>    what CONFIG_UPROBES=y is really about
> 
>  - do trivial renames to standardize around the uprobes_*() namespace
> 
>  - clean up and simplify various code flow details
> 
>  - separate basic blocks of functionality
> 
>  - line break artifact and white space related removal
> 
>  - use standard local varible definition blocks
> 
>  - use vertical spacing to make things more readable
> 
>  - remove unnecessary volatile
> 
>  - restructure comment blocks to make them more uniform and
>    more readable in general
> 
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Jim Keniston <jkenisto@us.ibm.com>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Arnaldo Carvalho de Melo <acme@infradead.org>
> Cc: Anton Arapov <anton@redhat.com>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Link: http://lkml.kernel.org/n/tip-ewbwhb8o6navvllsauu7k07p@git.kernel.org
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/Kconfig                   |   14 ++-
>  arch/x86/include/asm/uprobes.h |   17 ++--
>  arch/x86/kernel/uprobes.c      |  129 ++++++++++++-----------
>  include/linux/uprobes.h        |   28 +++---
>  kernel/uprobes.c               |  219 +++++++++++++++++++++++-----------------
>  mm/mmap.c                      |   12 +-
>  6 files changed, 233 insertions(+), 186 deletions(-)

FYI, I created a new -tip topic branch for uprobes commits 
[tip:perf/uprobes], with two patches applied so far:

 - your v10 #1/9 patch
 - the above clean-ups and restructurings above it

( Please have a good look at the code flow clean-ups I have 
  performed, I have only build-tested the result. )

As you can see it from the diffstat, there were many small 
details that hindered code cleanliness, and there's at least one 
bigger item that still needs to be addressed, which I have not 
done in the above patch:

The arch methods and structures need to be better separated from 
core uprobes code. Right now there's uprobes.h which exports 
'struct uprobe' to the rest of the kernel, such as 
arch/x86/kernel/uprobes.c.

What we want instead is a clean separation of core code from 
architecture code:

 - 'struct arch_uprobe' which includes all fields that 
   'struct uprobe_arch_info' contains today. In the first 
   approximation this is a rename.

 - please rename uprobe->arch_info to uprobe->arch - it's 
   already clear that it's information.

 - uprobe->insn[] needs to move from struct uprobe to
   uprobe->arch.insn

 - The uprobes_arch_*() method(s) should be passed a
   'struct arch_uprobe *', not a 'struct uprobe *'.

 - Once this is done, 'struct uprobe' can move to the head of 
   kernel/uprobes.c, without any ugly #ifdefs and wrappery - 
   that code only compiles if uprobes are enabled and if the 
   architecture supports it.

 - asm/uprobes.h defines 'struct arch_uprobe' and the arch 
   method(s) - nothing else.

 - unless uprobe_opcode_t is non-byte on any architecture this 
   typedef should probably be go away and be char * or void *.

 - write_opcode() and any similar functions should be renamed to 
   the arch_uprobes_write_opcode() pattern

These changes should be done on top of latest -tip, in a single 
patch or a small series, it's mostly pretty straightforward.

Please keep an eye on stylistic details for future patches - the 
above patch gives a laundry list of what details were wrong in 
the existing code.

Once this is done can we add #2, #3, etc. patches in a clean 
manner.

I'd suggest you don't send a full series but send small, edible 
steps on top of the minimal (and right now not yet functional) 
uprobes core code I've already applied, in series of 1-3 
patches.

I'm sure there will be other small details we'll have to change 
in these patches as we go forward, so not porting the full 
series straight away would save you time and effort that the 
inevitable conflicts create.

As an additional benefit, if you do it in such a workflow then I 
can apply your new patches to tip:perf/uprobes pretty much the 
moment you send them out, reviewing and testing them quickly - 
without the time-consuming review of a long and interdependent 
patch series and the resulting churn on your side.

I believe we could iterate this through in a couple of days and 
arrive to a working 'perf probe' prototype.

Thanks,

	Ingo

Re: [tip:perf/uprobes] uprobes/core: Clean up, refactor and improve the code

[Srikar Dronamraju]

> 
>  - remove unnecessary volatile

volatiles were added because of warnings thrown by gcc-4.6. Please see
below.

> 
>  - restructure comment blocks to make them more uniform and
>    more readable in general
> 

...

> diff --git a/arch/Kconfig b/arch/Kconfig
> index 284f589..cca5b54 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -66,13 +66,19 @@ config OPTPROBES
>  	depends on !PREEMPT
> 
>  config UPROBES
> -	bool "User-space probes (EXPERIMENTAL)"
> +	bool "Transparent user-space probes (EXPERIMENTAL)"
>  	depends on ARCH_SUPPORTS_UPROBES
>  	default n
>  	help
> -	  Uprobes enables kernel subsystems to establish probepoints
> -	  in user applications and execute handler functions when
> -	  the probepoints are hit.
> +	  Uprobes is the user-space counterpart to kprobes: they
> +	  enable instrumentation applications (such as 'perf probe')
> +	  to establish unintrusive probes in user-space binaries and
> +	  libraries, by executing handler functions when the probes
> +	  are hit by user-space applications.
> +
> +	  ( These probes come in the form of single-byte breakpoints,

One nit: In some architectures like powerpc, the breakpoints arent
single-byte.

> +	    managed by the kernel and kept transparent to the probed
> +	    application. )
> 
>  	  If in doubt, say "N".
> 
> 
>  #ifdef CONFIG_X86_64
> -static volatile u32 good_insns_64[256 / 32] = {
> +static u32 good_insns_64[256 / 32] = {

The volatiles were added to arch/x86/kernel/kprobes.c because of commit
7115e3fcf45 and 315eb8a2a1b. The volatiles are required because gcc 4.6
gave a warning about the asm operand for test_bit.  So the same were
added to arch/x86/kernel/uprobes.c.

-- 
Thanks and Regards
Srikar

Re: [tip:perf/uprobes] uprobes/core: Clean up, refactor and improve the code

[Ingo Molnar]

* Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

> The volatiles were added to arch/x86/kernel/kprobes.c because 
> of commit 7115e3fcf45 and 315eb8a2a1b. The volatiles are 
> required because gcc 4.6 gave a warning about the asm operand 
> for test_bit.  So the same were added to 
> arch/x86/kernel/uprobes.c.

Seems like a GCC bug - a bogus warning - or does it generate bad 
code as well?

In any case, kprobes.c did it correctly, it added the volatile 
*and a comment*, pointing out that it's a GCC bug. No such 
warning was added to uprobes.c, making the volatile look 
entirely spurious.

So feel free to re-add the volatile in a followup patch, just 
make sure the GCC workaround nature is documented.

Thanks,

	Ingo

Re: [tip:perf/uprobes] uprobes/core: Clean up, refactor and improve the code

[Srikar Dronamraju]

> 
> FYI, I created a new -tip topic branch for uprobes commits 
> [tip:perf/uprobes], with two patches applied so far:
> 
>  - your v10 #1/9 patch
>  - the above clean-ups and restructurings above it
> 
> ( Please have a good look at the code flow clean-ups I have 
>   performed, I have only build-tested the result. )

Okay, Will do accordingly.

> 
> As you can see it from the diffstat, there were many small 
> details that hindered code cleanliness, and there's at least one 
> bigger item that still needs to be addressed, which I have not 
> done in the above patch:
> 
> The arch methods and structures need to be better separated from 
> core uprobes code. Right now there's uprobes.h which exports 
> 'struct uprobe' to the rest of the kernel, such as 
> arch/x86/kernel/uprobes.c.
> 
> What we want instead is a clean separation of core code from 
> architecture code:
> 
>  - 'struct arch_uprobe' which includes all fields that 
>    'struct uprobe_arch_info' contains today. In the first 
>    approximation this is a rename.
> 
>  - please rename uprobe->arch_info to uprobe->arch - it's 
>    already clear that it's information.
> 

Okay.

>  - uprobe->insn[] needs to move from struct uprobe to
>    uprobe->arch.insn
> 
>  - The uprobes_arch_*() method(s) should be passed a
>    'struct arch_uprobe *', not a 'struct uprobe *'.
> 
>  - Once this is done, 'struct uprobe' can move to the head of 
>    kernel/uprobes.c, without any ugly #ifdefs and wrappery - 
>    that code only compiles if uprobes are enabled and if the 
>    architecture supports it.
> 
>  - asm/uprobes.h defines 'struct arch_uprobe' and the arch 
>    method(s) - nothing else.
> 
>  - write_opcode() and any similar functions should be renamed to 
>    the arch_uprobes_write_opcode() pattern

Currently the kernel/uprobes.c code handles insn as arch agnostic in
some cases and uses arch specific stuff for analysis, verification and
to set up fixups. The analysis, verification, and fixups is only done at
the probe insertion only.

The copy_insn code, write_opcode is mostly arch agnostic except for
the maximum length of any supported instruction for that architecture.
If we move the insn to arch_uprobe, then we would have to duplicate this 
code in arch specific files to do the copying of the instruction.
(not only at registration/unregistration times and also at probe hit
time to copy into the slot).
So I am still tempted to keep insn in struct uprobe rather than
arch_uprobe.

> 
>  - unless uprobe_opcode_t is non-byte on any architecture this 
>    typedef should probably be go away and be char * or void *.
> 

For architectures like powerpc, uprobe_opcode_t would be non-byte.
But I think we could still manage to remove uprobe_opcode_t.

> These changes should be done on top of latest -tip, in a single 
> patch or a small series, it's mostly pretty straightforward.
> 
> Please keep an eye on stylistic details for future patches - the 
> above patch gives a laundry list of what details were wrong in 
> the existing code.
> 
> Once this is done can we add #2, #3, etc. patches in a clean 
> manner.
> 

Okay.

> I'd suggest you don't send a full series but send small, edible 
> steps on top of the minimal (and right now not yet functional) 
> uprobes core code I've already applied, in series of 1-3 
> patches.
> 
> I'm sure there will be other small details we'll have to change 
> in these patches as we go forward, so not porting the full 
> series straight away would save you time and effort that the 
> inevitable conflicts create.

Okay. 

> 
> As an additional benefit, if you do it in such a workflow then I 
> can apply your new patches to tip:perf/uprobes pretty much the 
> moment you send them out, reviewing and testing them quickly - 
> without the time-consuming review of a long and interdependent 
> patch series and the resulting churn on your side.
> 
> I believe we could iterate this through in a couple of days and 
> arrive to a working 'perf probe' prototype.

Okay.

-- 
Thanks and Regards
Srikar

Re: [tip:perf/uprobes] uprobes/core: Clean up, refactor and improve the code

[Srikar Dronamraju]

* Ingo Molnar <mingo@elte.hu> [2012-02-20 08:38:23]:

> 
> * Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:
> 
> > The volatiles were added to arch/x86/kernel/kprobes.c because 
> > of commit 7115e3fcf45 and 315eb8a2a1b. The volatiles are 
> > required because gcc 4.6 gave a warning about the asm operand 
> > for test_bit.  So the same were added to 
> > arch/x86/kernel/uprobes.c.
> 
> Seems like a GCC bug - a bogus warning - or does it generate bad 
> code as well?

Yes it is a gcc bug and was fixed by Jakub.
As per Josh, only the first long is output if compiled on the buggy gcc.

> 
> In any case, kprobes.c did it correctly, it added the volatile 
> *and a comment*, pointing out that it's a GCC bug. No such 
> warning was added to uprobes.c, making the volatile look 
> entirely spurious.

okay. 

> 
> So feel free to re-add the volatile in a followup patch, just 
> make sure the GCC workaround nature is documented.
> 
> Thanks,
> 
> 	Ingo
> 

Re: [tip:perf/uprobes] uprobes/core: Clean up, refactor and improve the code

[Ingo Molnar]

* Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

> >  - uprobe->insn[] needs to move from struct uprobe to
> >    uprobe->arch.insn
> > 
> >  - The uprobes_arch_*() method(s) should be passed a
> >    'struct arch_uprobe *', not a 'struct uprobe *'.
> > 
> >  - Once this is done, 'struct uprobe' can move to the head of 
> >    kernel/uprobes.c, without any ugly #ifdefs and wrappery - 
> >    that code only compiles if uprobes are enabled and if the 
> >    architecture supports it.
> > 
> >  - asm/uprobes.h defines 'struct arch_uprobe' and the arch 
> >    method(s) - nothing else.
> > 
> >  - write_opcode() and any similar functions should be renamed to 
> >    the arch_uprobes_write_opcode() pattern
> 
> Currently the kernel/uprobes.c code handles insn as arch 
> agnostic in some cases and uses arch specific stuff for 
> analysis, verification and to set up fixups. The analysis, 
> verification, and fixups is only done at the probe insertion 
> only.
> 
> The copy_insn code, write_opcode is mostly arch agnostic 
> except for the maximum length of any supported instruction for 
> that architecture. If we move the insn to arch_uprobe, then we 
> would have to duplicate this code in arch specific files to do 
> the copying of the instruction. (not only at 
> registration/unregistration times and also at probe hit time 
> to copy into the slot).

Is there any reason why the core kernel uprobes.c code could not 
use uprobe->arch.insn directly?

It's in the architecture specific structure, mainly to 
encapsulate architecture-accessible fields and isolate low-level 
functionality from high-level one. This does not preclude the 
high-level code from using that field though.

Obviously every uprobes supporting architecture would have to 
define an 'insn' field in their 'struct arch_uprobe'.

Thanks,

	Ingo

Re: [tip:perf/uprobes] uprobes/core: Clean up, refactor and improve the code

[Ingo Molnar]

* Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:

> * Ingo Molnar <mingo@elte.hu> [2012-02-20 08:38:23]:
> 
> > 
> > * Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:
> > 
> > > The volatiles were added to arch/x86/kernel/kprobes.c because 
> > > of commit 7115e3fcf45 and 315eb8a2a1b. The volatiles are 
> > > required because gcc 4.6 gave a warning about the asm operand 
> > > for test_bit.  So the same were added to 
> > > arch/x86/kernel/uprobes.c.
> > 
> > Seems like a GCC bug - a bogus warning - or does it generate bad 
> > code as well?
> 
> Yes it is a gcc bug and was fixed by Jakub.
> As per Josh, only the first long is output if compiled on the buggy gcc.

That's an important piece of information - the more reason to 
document the quirk.

> > In any case, kprobes.c did it correctly, it added the volatile 
> > *and a comment*, pointing out that it's a GCC bug. No such 
> > warning was added to uprobes.c, making the volatile look 
> > entirely spurious.
> 
> okay. 

Thanks,

	Ingo