Message-ID: <5458FD56.8040304@hitachi.com>
Date:	Wed, 05 Nov 2014 01:22:46 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:	srikar@...ux.vnet.ibm.com, Peter Zijlstra <peterz@...radead.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Brendan Gregg <brendan.gregg@...il.com>,
	yrl.pp-manager.tt@...achi.com, namhyung@...nel.org,
	Hemant Kumar <hemant@...ux.vnet.ibm.com>,
	Ingo Molnar <mingo@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [PATCH perf/core 0/6] perf-probe: Bugfix and add new options
 for cache

(2014/11/04 23:38), Arnaldo Carvalho de Melo wrote:
> Em Tue, Nov 04, 2014 at 01:36:31PM +0900, Masami Hiramatsu escreveu:
>> (2014/11/04 1:19), Arnaldo Carvalho de Melo wrote:
>>> Em Mon, Nov 03, 2014 at 09:11:18PM +0900, Masami Hiramatsu escreveu:
>>>> (2014/10/31 21:13), Arnaldo Carvalho de Melo wrote:
>>>>> Em Fri, Oct 31, 2014 at 02:51:29PM -0400, Masami Hiramatsu escreveu:
>>>> Actually, kprobe event itself can reject command if the given address
>>>> is not in the kernel text nor instruction boundary (perhaps, uprobes
>>>> may have a problem...), so for the kernel level, it is safe.
> 
>>> No, it is not necessarily safe.
> 
>>> What if you specify function foo() that has address 0x1234 for kernel
>>> v3.16 and then run the cached probe on kernel v3.18 and on that kernel
>>> the function foo() maps to address 0x2345 and function bar() instead
>>> maps to address 0x1234? Oops.
>  
>> In that case user just trace bar() instead of foo(). Of course it's
>> not correct, but shouldn't break the kernel (if the kernel is broken,
>> it is a bug of kprobes).
> 
> I.e. no crashes, just misleading information :-\

Right.

>>> The build-id was designed to uniquely identify a DSO, we need to use it.
> 
>>> I think that at some point not using it should be left to a, in
>>> systemtap parlance, "guru" mode, with tooling warning profusely when
>>> build ids are not available and requiring even more forcing when it
>>> doesn't matches.
>  
>> But it is not necessarily everyone uses perf probe to set up the probe
>> events(because it is a part of ftrace), as we can see in the Brendan's
>> scripts.
> 
> Right, If I implied that some particular tool should be used, sorry
> about that, what I wanted to get accross was that the information that
> allows users or tools to make sure there is no mismatch between the
> cached probes and the target kernel is collected at cached probe
> creating time and available at target use time.

Yes, and if user setting probes via perf, the perf must ensure that it
picks up the correct cache by using build-id. If someone wants to use
other tools, he/she must ensure it. We just give a information how to
check that :)

> 
>> I think, at least what we need is clarifying how can they ensure
>> build-id before setting the probe events. I'd like to give them options
>> with knowledge instead of forcing by tools.
> 
> Right, so we need to have the build-id as part of the cache format,
> perhaps as the first line, starting with a comment (#), that way the
> user can use whatever way he has at its disposal to check that the
> running kernel build-id is the same as the one on that comment. Using
> that script you provided, that uses just things that are on the machine
> (od, /sys/kernel/notes).


Ah, that's a good idea :)
So, with such build-id comment line, would you think we can have an
--output option? Or we'd better moving onto the .debug/ cache file?

What I'm thinking about this feature is to make a compact and reduced
function-entry level probe cache while building the kernel (as a part
of kbuild), so that we can deploy the stripped kernel and the cache
to remote machines.

[snip]
>> OK, I agree using .debug/.buildid/ to store caches.
>> Here is what I'm thinking,
>  
>>   # this makes caches for given pattern instead of adding probes.
>>   perf probe --cache '* $params'
> 
>>   # the cache is stored in .debug/.buildid/<buildid>.probe
>>   # the cache entry can be queried by buildid and eventname
> 
> To follow the existing standard this would instead go to:
> 
>>   # the cache is stored in .debug/probes/path/to/dso/name/buildid
>>   # And can be found via its buildid link .debug/.buildid/bu/ildid -> ../../probes/path/to/dso/name/buildid

Ah, I see. so you meant adding a top-level .debug/probes/ dir.
But in that case, shouldn't we change .debug/.buildid/bu/ildid to
.debug/probes/.buildid/bu/ildid ?

> 
>>   perf probe --query ${remote_buildid}:do_fork
>>   p:probe/do_fork _text+298722 clone_flags=%di:u64 stack_start=%si:u64 stack_size=%dx:u64 parent_tidptr=%cx:u64 child_tidptr=%r8:u64
> 
>>   # or perf can set it up directly to local
>>   perf probe --query-add do_fork
> 
> You missed the build id above, no? I.e. it would be:
> 
>>   # or perf can set it up directly to local
>>   perf probe --query-add ${remote_buildid}:do_fork

No, since this command set the event to local machine, perf-probe
should check the local build-id and query the appropriate event
from the cache.
# BTW, maybe we'd better use perf probe --add '$do_fork' (calls
# "cache of do_fork")  instead of long --query-add. :)

> 
> Right? and that would be the equivalent to:
> 
>  perf probe --query ${remote_buildid}:do_fork > /sys/kernel/debug/tracing/kprobe_events
> 
> No?
> 
> And do you intend to generate that script above that checks the build
> id, tees into /sys/kernel/debug/tracing/kprobe_events, etc as part of
> this process?

No, not yet. But that sounds nice :) Thanks!

> 
> I.e. the process would be, as you describe above plus telling the user
> that he/she then should unconpress the script and run it.
> 
> Scripts like Brendan's would do whatever they want with it of course,
> but the default end result of 'perf probe' would be usable straight away
> and doing the build id checking without requiring any extra tooling to
> be available at the target machine.

Agreed, with .debug/ archive, perf-probe should pick and set event
correctly without any other tools.

Thank you!

> 
>>   Added new event:
>>     probe:do_fork        (on do_fork with clone_flags satck_start stack_size parent_tidptr child_tidptr)
> 
>>   You can now use it in all perf tools, such as:
> 
>>         perf record -e probe:do_fork -aR sleep 1
> 
>> What would you think about this? :)
> 
> Cool feature! :-)
> 
>>>>> Then, later, one would use 'perf archive' passing some keys (or a
>>>>> perf.data file, like done nowadays to pick the files in ~/.debug for
>>>>> dsos that had hits on the specified perf.data file) to get the cached
>>>>> values to use on some other machine, to avoid having to use the
>>>>> debuginfo files there.
> 
>>>> Yeah, querying it from the BUILDID database by using a pair of remote
>>>> build-id and the binary path is a good feature.
> 
>>>>> I.e. in summary I think that the format is ok, but we need to have this
>>>>> inside the ~/.debug hierarchy so that we can make sure that we use the
>>>>> right probe definition, one that matches the DSOs being used (the kernel
>>>>> or some other userspace binary).
>>>
>>>> OK, perhaps, that is also good to SDT series at last.
>>>
>>> Sure thing!
>>>
>>
>> Thank you!
> 
> You're welcome!
> 
> - Arnaldo
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists