(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...@hitachi.com -- 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/