On 11/11, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <o...@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 <sri...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
