Messages in this thread |  | | Date | Sat, 09 Nov 2013 12:18:47 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCHSET 00/13] tracing/uprobes: Add support for more fetch methods (v6) |
| |
(2013/11/07 17:48), Namhyung Kim wrote: > On Wed, 6 Nov 2013 18:37:54 +0100, Oleg Nesterov wrote: >> On 11/06, Namhyung Kim wrote: >>> >>> On Tue, 5 Nov 2013 20:24:01 +0100, Oleg Nesterov wrote: >>>> On 11/05, Oleg Nesterov wrote: >>>>> >>>>> As for "-= tu->offset"... Can't we avoid it? User-space needs to calculate >>>>> the "@" argument anyway, why it can't also substruct this offset? >>>>> >>>>> Or perhaps we can change parse_probe_arg("@") to update "param" ? Yes, >>>>> in this case it needs another argument, not sure... >>>> >>>> Or, >>>> >>>>> + if (is_ret_probe(tu)) { >>>>> + saved_ip = instruction_pointer(regs); >>>>> + instruction_pointer_set(func); >>>>> + } >>>>> store_trace_args(...); >>>>> + if (is_ret_probe(tu)) >>>>> + instruction_pointer_set(saved_ip); >>>> >>>> we can put "-= tu->offset" here. >>> >>> I don't think I get the point. >> >> I meant, >> >> saved_ip = instruction_pointer(regs); >> >> // pass the "ip" which was used to calculate >> // the @addr argument to fetch_*() methods >> >> temp_ip = is_ret_probe(tu) ? func : saved_ip; >> temp_ip -= tu->offset; >> instruction_pointer_set(temp_ip); >> >> store_trace_args(...); >> >> instruction_pointer_set(saved_ip); >> >> This way we can avoid the new "void *" argument for fetch_func_t, >> we do not need it to calculate the address. > > Okay, but as I said before, subtracting tu->offset part can be removed.
Ah, that's good to me too :)
>> >> However, now I think it would be more clean to leave FETCH_MTD_memory >> alone and add FETCH_MTD_memory_dotranslate instead. >> >> So trace_uprobes.c should define >> >> void FETCH_FUNC_NAME(memory, type)(addr, ...) >> { >> copy_from_user((void __user *)addr); >> } >> >> void FETCH_FUNC_NAME(memory_dotranslate, type)(addr, ...) >> { >> void __user *uaddr = get_user_vaddr(regs, addr); >> copy_from_user(uaddr); >> } > > Looks good. > >> >> Then, >> >>>> Or. Perhaps we can leave "case '@'" in parse_probe_arg() and >>>> FETCH_MTD_memory alone. You seem to agree that "absolute address" >>>> can be useful anyway. >>> >>> Yes, but it's only meaningful to process-wide tracing sessions IMHO. >> >> Yes, yes, sure. >> >> I meant, we need both. Say, "perf probe "func global=@addr" means >> FETCH_MTD_memory, and "perf probe "func global=*addr" means >> FETCH_MTD_memory_dotranslate. >> >> Just in case, of course I do not care about the syntax, for example we >> can use "@~addr" for translate (or not translate) or whatever. > > Yeah, and I want to hear from Masami.
Hm, this part I need to clarify. So you mean the @addr is for referring the absolute address in a user process, but @~addr is for referring the relative address of a executable or library, correct? In that case, I suggest you to use "@+addr" for the relative address, since that is an offset, isn't that? :)
BTW, it seems that @addr syntax is hard to use for uprobes, because current uprobes is based on a binary, not a process, we cannot specify which process is probed when we define it.
>> My only point: I think we need both to >> >> 1. avoid the new argument in fetch_func_t >> >> 2. allow the dump the data from the absolute address
Looks good to me :)
Thank you!
-- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com
|  |