2014-06-01 19:26:12

by Oleg Nesterov

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

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 <[email protected]>
Signed-off-by: Oleg Nesterov <[email protected]>
---
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


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

(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 <[email protected]>
> Signed-off-by: Oleg Nesterov <[email protected]>

Looks OK for me :)

Reviewed-by: Masami Hiramatsu <[email protected]>

> ---
> 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: [email protected]

2014-06-02 06:04:51

by Srikar Dronamraju

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

* Oleg Nesterov <[email protected]> [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 <[email protected]>
> Signed-off-by: Oleg Nesterov <[email protected]>
> ---

Acked-by: Srikar Dronamraju <[email protected]>

--
Thanks and Regards
Srikar Dronamraju

2014-06-02 15:28:07

by Jim Keniston

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

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 <[email protected]>
> Signed-off-by: Oleg Nesterov <[email protected]>

Looks good. Thanks.

Reviewed-by: Jim Keniston <[email protected]>

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

2014-06-03 18:21:48

by Ingo Molnar

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


* Oleg Nesterov <[email protected]> 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 <[email protected]>
> Signed-off-by: Oleg Nesterov <[email protected]>
> ---
> 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

2014-06-03 18:31:51

by Oleg Nesterov

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

On 06/03, Ingo Molnar wrote:
>
> * Oleg Nesterov <[email protected]> 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.

2014-06-03 18:39:26

by Ingo Molnar

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


* Oleg Nesterov <[email protected]> wrote:

> On 06/03, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <[email protected]> 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

2014-06-03 19:14:08

by Oleg Nesterov

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

On 06/03, Ingo Molnar wrote:
>
> * Oleg Nesterov <[email protected]> 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.

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

(2014/06/04 4:13), Oleg Nesterov wrote:
> On 06/03, Ingo Molnar wrote:
>>
>> * Oleg Nesterov <[email protected]> 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: [email protected]

2014-06-04 16:40:40

by Oleg Nesterov

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

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.

2014-06-05 09:28:56

by Ingo Molnar

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


* Oleg Nesterov <[email protected]> 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

2014-06-05 14:34:13

by Oleg Nesterov

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

On 06/05, Ingo Molnar wrote:
>
> * Oleg Nesterov <[email protected]> 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(-)

2014-06-05 14:54:53

by Ingo Molnar

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


* Oleg Nesterov <[email protected]> wrote:

> On 06/05, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <[email protected]> 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