[PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[Oleg Nesterov]

Purely cosmetic, no changes in .o,

1. As Jim pointed out arch_uprobe->def looks ambiguous, rename it to
   ->dflt.

2. Add the comment into default_post_xol_op() to explain "regs->sp +=".

3. Remove the stale part of the comment in arch_uprobe_analyze_insn().

Suggested-by: Jim Keniston <jkenisto@us.ibm.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/include/asm/uprobes.h |    2 +-
 arch/x86/kernel/uprobes.c      |   37 ++++++++++++++++++-------------------
 2 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
index 7be3c07..b3d9442 100644
--- a/arch/x86/include/asm/uprobes.h
+++ b/arch/x86/include/asm/uprobes.h
@@ -52,7 +52,7 @@ struct arch_uprobe {
 		struct {
 			u8	fixups;
 			u8	ilen;
-		} 			def;
+		} 			dflt;
 	};
 };
 
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index fcf6279..33e239f 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -254,7 +254,7 @@ static inline bool is_64bit_mm(struct mm_struct *mm)
  * If arch_uprobe->insn doesn't use rip-relative addressing, return
  * immediately.  Otherwise, rewrite the instruction so that it accesses
  * its memory operand indirectly through a scratch register.  Set
- * def->fixups accordingly. (The contents of the scratch register
+ * dflt->fixups accordingly. (The contents of the scratch register
  * will be saved before we single-step the modified instruction,
  * and restored afterward).
  *
@@ -372,14 +372,14 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
 	 */
 	if (reg != 6 && reg2 != 6) {
 		reg2 = 6;
-		auprobe->def.fixups |= UPROBE_FIX_RIP_SI;
+		auprobe->dflt.fixups |= UPROBE_FIX_RIP_SI;
 	} else if (reg != 7 && reg2 != 7) {
 		reg2 = 7;
-		auprobe->def.fixups |= UPROBE_FIX_RIP_DI;
+		auprobe->dflt.fixups |= UPROBE_FIX_RIP_DI;
 		/* TODO (paranoia): force maskmovq to not use di */
 	} else {
 		reg2 = 3;
-		auprobe->def.fixups |= UPROBE_FIX_RIP_BX;
+		auprobe->dflt.fixups |= UPROBE_FIX_RIP_BX;
 	}
 	/*
 	 * Point cursor at the modrm byte.  The next 4 bytes are the
@@ -398,9 +398,9 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
 static inline unsigned long *
 scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_SI)
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_SI)
 		return &regs->si;
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_DI)
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_DI)
 		return &regs->di;
 	return &regs->bx;
 }
@@ -411,18 +411,18 @@ scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
  */
 static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) {
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) {
 		struct uprobe_task *utask = current->utask;
 		unsigned long *sr = scratch_reg(auprobe, regs);
 
 		utask->autask.saved_scratch_register = *sr;
-		*sr = utask->vaddr + auprobe->def.ilen;
+		*sr = utask->vaddr + auprobe->dflt.ilen;
 	}
 }
 
 static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) {
+	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) {
 		struct uprobe_task *utask = current->utask;
 		unsigned long *sr = scratch_reg(auprobe, regs);
 
@@ -499,16 +499,16 @@ static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs
 	struct uprobe_task *utask = current->utask;
 
 	riprel_post_xol(auprobe, regs);
-	if (auprobe->def.fixups & UPROBE_FIX_IP) {
+	if (auprobe->dflt.fixups & UPROBE_FIX_IP) {
 		long correction = utask->vaddr - utask->xol_vaddr;
 		regs->ip += correction;
-	} else if (auprobe->def.fixups & UPROBE_FIX_CALL) {
-		regs->sp += sizeof_long();
-		if (push_ret_address(regs, utask->vaddr + auprobe->def.ilen))
+	} else if (auprobe->dflt.fixups & UPROBE_FIX_CALL) {
+		regs->sp += sizeof_long(); /* Pop incorrect return address */
+		if (push_ret_address(regs, utask->vaddr + auprobe->dflt.ilen))
 			return -ERESTART;
 	}
 	/* popf; tell the caller to not touch TF */
-	if (auprobe->def.fixups & UPROBE_FIX_SETF)
+	if (auprobe->dflt.fixups & UPROBE_FIX_SETF)
 		utask->autask.saved_tf = true;
 
 	return 0;
@@ -711,12 +711,11 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 
 	/*
 	 * Figure out which fixups default_post_xol_op() will need to perform,
-	 * and annotate def->fixups accordingly. To start with, ->fixups is
-	 * either zero or it reflects rip-related fixups.
+	 * and annotate dflt->fixups accordingly.
 	 */
 	switch (OPCODE1(&insn)) {
 	case 0x9d:		/* popf */
-		auprobe->def.fixups |= UPROBE_FIX_SETF;
+		auprobe->dflt.fixups |= UPROBE_FIX_SETF;
 		break;
 	case 0xc3:		/* ret or lret -- ip is correct */
 	case 0xcb:
@@ -742,8 +741,8 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 		riprel_analyze(auprobe, &insn);
 	}
 
-	auprobe->def.ilen = insn.length;
-	auprobe->def.fixups |= fix_ip_or_call;
+	auprobe->dflt.ilen = insn.length;
+	auprobe->dflt.fixups |= fix_ip_or_call;
 
 	auprobe->ops = &default_xol_ops;
 	return 0;
-- 
1.5.5.1

Re: [PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[Masami Hiramatsu]

(2014/06/02 4:25), Oleg Nesterov wrote:
> Purely cosmetic, no changes in .o,
> 
> 1. As Jim pointed out arch_uprobe->def looks ambiguous, rename it to
>    ->dflt.
> 
> 2. Add the comment into default_post_xol_op() to explain "regs->sp +=".
> 
> 3. Remove the stale part of the comment in arch_uprobe_analyze_insn().
> 
> Suggested-by: Jim Keniston <jkenisto@us.ibm.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Looks OK for me :)

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

> ---
>  arch/x86/include/asm/uprobes.h |    2 +-
>  arch/x86/kernel/uprobes.c      |   37 ++++++++++++++++++-------------------
>  2 files changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index 7be3c07..b3d9442 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -52,7 +52,7 @@ struct arch_uprobe {
>  		struct {
>  			u8	fixups;
>  			u8	ilen;
> -		} 			def;
> +		} 			dflt;
>  	};
>  };
>  
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index fcf6279..33e239f 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -254,7 +254,7 @@ static inline bool is_64bit_mm(struct mm_struct *mm)
>   * If arch_uprobe->insn doesn't use rip-relative addressing, return
>   * immediately.  Otherwise, rewrite the instruction so that it accesses
>   * its memory operand indirectly through a scratch register.  Set
> - * def->fixups accordingly. (The contents of the scratch register
> + * dflt->fixups accordingly. (The contents of the scratch register
>   * will be saved before we single-step the modified instruction,
>   * and restored afterward).
>   *
> @@ -372,14 +372,14 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
>  	 */
>  	if (reg != 6 && reg2 != 6) {
>  		reg2 = 6;
> -		auprobe->def.fixups |= UPROBE_FIX_RIP_SI;
> +		auprobe->dflt.fixups |= UPROBE_FIX_RIP_SI;
>  	} else if (reg != 7 && reg2 != 7) {
>  		reg2 = 7;
> -		auprobe->def.fixups |= UPROBE_FIX_RIP_DI;
> +		auprobe->dflt.fixups |= UPROBE_FIX_RIP_DI;
>  		/* TODO (paranoia): force maskmovq to not use di */
>  	} else {
>  		reg2 = 3;
> -		auprobe->def.fixups |= UPROBE_FIX_RIP_BX;
> +		auprobe->dflt.fixups |= UPROBE_FIX_RIP_BX;
>  	}
>  	/*
>  	 * Point cursor at the modrm byte.  The next 4 bytes are the
> @@ -398,9 +398,9 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
>  static inline unsigned long *
>  scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  {
> -	if (auprobe->def.fixups & UPROBE_FIX_RIP_SI)
> +	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_SI)
>  		return &regs->si;
> -	if (auprobe->def.fixups & UPROBE_FIX_RIP_DI)
> +	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_DI)
>  		return &regs->di;
>  	return &regs->bx;
>  }
> @@ -411,18 +411,18 @@ scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
>   */
>  static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  {
> -	if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) {
> +	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) {
>  		struct uprobe_task *utask = current->utask;
>  		unsigned long *sr = scratch_reg(auprobe, regs);
>  
>  		utask->autask.saved_scratch_register = *sr;
> -		*sr = utask->vaddr + auprobe->def.ilen;
> +		*sr = utask->vaddr + auprobe->dflt.ilen;
>  	}
>  }
>  
>  static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  {
> -	if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) {
> +	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) {
>  		struct uprobe_task *utask = current->utask;
>  		unsigned long *sr = scratch_reg(auprobe, regs);
>  
> @@ -499,16 +499,16 @@ static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs
>  	struct uprobe_task *utask = current->utask;
>  
>  	riprel_post_xol(auprobe, regs);
> -	if (auprobe->def.fixups & UPROBE_FIX_IP) {
> +	if (auprobe->dflt.fixups & UPROBE_FIX_IP) {
>  		long correction = utask->vaddr - utask->xol_vaddr;
>  		regs->ip += correction;
> -	} else if (auprobe->def.fixups & UPROBE_FIX_CALL) {
> -		regs->sp += sizeof_long();
> -		if (push_ret_address(regs, utask->vaddr + auprobe->def.ilen))
> +	} else if (auprobe->dflt.fixups & UPROBE_FIX_CALL) {
> +		regs->sp += sizeof_long(); /* Pop incorrect return address */
> +		if (push_ret_address(regs, utask->vaddr + auprobe->dflt.ilen))
>  			return -ERESTART;
>  	}
>  	/* popf; tell the caller to not touch TF */
> -	if (auprobe->def.fixups & UPROBE_FIX_SETF)
> +	if (auprobe->dflt.fixups & UPROBE_FIX_SETF)
>  		utask->autask.saved_tf = true;
>  
>  	return 0;
> @@ -711,12 +711,11 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  
>  	/*
>  	 * Figure out which fixups default_post_xol_op() will need to perform,
> -	 * and annotate def->fixups accordingly. To start with, ->fixups is
> -	 * either zero or it reflects rip-related fixups.
> +	 * and annotate dflt->fixups accordingly.
>  	 */
>  	switch (OPCODE1(&insn)) {
>  	case 0x9d:		/* popf */
> -		auprobe->def.fixups |= UPROBE_FIX_SETF;
> +		auprobe->dflt.fixups |= UPROBE_FIX_SETF;
>  		break;
>  	case 0xc3:		/* ret or lret -- ip is correct */
>  	case 0xcb:
> @@ -742,8 +741,8 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  		riprel_analyze(auprobe, &insn);
>  	}
>  
> -	auprobe->def.ilen = insn.length;
> -	auprobe->def.fixups |= fix_ip_or_call;
> +	auprobe->dflt.ilen = insn.length;
> +	auprobe->dflt.fixups |= fix_ip_or_call;
>  
>  	auprobe->ops = &default_xol_ops;
>  	return 0;
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2014-06-01 21:25:20]:

> Purely cosmetic, no changes in .o,
> 
> 1. As Jim pointed out arch_uprobe->def looks ambiguous, rename it to
>    ->dflt.
> 
> 2. Add the comment into default_post_xol_op() to explain "regs->sp +=".
> 
> 3. Remove the stale part of the comment in arch_uprobe_analyze_insn().
> 
> Suggested-by: Jim Keniston <jkenisto@us.ibm.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---

Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[Jim Keniston]

On Sun, 2014-06-01 at 21:25 +0200, Oleg Nesterov wrote:
> Purely cosmetic, no changes in .o,
> 
> 1. As Jim pointed out arch_uprobe->def looks ambiguous, rename it to
>    ->dflt.
> 
> 2. Add the comment into default_post_xol_op() to explain "regs->sp +=".
> 
> 3. Remove the stale part of the comment in arch_uprobe_analyze_insn().
> 
> Suggested-by: Jim Keniston <jkenisto@us.ibm.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Looks good.  Thanks.

Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>

> ---
>  arch/x86/include/asm/uprobes.h |    2 +-
>  arch/x86/kernel/uprobes.c      |   37 ++++++++++++++++++-------------------
>  2 files changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index 7be3c07..b3d9442 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -52,7 +52,7 @@ struct arch_uprobe {
>  		struct {
>  			u8	fixups;
>  			u8	ilen;
> -		} 			def;
> +		} 			dflt;
>  	};
>  };
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index fcf6279..33e239f 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -254,7 +254,7 @@ static inline bool is_64bit_mm(struct mm_struct *mm)
>   * If arch_uprobe->insn doesn't use rip-relative addressing, return
>   * immediately.  Otherwise, rewrite the instruction so that it accesses
>   * its memory operand indirectly through a scratch register.  Set
> - * def->fixups accordingly. (The contents of the scratch register
> + * dflt->fixups accordingly. (The contents of the scratch register
>   * will be saved before we single-step the modified instruction,
>   * and restored afterward).
>   *
> @@ -372,14 +372,14 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
>  	 */
>  	if (reg != 6 && reg2 != 6) {
>  		reg2 = 6;
> -		auprobe->def.fixups |= UPROBE_FIX_RIP_SI;
> +		auprobe->dflt.fixups |= UPROBE_FIX_RIP_SI;
>  	} else if (reg != 7 && reg2 != 7) {
>  		reg2 = 7;
> -		auprobe->def.fixups |= UPROBE_FIX_RIP_DI;
> +		auprobe->dflt.fixups |= UPROBE_FIX_RIP_DI;
>  		/* TODO (paranoia): force maskmovq to not use di */
>  	} else {
>  		reg2 = 3;
> -		auprobe->def.fixups |= UPROBE_FIX_RIP_BX;
> +		auprobe->dflt.fixups |= UPROBE_FIX_RIP_BX;
>  	}
>  	/*
>  	 * Point cursor at the modrm byte.  The next 4 bytes are the
> @@ -398,9 +398,9 @@ static void riprel_analyze(struct arch_uprobe *auprobe, struct insn *insn)
>  static inline unsigned long *
>  scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  {
> -	if (auprobe->def.fixups & UPROBE_FIX_RIP_SI)
> +	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_SI)
>  		return &regs->si;
> -	if (auprobe->def.fixups & UPROBE_FIX_RIP_DI)
> +	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_DI)
>  		return &regs->di;
>  	return &regs->bx;
>  }
> @@ -411,18 +411,18 @@ scratch_reg(struct arch_uprobe *auprobe, struct pt_regs *regs)
>   */
>  static void riprel_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  {
> -	if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) {
> +	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) {
>  		struct uprobe_task *utask = current->utask;
>  		unsigned long *sr = scratch_reg(auprobe, regs);
> 
>  		utask->autask.saved_scratch_register = *sr;
> -		*sr = utask->vaddr + auprobe->def.ilen;
> +		*sr = utask->vaddr + auprobe->dflt.ilen;
>  	}
>  }
> 
>  static void riprel_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  {
> -	if (auprobe->def.fixups & UPROBE_FIX_RIP_MASK) {
> +	if (auprobe->dflt.fixups & UPROBE_FIX_RIP_MASK) {
>  		struct uprobe_task *utask = current->utask;
>  		unsigned long *sr = scratch_reg(auprobe, regs);
> 
> @@ -499,16 +499,16 @@ static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs
>  	struct uprobe_task *utask = current->utask;
> 
>  	riprel_post_xol(auprobe, regs);
> -	if (auprobe->def.fixups & UPROBE_FIX_IP) {
> +	if (auprobe->dflt.fixups & UPROBE_FIX_IP) {
>  		long correction = utask->vaddr - utask->xol_vaddr;
>  		regs->ip += correction;
> -	} else if (auprobe->def.fixups & UPROBE_FIX_CALL) {
> -		regs->sp += sizeof_long();
> -		if (push_ret_address(regs, utask->vaddr + auprobe->def.ilen))
> +	} else if (auprobe->dflt.fixups & UPROBE_FIX_CALL) {
> +		regs->sp += sizeof_long(); /* Pop incorrect return address */
> +		if (push_ret_address(regs, utask->vaddr + auprobe->dflt.ilen))
>  			return -ERESTART;
>  	}
>  	/* popf; tell the caller to not touch TF */
> -	if (auprobe->def.fixups & UPROBE_FIX_SETF)
> +	if (auprobe->dflt.fixups & UPROBE_FIX_SETF)
>  		utask->autask.saved_tf = true;
> 
>  	return 0;
> @@ -711,12 +711,11 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> 
>  	/*
>  	 * Figure out which fixups default_post_xol_op() will need to perform,
> -	 * and annotate def->fixups accordingly. To start with, ->fixups is
> -	 * either zero or it reflects rip-related fixups.
> +	 * and annotate dflt->fixups accordingly.
>  	 */
>  	switch (OPCODE1(&insn)) {
>  	case 0x9d:		/* popf */
> -		auprobe->def.fixups |= UPROBE_FIX_SETF;
> +		auprobe->dflt.fixups |= UPROBE_FIX_SETF;
>  		break;
>  	case 0xc3:		/* ret or lret -- ip is correct */
>  	case 0xcb:
> @@ -742,8 +741,8 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  		riprel_analyze(auprobe, &insn);
>  	}
> 
> -	auprobe->def.ilen = insn.length;
> -	auprobe->def.fixups |= fix_ip_or_call;
> +	auprobe->dflt.ilen = insn.length;
> +	auprobe->dflt.fixups |= fix_ip_or_call;
> 
>  	auprobe->ops = &default_xol_ops;
>  	return 0;

Re: [PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> Purely cosmetic, no changes in .o,
> 
> 1. As Jim pointed out arch_uprobe->def looks ambiguous, rename it to
>    ->dflt.
> 
> 2. Add the comment into default_post_xol_op() to explain "regs->sp +=".
> 
> 3. Remove the stale part of the comment in arch_uprobe_analyze_insn().
> 
> Suggested-by: Jim Keniston <jkenisto@us.ibm.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/x86/include/asm/uprobes.h |    2 +-
>  arch/x86/kernel/uprobes.c      |   37 ++++++++++++++++++-------------------
>  2 files changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index 7be3c07..b3d9442 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -52,7 +52,7 @@ struct arch_uprobe {
>  		struct {
>  			u8	fixups;
>  			u8	ilen;
> -		} 			def;
> +		} 			dflt;

Pls lts nt use slly abbrvtns, ok?

How about arch_uprobe->default?

Thanks,

	Ingo

Re: [PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[Oleg Nesterov]

On 06/03, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > --- a/arch/x86/include/asm/uprobes.h
> > +++ b/arch/x86/include/asm/uprobes.h
> > @@ -52,7 +52,7 @@ struct arch_uprobe {
> >  		struct {
> >  			u8	fixups;
> >  			u8	ilen;
> > -		} 			def;
> > +		} 			dflt;
>
> Pls lts nt use slly abbrvtns, ok?

OK. As I said in the previous dicussion, I agree with any naming.

> How about arch_uprobe->default?

And this is how it was named when I wrote this code. Unfortunately gcc
dislikes this name ;) So I renamed it to ->def. Then I was asked to
rename it and I agree, ->def doesn't look good.

Could you suggest something better?

Oleg.

Re: [PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 06/03, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > --- a/arch/x86/include/asm/uprobes.h
> > > +++ b/arch/x86/include/asm/uprobes.h
> > > @@ -52,7 +52,7 @@ struct arch_uprobe {
> > >  		struct {
> > >  			u8	fixups;
> > >  			u8	ilen;
> > > -		} 			def;
> > > +		} 			dflt;
> >
> > Pls lts nt use slly abbrvtns, ok?
> 
> OK. As I said in the previous dicussion, I agree with any naming.
> 
> > How about arch_uprobe->default?
> 
> And this is how it was named when I wrote this code. Unfortunately gcc
> dislikes this name ;) So I renamed it to ->def. Then I was asked to
> rename it and I agree, ->def doesn't look good.
> 
> Could you suggest something better?

So exactly what do those fields do? If it's scratch register handling, 
would it be logical to name it arch_uprobe->scratch, or so?

Thanks,

	Ingo

Re: [PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[Oleg Nesterov]

On 06/03, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > And this is how it was named when I wrote this code. Unfortunately gcc
> > dislikes this name ;) So I renamed it to ->def. Then I was asked to
> > rename it and I agree, ->def doesn't look good.
> >
> > Could you suggest something better?
>
> So exactly what do those fields do? If it's scratch register handling,
> would it be logical to name it arch_uprobe->scratch, or so?

Not only, ->fixups encodes other flags. and ->ilen is used by UPROBE_FIX_CALL.

arch_uprobe->def contains the arguments for default_xol_ops methods, currently
this handles everything except relative jmp/call insns.

So perhaps ->dflt is not that ugly in this case? I simply do not see anything
better. But again, I agree with any name in advance.

Oleg.

Re: Re: [PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[Masami Hiramatsu]

(2014/06/04 4:13), Oleg Nesterov wrote:
> On 06/03, Ingo Molnar wrote:
>>
>> * Oleg Nesterov <oleg@redhat.com> wrote:
>>
>>> And this is how it was named when I wrote this code. Unfortunately gcc
>>> dislikes this name ;) So I renamed it to ->def. Then I was asked to
>>> rename it and I agree, ->def doesn't look good.
>>>
>>> Could you suggest something better?
>>
>> So exactly what do those fields do? If it's scratch register handling,
>> would it be logical to name it arch_uprobe->scratch, or so?
> 
> Not only, ->fixups encodes other flags. and ->ilen is used by UPROBE_FIX_CALL.
> 
> arch_uprobe->def contains the arguments for default_xol_ops methods, currently
> this handles everything except relative jmp/call insns.
> 
> So perhaps ->dflt is not that ugly in this case? I simply do not see anything
> better. But again, I agree with any name in advance.

Hmm, how about ->defparam ? :)

Thank you,

-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: Re: [PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[Oleg Nesterov]

On 06/04, Masami Hiramatsu wrote:
>
> (2014/06/04 4:13), Oleg Nesterov wrote:
> > On 06/03, Ingo Molnar wrote:
> >>
> >> So exactly what do those fields do? If it's scratch register handling,
> >> would it be logical to name it arch_uprobe->scratch, or so?
> >
> > Not only, ->fixups encodes other flags. and ->ilen is used by UPROBE_FIX_CALL.
> >
> > arch_uprobe->def contains the arguments for default_xol_ops methods, currently
> > this handles everything except relative jmp/call insns.
> >
> > So perhaps ->dflt is not that ugly in this case? I simply do not see anything
> > better. But again, I agree with any name in advance.
>
> Hmm, how about ->defparam ? :)

Fine with me ;)

Ingo, will you agree with s/def/defparam/ ?

Oleg.

Re: Re: [PATCH] uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 06/04, Masami Hiramatsu wrote:
> >
> > (2014/06/04 4:13), Oleg Nesterov wrote:
> > > On 06/03, Ingo Molnar wrote:
> > >>
> > >> So exactly what do those fields do? If it's scratch register handling,
> > >> would it be logical to name it arch_uprobe->scratch, or so?
> > >
> > > Not only, ->fixups encodes other flags. and ->ilen is used by UPROBE_FIX_CALL.
> > >
> > > arch_uprobe->def contains the arguments for default_xol_ops methods, currently
> > > this handles everything except relative jmp/call insns.
> > >
> > > So perhaps ->dflt is not that ugly in this case? I simply do not see anything
> > > better. But again, I agree with any name in advance.
> >
> > Hmm, how about ->defparam ? :)
> 
> Fine with me ;)
> 
> Ingo, will you agree with s/def/defparam/ ?

Certainly! :)

Thanks,

	Ingo

[GIT PULL] uprobes: tmpfs support (Was: uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates)

[Oleg Nesterov]

On 06/05, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 06/04, Masami Hiramatsu wrote:
> > >
> > > Hmm, how about ->defparam ? :)
> >
> > Fine with me ;)
> >
> > Ingo, will you agree with s/def/defparam/ ?
>
> Certainly! :)

Great ;) I updated this patch in uprobes/core, please pull from

  git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core

based on tip:perf/uprobes


Oleg Nesterov (3):
      uprobes: Shift ->readpage check from __copy_insn() to uprobe_register()
      uprobes: Teach copy_insn() to support tmpfs
      uprobes/x86: Rename arch_uprobe->def to ->defparam, minor comment updates

 arch/x86/include/asm/uprobes.h |    2 +-
 arch/x86/kernel/uprobes.c      |   37 ++++++++++++++++++-------------------
 kernel/events/uprobes.c        |   17 +++++++++++------
 3 files changed, 30 insertions(+), 26 deletions(-)

Re: [GIT PULL] uprobes: tmpfs support (Was: uprobes/x86: Rename arch_uprobe->def into ->dflt, minor comment updates)

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 06/05, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > On 06/04, Masami Hiramatsu wrote:
> > > >
> > > > Hmm, how about ->defparam ? :)
> > >
> > > Fine with me ;)
> > >
> > > Ingo, will you agree with s/def/defparam/ ?
> >
> > Certainly! :)
> 
> Great ;) I updated this patch in uprobes/core, please pull from
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core
> 
> based on tip:perf/uprobes
> 
> 
> Oleg Nesterov (3):
>       uprobes: Shift ->readpage check from __copy_insn() to uprobe_register()
>       uprobes: Teach copy_insn() to support tmpfs
>       uprobes/x86: Rename arch_uprobe->def to ->defparam, minor comment updates
> 
>  arch/x86/include/asm/uprobes.h |    2 +-
>  arch/x86/kernel/uprobes.c      |   37 ++++++++++++++++++-------------------
>  kernel/events/uprobes.c        |   17 +++++++++++------
>  3 files changed, 30 insertions(+), 26 deletions(-)

Pulled, thanks a lot Oleg!

	Ingo