lkml.org 
[lkml]   [2013]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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




\
 
 \ /
  Last update: 2013-11-09 04:41    [W:1.202 / U:0.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site