Messages in this thread | | | Date | Mon, 11 Nov 2013 17:55:43 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH] uprobes: Add uprobe_task->dup_work/dup_addr |
| |
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.
| |