lkml.org 
[lkml]   [2013]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] uprobes: Add uprobe_task->dup_work/dup_addr
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.



\
 
 \ /
  Last update: 2013-11-10 18:41    [W:0.122 / U:0.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site