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

[Oleg Nesterov <oleg@redhat.com>]

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.