Re: [PATCHSET 00/13] tracing/uprobes: Add support for more fetch methods (v6)

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(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