[PATCH] uprobes: Add uprobe_task->dup_work/dup_addr

[PATCH] uprobes: Add uprobe_task->dup_work/dup_addr

[Oleg Nesterov]

uprobe_task->vaddr is a bit strange. First of all it is not really
needed, we can move it into arch_uprobe_task. The generic code uses
it only to pass the additional argument to arch_uprobe_pre_xol(),
and since it is always equal to instruction_pointer() this looks
even more strange.

And both utask->vaddr and and utask->autask have the same scope,
they only have the meaning when the task executes the probed insn
out-of-line. This means it is safe to reuse both in UTASK_RUNNING
state.

OTOH, it is also used by uprobe_copy_process() and dup_xol_work()
for another purpose, this doesn't look clean and doesn't allow to
move this member into arch_uprobe_task.

This patch adds the union with 2 anonymous structs into uprobe_task.

The first struct is autask + vaddr, this way we "almost" move vaddr
into autask.

The second struct has 2 new members for uprobe_copy_process() paths:
->dup_addr which can be used instead ->vaddr, and ->dup_work which
can be used to avoid kmalloc() and simplify the code.

Note that this union will likely have another member(s), we need
something like "private_data_for_handlers" so that the tracing
handlers could use it to communicate with call_fetch() methods.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/uprobes.h |   21 ++++++++++++++++-----
 kernel/events/uprobes.c |   16 ++++------------
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 319eae7..366b8b2 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -26,6 +26,7 @@
 
 #include <linux/errno.h>
 #include <linux/rbtree.h>
+#include <linux/types.h>
 
 struct vm_area_struct;
 struct mm_struct;
@@ -72,14 +73,24 @@ enum uprobe_task_state {
  */
 struct uprobe_task {
 	enum uprobe_task_state		state;
-	struct arch_uprobe_task		autask;
 
-	struct return_instance		*return_instances;
-	unsigned int			depth;
-	struct uprobe			*active_uprobe;
+	union {
+		struct {
+			struct arch_uprobe_task	autask;
+			unsigned long		vaddr;
+		};
+
+		struct {
+			struct callback_head	dup_work;
+			unsigned long		dup_addr;
+		};
+	};
 
+	struct uprobe			*active_uprobe;
 	unsigned long			xol_vaddr;
-	unsigned long			vaddr;
+
+	struct return_instance		*return_instances;
+	unsigned int			depth;
 };
 
 /*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 24b7d6c..2546a7b 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1403,12 +1403,10 @@ static void uprobe_warn(struct task_struct *t, const char *msg)
 
 static void dup_xol_work(struct callback_head *work)
 {
-	kfree(work);
-
 	if (current->flags & PF_EXITING)
 		return;
 
-	if (!__create_xol_area(current->utask->vaddr))
+	if (!__create_xol_area(current->utask->dup_addr))
 		uprobe_warn(current, "dup xol area");
 }
 
@@ -1419,7 +1417,6 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
 {
 	struct uprobe_task *utask = current->utask;
 	struct mm_struct *mm = current->mm;
-	struct callback_head *work;
 	struct xol_area *area;
 
 	t->utask = NULL;
@@ -1441,14 +1438,9 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
 	if (mm == t->mm)
 		return;
 
-	/* TODO: move it into the union in uprobe_task */
-	work = kmalloc(sizeof(*work), GFP_KERNEL);
-	if (!work)
-		return uprobe_warn(t, "dup xol area");
-
-	t->utask->vaddr = area->vaddr;
-	init_task_work(work, dup_xol_work);
-	task_work_add(t, work, true);
+	t->utask->dup_addr = area->vaddr;
+	init_task_work(&t->utask->dup_work, dup_xol_work);
+	task_work_add(t, &t->utask->dup_work, true);
 }
 
 /*
-- 
1.5.5.1

Re: [PATCH] uprobes: Add uprobe_task->dup_work/dup_addr

[Masami Hiramatsu]

(2013/11/09 4:00), Oleg Nesterov wrote:
> uprobe_task->vaddr is a bit strange. First of all it is not really
> needed, we can move it into arch_uprobe_task. The generic code uses
> it only to pass the additional argument to arch_uprobe_pre_xol(),
> and since it is always equal to instruction_pointer() this looks
> even more strange.
> 
> And both utask->vaddr and and utask->autask have the same scope,
> they only have the meaning when the task executes the probed insn
> out-of-line. This means it is safe to reuse both in UTASK_RUNNING
> state.
> 
> OTOH, it is also used by uprobe_copy_process() and dup_xol_work()
> for another purpose, this doesn't look clean and doesn't allow to
> move this member into arch_uprobe_task.
> 
> This patch adds the union with 2 anonymous structs into uprobe_task.
> 
> The first struct is autask + vaddr, this way we "almost" move vaddr
> into autask.
> 
> The second struct has 2 new members for uprobe_copy_process() paths:
> ->dup_addr which can be used instead ->vaddr, and ->dup_work which
> can be used to avoid kmalloc() and simplify the code.

Hmm, I'm not so sure about uprobes implementation so deeply.
Is there no possibility to run xol preparing code (e.g. adding
new uprobe?) between the task_work_add() and task_work_run()?

> Note that this union will likely have another member(s), we need
> something like "private_data_for_handlers" so that the tracing
> handlers could use it to communicate with call_fetch() methods.
> 

If those data structures are small, I think we don't need to
use such union...

Thank you,


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

Re: [PATCH] uprobes: Add uprobe_task->dup_work/dup_addr

[Oleg Nesterov]

On 11/11, Masami Hiramatsu wrote:
>
> (2013/11/09 4:00), Oleg Nesterov wrote:
> > uprobe_task->vaddr is a bit strange. First of all it is not really
> > needed, we can move it into arch_uprobe_task. The generic code uses
> > it only to pass the additional argument to arch_uprobe_pre_xol(),
> > and since it is always equal to instruction_pointer() this looks
> > even more strange.
> >
> > And both utask->vaddr and and utask->autask have the same scope,
> > they only have the meaning when the task executes the probed insn
> > out-of-line. This means it is safe to reuse both in UTASK_RUNNING
> > state.
> >
> > OTOH, it is also used by uprobe_copy_process() and dup_xol_work()
> > for another purpose, this doesn't look clean and doesn't allow to
> > move this member into arch_uprobe_task.
> >
> > This patch adds the union with 2 anonymous structs into uprobe_task.
> >
> > The first struct is autask + vaddr, this way we "almost" move vaddr
> > into autask.
> >
> > The second struct has 2 new members for uprobe_copy_process() paths:
> > ->dup_addr which can be used instead ->vaddr, and ->dup_work which
> > can be used to avoid kmalloc() and simplify the code.
>
> Hmm, I'm not so sure about uprobes implementation so deeply.
> Is there no possibility to run xol preparing code (e.g. adding
> new uprobe?) between the task_work_add() and task_work_run()?

No, task_work_run() must be called before the new child returns
to user-mode.

And it obviously can't hit the breakpoint until it returns to
user mode. "adding new uprobe" doesn't matter at all, the task
itself runs xol preparing code when it hits the bp first time.

> > Note that this union will likely have another member(s), we need
> > something like "private_data_for_handlers" so that the tracing
> > handlers could use it to communicate with call_fetch() methods.
> >
>
> If those data structures are small, I think we don't need to
> use such union...

Well, I disagree. First of all, to me this patch cleanups the code
but this is subjective.

Why should we blow the size of task_struct->utask if there is no
reason?

For example, should we instead add utask->dup_addr outiside of this
union? Or create dup_xol_struct which holds this argument along
with callback_head ? I don't think so. The scope of autask/vaddr and
dup_work/addr is not interactable.

The same for the new ->private (or whatever) we are going to add for
FETCH_MTD_relative. It will only have a meaning inside the ->handler()
paths, to me it would be strange to not reuse the "free" memory we
already have.

Oleg.

Re: Re: [PATCH] uprobes: Add uprobe_task->dup_work/dup_addr

[Masami Hiramatsu]

(2013/11/11 2:28), Oleg Nesterov wrote:
> On 11/11, Masami Hiramatsu wrote:
>>
>> (2013/11/09 4:00), Oleg Nesterov wrote:
>>> uprobe_task->vaddr is a bit strange. First of all it is not really
>>> needed, we can move it into arch_uprobe_task. The generic code uses
>>> it only to pass the additional argument to arch_uprobe_pre_xol(),
>>> and since it is always equal to instruction_pointer() this looks
>>> even more strange.
>>>
>>> And both utask->vaddr and and utask->autask have the same scope,
>>> they only have the meaning when the task executes the probed insn
>>> out-of-line. This means it is safe to reuse both in UTASK_RUNNING
>>> state.
>>>
>>> OTOH, it is also used by uprobe_copy_process() and dup_xol_work()
>>> for another purpose, this doesn't look clean and doesn't allow to
>>> move this member into arch_uprobe_task.
>>>
>>> This patch adds the union with 2 anonymous structs into uprobe_task.
>>>
>>> The first struct is autask + vaddr, this way we "almost" move vaddr
>>> into autask.
>>>
>>> The second struct has 2 new members for uprobe_copy_process() paths:
>>> ->dup_addr which can be used instead ->vaddr, and ->dup_work which
>>> can be used to avoid kmalloc() and simplify the code.
>>
>> Hmm, I'm not so sure about uprobes implementation so deeply.
>> Is there no possibility to run xol preparing code (e.g. adding
>> new uprobe?) between the task_work_add() and task_work_run()?
> 
> No, task_work_run() must be called before the new child returns
> to user-mode.
> 
> And it obviously can't hit the breakpoint until it returns to
> user mode. "adding new uprobe" doesn't matter at all, the task
> itself runs xol preparing code when it hits the bp first time.

Ah, I misunderstood. XOL area should be placed in each process
address space, thus until it hits the probe, uprobe can't
create XOL code, I got it.

>>> Note that this union will likely have another member(s), we need
>>> something like "private_data_for_handlers" so that the tracing
>>> handlers could use it to communicate with call_fetch() methods.
>>>
>>
>> If those data structures are small, I think we don't need to
>> use such union...
> 
> Well, I disagree. First of all, to me this patch cleanups the code
> but this is subjective.
> 
> Why should we blow the size of task_struct->utask if there is no
> reason?
> 
> For example, should we instead add utask->dup_addr outiside of this
> union? Or create dup_xol_struct which holds this argument along
> with callback_head ? I don't think so. The scope of autask/vaddr and
> dup_work/addr is not interactable.

I see your point.

> The same for the new ->private (or whatever) we are going to add for
> FETCH_MTD_relative. It will only have a meaning inside the ->handler()
> paths, to me it would be strange to not reuse the "free" memory we
> already have.

Looks nice ;)

Thank you,

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

Re: [PATCH] uprobes: Add uprobe_task->dup_work/dup_addr

[Masami Hiramatsu]

(2013/11/09 4:00), Oleg Nesterov wrote:
> uprobe_task->vaddr is a bit strange. First of all it is not really
> needed, we can move it into arch_uprobe_task. The generic code uses
> it only to pass the additional argument to arch_uprobe_pre_xol(),
> and since it is always equal to instruction_pointer() this looks
> even more strange.
> 
> And both utask->vaddr and and utask->autask have the same scope,
> they only have the meaning when the task executes the probed insn
> out-of-line. This means it is safe to reuse both in UTASK_RUNNING
> state.
> 
> OTOH, it is also used by uprobe_copy_process() and dup_xol_work()
> for another purpose, this doesn't look clean and doesn't allow to
> move this member into arch_uprobe_task.
> 
> This patch adds the union with 2 anonymous structs into uprobe_task.
> 
> The first struct is autask + vaddr, this way we "almost" move vaddr
> into autask.
> 
> The second struct has 2 new members for uprobe_copy_process() paths:
> ->dup_addr which can be used instead ->vaddr, and ->dup_work which
> can be used to avoid kmalloc() and simplify the code.
> 
> Note that this union will likely have another member(s), we need
> something like "private_data_for_handlers" so that the tracing
> handlers could use it to communicate with call_fetch() methods.

OK, those are used in the different phase as a scratchpad.

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

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

Thanks!

> ---
>  include/linux/uprobes.h |   21 ++++++++++++++++-----
>  kernel/events/uprobes.c |   16 ++++------------
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 319eae7..366b8b2 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -26,6 +26,7 @@
>  
>  #include <linux/errno.h>
>  #include <linux/rbtree.h>
> +#include <linux/types.h>
>  
>  struct vm_area_struct;
>  struct mm_struct;
> @@ -72,14 +73,24 @@ enum uprobe_task_state {
>   */
>  struct uprobe_task {
>  	enum uprobe_task_state		state;
> -	struct arch_uprobe_task		autask;
>  
> -	struct return_instance		*return_instances;
> -	unsigned int			depth;
> -	struct uprobe			*active_uprobe;
> +	union {
> +		struct {
> +			struct arch_uprobe_task	autask;
> +			unsigned long		vaddr;
> +		};
> +
> +		struct {
> +			struct callback_head	dup_work;
> +			unsigned long		dup_addr;
> +		};
> +	};
>  
> +	struct uprobe			*active_uprobe;
>  	unsigned long			xol_vaddr;
> -	unsigned long			vaddr;
> +
> +	struct return_instance		*return_instances;
> +	unsigned int			depth;
>  };
>  
>  /*
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 24b7d6c..2546a7b 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1403,12 +1403,10 @@ static void uprobe_warn(struct task_struct *t, const char *msg)
>  
>  static void dup_xol_work(struct callback_head *work)
>  {
> -	kfree(work);
> -
>  	if (current->flags & PF_EXITING)
>  		return;
>  
> -	if (!__create_xol_area(current->utask->vaddr))
> +	if (!__create_xol_area(current->utask->dup_addr))
>  		uprobe_warn(current, "dup xol area");
>  }
>  
> @@ -1419,7 +1417,6 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
>  {
>  	struct uprobe_task *utask = current->utask;
>  	struct mm_struct *mm = current->mm;
> -	struct callback_head *work;
>  	struct xol_area *area;
>  
>  	t->utask = NULL;
> @@ -1441,14 +1438,9 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
>  	if (mm == t->mm)
>  		return;
>  
> -	/* TODO: move it into the union in uprobe_task */
> -	work = kmalloc(sizeof(*work), GFP_KERNEL);
> -	if (!work)
> -		return uprobe_warn(t, "dup xol area");
> -
> -	t->utask->vaddr = area->vaddr;
> -	init_task_work(work, dup_xol_work);
> -	task_work_add(t, work, true);
> +	t->utask->dup_addr = area->vaddr;
> +	init_task_work(&t->utask->dup_work, dup_xol_work);
> +	task_work_add(t, &t->utask->dup_work, true);
>  }
>  
>  /*
> 


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

Re: [PATCH] uprobes: Add uprobe_task->dup_work/dup_addr

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-11-08 20:00:03]:

> uprobe_task->vaddr is a bit strange. First of all it is not really
> needed, we can move it into arch_uprobe_task. The generic code uses
> it only to pass the additional argument to arch_uprobe_pre_xol(),
> and since it is always equal to instruction_pointer() this looks
> even more strange.
> 

While the code changes look good, I would disagree with the above
statement.  uprobe_task->vaddr is a bit strange only in
uprobe_copy_process() and not in arch_uprobe_pre_xol() context.
uprobe_task->vaddr was used to refer to the actual instruction pointer
and do the necessary fixups after single stepping out of line.

The casual reading of this commit message, one can get an impression
that vaddr is never needed.  Your change still retains it. So can we
modify the commit message accordingly? 

> And both utask->vaddr and and utask->autask have the same scope,
> they only have the meaning when the task executes the probed insn
> out-of-line. This means it is safe to reuse both in UTASK_RUNNING
> state.
> 
> OTOH, it is also used by uprobe_copy_process() and dup_xol_work()
> for another purpose, this doesn't look clean and doesn't allow to
> move this member into arch_uprobe_task.
> 
> This patch adds the union with 2 anonymous structs into uprobe_task.
> 
> The first struct is autask + vaddr, this way we "almost" move vaddr
> into autask.
> 
> The second struct has 2 new members for uprobe_copy_process() paths:
> ->dup_addr which can be used instead ->vaddr, and ->dup_work which
> can be used to avoid kmalloc() and simplify the code.
> 
> Note that this union will likely have another member(s), we need
> something like "private_data_for_handlers" so that the tracing
> handlers could use it to communicate with call_fetch() methods.

One nit below + request for change in the above commit message. 
Otherwise 
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>

> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  include/linux/uprobes.h |   21 ++++++++++++++++-----
>  kernel/events/uprobes.c |   16 ++++------------
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 319eae7..366b8b2 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -26,6 +26,7 @@
> 
>  #include <linux/errno.h>
>  #include <linux/rbtree.h>
> +#include <linux/types.h>
> 
>  struct vm_area_struct;
>  struct mm_struct;
> @@ -72,14 +73,24 @@ enum uprobe_task_state {
>   */
>  struct uprobe_task {
>  	enum uprobe_task_state		state;
> -	struct arch_uprobe_task		autask;
> 
> -	struct return_instance		*return_instances;
> -	unsigned int			depth;
> -	struct uprobe			*active_uprobe;
> +	union {
> +		struct {
> +			struct arch_uprobe_task	autask;
> +			unsigned long		vaddr;
> +		};
> +
> +		struct {
> +			struct callback_head	dup_work;
> +			unsigned long		dup_addr;

Nit:
Can we rename dup_addr to mean that it refers to the xol; something like
dup_xol_addr or even xol_addr. So that its more clear what address it
refers to.


-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH] uprobes: Add uprobe_task->dup_work/dup_addr

[Oleg Nesterov]

On 11/11, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2013-11-08 20:00:03]:
>
> > uprobe_task->vaddr is a bit strange. First of all it is not really
> > needed, we can move it into arch_uprobe_task. The generic code uses
> > it only to pass the additional argument to arch_uprobe_pre_xol(),
> > and since it is always equal to instruction_pointer() this looks
> > even more strange.
> >
>
> While the code changes look good, I would disagree with the above
> statement.  uprobe_task->vaddr is a bit strange only in
> uprobe_copy_process() and not in arch_uprobe_pre_xol() context.
> uprobe_task->vaddr was used to refer to the actual instruction pointer

Yes, and it is always equal to regs->ip when pre_ssout() is called,

> and do the necessary fixups after single stepping out of line.

Exactly. So it is write-only (and meaningless) to the generic uprobe
code. We can (and perhaps should) move it into autask->saved_vaddr,
arch_uprobe_pre_xol() can initialize it.

> The casual reading of this commit message, one can get an impression
> that vaddr is never needed.

See above. The changelog doesn't say we can simply remove it, it says
"move it".

> Your change still retains it.

Of course we can't kill utas->vaddr until we change x86/powerpc. And
just in case, of course I understand that this is minor and even
subjective.

But at least this patch logically "joins" autask and vaddr, and I
believe this is good because they are always used together, because,
well, logically vaddr belongs to autask ;)

> So can we
> modify the commit message accordingly?

Well, I'll try... but perhaps you can help? I mean, I am not sure
about how I can improve it. Could you suggest a better wording?

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

Thanks!

> > @@ -72,14 +73,24 @@ enum uprobe_task_state {
> >   */
> >  struct uprobe_task {
> >  	enum uprobe_task_state		state;
> > -	struct arch_uprobe_task		autask;
> >
> > -	struct return_instance		*return_instances;
> > -	unsigned int			depth;
> > -	struct uprobe			*active_uprobe;
> > +	union {
> > +		struct {
> > +			struct arch_uprobe_task	autask;
> > +			unsigned long		vaddr;
> > +		};
> > +
> > +		struct {
> > +			struct callback_head	dup_work;
> > +			unsigned long		dup_addr;
>
> Nit:
> Can we rename dup_addr to mean that it refers to the xol; something like
> dup_xol_addr or even xol_addr. So that its more clear what address it
> refers to.

OK. How about dup_xol_work/dup_xol_vaddr ?

Oleg.

Re: [PATCH] uprobes: Add uprobe_task->dup_work/dup_addr

[Oleg Nesterov]

On 11/11, Oleg Nesterov wrote:
>
> On 11/11, Srikar Dronamraju wrote:
> >
> > Nit:
> > Can we rename dup_addr to mean that it refers to the xol; something like
> > dup_xol_addr or even xol_addr. So that its more clear what address it
> > refers to.
>
> OK. How about dup_xol_work/dup_xol_vaddr ?

I didn't notice you suggested the same naming ;) OK, will rename to
dup_xol_work/addr.

Oleg.

Re: [PATCH] uprobes: Add uprobe_task->dup_work/dup_addr

[Srikar Dronamraju]

> Yes, and it is always equal to regs->ip when pre_ssout() is called,
> 
> > and do the necessary fixups after single stepping out of line.
> 
> Exactly. So it is write-only (and meaningless) to the generic uprobe
> code. We can (and perhaps should) move it into autask->saved_vaddr,
> arch_uprobe_pre_xol() can initialize it.
> 
> > The casual reading of this commit message, one can get an impression
> > that vaddr is never needed.
> 
> See above. The changelog doesn't say we can simply remove it, it says
> "move it".


Okay, moving to arch_uprobe_task is fine. I probably got confused by 
"First of all it is not really needed," 
> 
> > Your change still retains it.
> 
> 
> OK. How about dup_xol_work/dup_xol_vaddr ?
> 

Yes fine with me.

-- 
Thanks and Regards
Srikar Dronamraju

[PATCH v2] uprobes: Add uprobe_task->dup_work/dup_addr

[Oleg Nesterov]

On 11/12, Srikar Dronamraju wrote:
>
> Okay, moving to arch_uprobe_task is fine. I probably got confused by
> "First of all it is not really needed,"

OK, this doesn't look good, I agree.

Please see v2 below, I tried to improve the changelog.

> > OK. How about dup_xol_work/dup_xol_vaddr ?
>
> Yes fine with me.

Done.

I added your ack optimistically, please let me know if you think I
should change something else.

Oleg.
---

Subject: [PATCH] uprobes: Add uprobe_task->dup_xol_work/dup_xol_addr

uprobe_task->vaddr is a bit strange. The generic code uses it only
to pass the additional argument to arch_uprobe_pre_xol(), and since
it is always equal to instruction_pointer() this looks even more
strange.

And both utask->vaddr and and utask->autask have the same scope,
they only have the meaning when the task executes the probed insn
out-of-line, so it is safe to reuse both in UTASK_RUNNING state.

This all means that logically ->vaddr belongs to arch_uprobe_task
and we should probably move it there, arch_uprobe_pre_xol() can
record instruction_pointer() itself.

OTOH, it is also used by uprobe_copy_process() and dup_xol_work()
for another purpose, this doesn't look clean and doesn't allow to
move this member into arch_uprobe_task.

This patch adds the union with 2 anonymous structs into uprobe_task.

The first struct is autask + vaddr, this way we "almost" move vaddr
into autask.

The second struct has 2 new members for uprobe_copy_process() paths:
->dup_xol_addr which can be used instead ->vaddr, and ->dup_xol_work
which can be used to avoid kmalloc() and simplify the code.

Note that this union will likely have another member(s), we need
something like "private_data_for_handlers" so that the tracing
handlers could use it to communicate with call_fetch() methods.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 include/linux/uprobes.h |   21 ++++++++++++++++-----
 kernel/events/uprobes.c |   16 ++++------------
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 319eae7..2225542 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -26,6 +26,7 @@
 
 #include <linux/errno.h>
 #include <linux/rbtree.h>
+#include <linux/types.h>
 
 struct vm_area_struct;
 struct mm_struct;
@@ -72,14 +73,24 @@ enum uprobe_task_state {
  */
 struct uprobe_task {
 	enum uprobe_task_state		state;
-	struct arch_uprobe_task		autask;
 
-	struct return_instance		*return_instances;
-	unsigned int			depth;
-	struct uprobe			*active_uprobe;
+	union {
+		struct {
+			struct arch_uprobe_task	autask;
+			unsigned long		vaddr;
+		};
+
+		struct {
+			struct callback_head	dup_xol_work;
+			unsigned long		dup_xol_addr;
+		};
+	};
 
+	struct uprobe			*active_uprobe;
 	unsigned long			xol_vaddr;
-	unsigned long			vaddr;
+
+	struct return_instance		*return_instances;
+	unsigned int			depth;
 };
 
 /*
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 24b7d6c..df4ef09 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1403,12 +1403,10 @@ static void uprobe_warn(struct task_struct *t, const char *msg)
 
 static void dup_xol_work(struct callback_head *work)
 {
-	kfree(work);
-
 	if (current->flags & PF_EXITING)
 		return;
 
-	if (!__create_xol_area(current->utask->vaddr))
+	if (!__create_xol_area(current->utask->dup_xol_addr))
 		uprobe_warn(current, "dup xol area");
 }
 
@@ -1419,7 +1417,6 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
 {
 	struct uprobe_task *utask = current->utask;
 	struct mm_struct *mm = current->mm;
-	struct callback_head *work;
 	struct xol_area *area;
 
 	t->utask = NULL;
@@ -1441,14 +1438,9 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
 	if (mm == t->mm)
 		return;
 
-	/* TODO: move it into the union in uprobe_task */
-	work = kmalloc(sizeof(*work), GFP_KERNEL);
-	if (!work)
-		return uprobe_warn(t, "dup xol area");
-
-	t->utask->vaddr = area->vaddr;
-	init_task_work(work, dup_xol_work);
-	task_work_add(t, work, true);
+	t->utask->dup_xol_addr = area->vaddr;
+	init_task_work(&t->utask->dup_xol_work, dup_xol_work);
+	task_work_add(t, &t->utask->dup_xol_work, true);
 }
 
 /*
-- 
1.5.5.1

Re: [PATCH v2] uprobes: Add uprobe_task->dup_work/dup_addr

[Srikar Dronamraju]

* Oleg Nesterov <oleg@redhat.com> [2013-11-12 20:20:38]:

> On 11/12, Srikar Dronamraju wrote:
> >
> > Okay, moving to arch_uprobe_task is fine. I probably got confused by
> > "First of all it is not really needed,"
> 
> OK, this doesn't look good, I agree.
> 
> Please see v2 below, I tried to improve the changelog.
> 
> > > OK. How about dup_xol_work/dup_xol_vaddr ?
> >
> > Yes fine with me.
> 
> Done.
> 
> I added your ack optimistically, please let me know if you think I
> should change something else.

Looks nice to me.

-- 
Thanks and Regards
Srikar Dronamraju

Re: [PATCH v2] uprobes: Add uprobe_task->dup_work/dup_addr

[Masami Hiramatsu]

Ping?

Is this already pulled?
I think it is enough discussed and reviewed.

Thank you,

(2013/11/13 4:20), Oleg Nesterov wrote:
> On 11/12, Srikar Dronamraju wrote:
>>
>> Okay, moving to arch_uprobe_task is fine. I probably got confused by
>> "First of all it is not really needed,"
> 
> OK, this doesn't look good, I agree.
> 
> Please see v2 below, I tried to improve the changelog.
> 
>>> OK. How about dup_xol_work/dup_xol_vaddr ?
>>
>> Yes fine with me.
> 
> Done.
> 
> I added your ack optimistically, please let me know if you think I
> should change something else.
> 
> Oleg.
> ---
> 
> Subject: [PATCH] uprobes: Add uprobe_task->dup_xol_work/dup_xol_addr
> 
> uprobe_task->vaddr is a bit strange. The generic code uses it only
> to pass the additional argument to arch_uprobe_pre_xol(), and since
> it is always equal to instruction_pointer() this looks even more
> strange.
> 
> And both utask->vaddr and and utask->autask have the same scope,
> they only have the meaning when the task executes the probed insn
> out-of-line, so it is safe to reuse both in UTASK_RUNNING state.
> 
> This all means that logically ->vaddr belongs to arch_uprobe_task
> and we should probably move it there, arch_uprobe_pre_xol() can
> record instruction_pointer() itself.
> 
> OTOH, it is also used by uprobe_copy_process() and dup_xol_work()
> for another purpose, this doesn't look clean and doesn't allow to
> move this member into arch_uprobe_task.
> 
> This patch adds the union with 2 anonymous structs into uprobe_task.
> 
> The first struct is autask + vaddr, this way we "almost" move vaddr
> into autask.
> 
> The second struct has 2 new members for uprobe_copy_process() paths:
> ->dup_xol_addr which can be used instead ->vaddr, and ->dup_xol_work
> which can be used to avoid kmalloc() and simplify the code.
> 
> Note that this union will likely have another member(s), we need
> something like "private_data_for_handlers" so that the tracing
> handlers could use it to communicate with call_fetch() methods.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  include/linux/uprobes.h |   21 ++++++++++++++++-----
>  kernel/events/uprobes.c |   16 ++++------------
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
> index 319eae7..2225542 100644
> --- a/include/linux/uprobes.h
> +++ b/include/linux/uprobes.h
> @@ -26,6 +26,7 @@
>  
>  #include <linux/errno.h>
>  #include <linux/rbtree.h>
> +#include <linux/types.h>
>  
>  struct vm_area_struct;
>  struct mm_struct;
> @@ -72,14 +73,24 @@ enum uprobe_task_state {
>   */
>  struct uprobe_task {
>  	enum uprobe_task_state		state;
> -	struct arch_uprobe_task		autask;
>  
> -	struct return_instance		*return_instances;
> -	unsigned int			depth;
> -	struct uprobe			*active_uprobe;
> +	union {
> +		struct {
> +			struct arch_uprobe_task	autask;
> +			unsigned long		vaddr;
> +		};
> +
> +		struct {
> +			struct callback_head	dup_xol_work;
> +			unsigned long		dup_xol_addr;
> +		};
> +	};
>  
> +	struct uprobe			*active_uprobe;
>  	unsigned long			xol_vaddr;
> -	unsigned long			vaddr;
> +
> +	struct return_instance		*return_instances;
> +	unsigned int			depth;
>  };
>  
>  /*
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 24b7d6c..df4ef09 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1403,12 +1403,10 @@ static void uprobe_warn(struct task_struct *t, const char *msg)
>  
>  static void dup_xol_work(struct callback_head *work)
>  {
> -	kfree(work);
> -
>  	if (current->flags & PF_EXITING)
>  		return;
>  
> -	if (!__create_xol_area(current->utask->vaddr))
> +	if (!__create_xol_area(current->utask->dup_xol_addr))
>  		uprobe_warn(current, "dup xol area");
>  }
>  
> @@ -1419,7 +1417,6 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
>  {
>  	struct uprobe_task *utask = current->utask;
>  	struct mm_struct *mm = current->mm;
> -	struct callback_head *work;
>  	struct xol_area *area;
>  
>  	t->utask = NULL;
> @@ -1441,14 +1438,9 @@ void uprobe_copy_process(struct task_struct *t, unsigned long flags)
>  	if (mm == t->mm)
>  		return;
>  
> -	/* TODO: move it into the union in uprobe_task */
> -	work = kmalloc(sizeof(*work), GFP_KERNEL);
> -	if (!work)
> -		return uprobe_warn(t, "dup xol area");
> -
> -	t->utask->vaddr = area->vaddr;
> -	init_task_work(work, dup_xol_work);
> -	task_work_add(t, work, true);
> +	t->utask->dup_xol_addr = area->vaddr;
> +	init_task_work(&t->utask->dup_xol_work, dup_xol_work);
> +	task_work_add(t, &t->utask->dup_xol_work, true);
>  }
>  
>  /*
> 


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

Re: [PATCH v2] uprobes: Add uprobe_task->dup_work/dup_addr

[Oleg Nesterov]

On 11/24, Masami Hiramatsu wrote:
>
> Ping?
>
> Is this already pulled?
> I think it is enough discussed and reviewed.

Yes, thanks, this is already in tip/perf/core.

Oleg.