Re: [RFC] perf/sdt: Directly record SDT event with 'perf record'
Em Thu, Feb 23, 2017 at 01:43:38PM +0530, Ravi Bangoria escreveu: > Thanks Arnaldo, > > I'm working on this but it's taking bit longer time. Will post out a patch > within > few days. Take your time and thanks for giving consideration to my observations, Regards, - Arnaldo > Ravi > > On Monday 20 February 2017 07:41 PM, Arnaldo Carvalho de Melo wrote: > > Em Mon, Feb 20, 2017 at 04:31:50PM +0530, Ravi Bangoria escreveu: > >> Thanks Ingo, > >> > >> On Monday 20 February 2017 02:12 PM, Ingo Molnar wrote: > >>> * Ravi Bangoria wrote: > >>> > What should be the behavior of the tool? Should it record only one > 'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it > should record all the SDT events from libpthread? We can choose either > of two but both the cases are ambiguous. > >>> They are not ambiguous really if coded right: just pick one of the > >>> outcomes and > >>> maybe print a warning to inform the user that something weird is going on > >>> because > >>> not all markers are enabled? > >>> > >>> As a user I'd expect 'perf record' to enable all markers and print a > >>> warning that > >>> the markers were in a partial state. This would result in consistent > >>> behaviour. > >> Yes, makes sense. > >> > >>> Does it make sense to only enable some of the markers that alias on the > >>> same name? > >>> If not then maybe disallow that in perf probe - or change perf probe to > >>> do the > >>> same thing as perf record. > >> 'perf probe' is doing that correctly. It fetches all events with given > >> name from > >> probe-cache and creates entries for them in uprobe_events. > >> > >> The problem is the 2-step process of adding probes and then recording, > >> allowing users to select individual markers to record on. > > So, the more streamlined one works for most people, i.e. just use perf > > record, no need to perf probe anything. But, for people who "know what > > they are doing", perf probe can be used first to control exactly which > > SDT probes one wants in place, and then those will be used. > > > > We need to make sure that when processing the file there is information > > that says which probes were in place and enabled in the record session, > > tho. Is that possible? > > > >>> I.e. this is IMHO an artificial problem that users should not be exposed > >>> to and > >>> which can be solved by tooling. > >>> > >>> In particular if it's possible to enable only a part of the markers then > >>> perf > >>> record not continuing would be a failure mode: if for example a previous > >>> perf > >>> record session segfaulted (or ran out of RAM or was killed in the wrong > >>> moment or > >>> whatever) then it would not be possible to (easily) clean up the mess. > >> Agreed. We need to make this more robust. > > Right, disambiguating a 'probes left by a session that did auto-probing' > > from a 'hey, those probes are there intentionally, just use those' is > > important. > > > Not allowing 'perf probe' for SDT event will solve all such issues. > Also it will make user interface simple and consistent. Other current > tooling (systemtap, for instance) also do not allow probing individual > markers when there are multiple markers with the same name. > >>> In any case if others agree with your change in UI flow too then it's > >>> fine by me, > >>> but please make it robust, i.e. if perf record sees partially enabled > >>> probes it > >>> should still continue. > >> @Masami, can you please provide your thoughts as well. > > Yeah, if technically possible to allow both variants, we should leave > > it up to users to decide what is best? > > > > I.e. most people will do auto-probing, not using 'perf probe' at all, > > documentation should state the pitfalls in doing so. > > > > So, after writing the above, perhaps we should warn the user that > > pre-existing probes are being used, as this will be the odd case? > > > > The normal flow will be just using perf record with SDT probes, that > > will auto-probe them and remove on exit, or better drop a reference to > > them, as simultaneous use also needs to be covered? > > > > - Arnaldo > >
Re: [RFC] perf/sdt: Directly record SDT event with 'perf record'
Thanks Arnaldo, I'm working on this but it's taking bit longer time. Will post out a patch within few days. Ravi On Monday 20 February 2017 07:41 PM, Arnaldo Carvalho de Melo wrote: > Em Mon, Feb 20, 2017 at 04:31:50PM +0530, Ravi Bangoria escreveu: >> Thanks Ingo, >> >> On Monday 20 February 2017 02:12 PM, Ingo Molnar wrote: >>> * Ravi Bangoria wrote: >>> What should be the behavior of the tool? Should it record only one 'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it should record all the SDT events from libpthread? We can choose either of two but both the cases are ambiguous. >>> They are not ambiguous really if coded right: just pick one of the outcomes >>> and >>> maybe print a warning to inform the user that something weird is going on >>> because >>> not all markers are enabled? >>> >>> As a user I'd expect 'perf record' to enable all markers and print a >>> warning that >>> the markers were in a partial state. This would result in consistent >>> behaviour. >> Yes, makes sense. >> >>> Does it make sense to only enable some of the markers that alias on the >>> same name? >>> If not then maybe disallow that in perf probe - or change perf probe to do >>> the >>> same thing as perf record. >> 'perf probe' is doing that correctly. It fetches all events with given name >> from >> probe-cache and creates entries for them in uprobe_events. >> >> The problem is the 2-step process of adding probes and then recording, >> allowing users to select individual markers to record on. > So, the more streamlined one works for most people, i.e. just use perf > record, no need to perf probe anything. But, for people who "know what > they are doing", perf probe can be used first to control exactly which > SDT probes one wants in place, and then those will be used. > > We need to make sure that when processing the file there is information > that says which probes were in place and enabled in the record session, > tho. Is that possible? > >>> I.e. this is IMHO an artificial problem that users should not be exposed to >>> and >>> which can be solved by tooling. >>> >>> In particular if it's possible to enable only a part of the markers then >>> perf >>> record not continuing would be a failure mode: if for example a previous >>> perf >>> record session segfaulted (or ran out of RAM or was killed in the wrong >>> moment or >>> whatever) then it would not be possible to (easily) clean up the mess. >> Agreed. We need to make this more robust. > Right, disambiguating a 'probes left by a session that did auto-probing' > from a 'hey, those probes are there intentionally, just use those' is > important. > Not allowing 'perf probe' for SDT event will solve all such issues. Also it will make user interface simple and consistent. Other current tooling (systemtap, for instance) also do not allow probing individual markers when there are multiple markers with the same name. >>> In any case if others agree with your change in UI flow too then it's fine >>> by me, >>> but please make it robust, i.e. if perf record sees partially enabled >>> probes it >>> should still continue. >> @Masami, can you please provide your thoughts as well. > Yeah, if technically possible to allow both variants, we should leave > it up to users to decide what is best? > > I.e. most people will do auto-probing, not using 'perf probe' at all, > documentation should state the pitfalls in doing so. > > So, after writing the above, perhaps we should warn the user that > pre-existing probes are being used, as this will be the odd case? > > The normal flow will be just using perf record with SDT probes, that > will auto-probe them and remove on exit, or better drop a reference to > them, as simultaneous use also needs to be covered? > > - Arnaldo >
Re: [RFC] perf/sdt: Directly record SDT event with 'perf record'
Em Mon, Feb 20, 2017 at 04:31:50PM +0530, Ravi Bangoria escreveu: > Thanks Ingo, > > On Monday 20 February 2017 02:12 PM, Ingo Molnar wrote: > > * Ravi Bangoria wrote: > > > >> What should be the behavior of the tool? Should it record only one > >> 'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it > >> should record all the SDT events from libpthread? We can choose either > >> of two but both the cases are ambiguous. > > They are not ambiguous really if coded right: just pick one of the outcomes > > and > > maybe print a warning to inform the user that something weird is going on > > because > > not all markers are enabled? > > > > As a user I'd expect 'perf record' to enable all markers and print a > > warning that > > the markers were in a partial state. This would result in consistent > > behaviour. > > Yes, makes sense. > > > Does it make sense to only enable some of the markers that alias on the > > same name? > > If not then maybe disallow that in perf probe - or change perf probe to do > > the > > same thing as perf record. > > 'perf probe' is doing that correctly. It fetches all events with given name > from > probe-cache and creates entries for them in uprobe_events. > > The problem is the 2-step process of adding probes and then recording, > allowing users to select individual markers to record on. So, the more streamlined one works for most people, i.e. just use perf record, no need to perf probe anything. But, for people who "know what they are doing", perf probe can be used first to control exactly which SDT probes one wants in place, and then those will be used. We need to make sure that when processing the file there is information that says which probes were in place and enabled in the record session, tho. Is that possible? > > I.e. this is IMHO an artificial problem that users should not be exposed to > > and > > which can be solved by tooling. > > > > In particular if it's possible to enable only a part of the markers then > > perf > > record not continuing would be a failure mode: if for example a previous > > perf > > record session segfaulted (or ran out of RAM or was killed in the wrong > > moment or > > whatever) then it would not be possible to (easily) clean up the mess. > Agreed. We need to make this more robust. Right, disambiguating a 'probes left by a session that did auto-probing' from a 'hey, those probes are there intentionally, just use those' is important. > >> Not allowing 'perf probe' for SDT event will solve all such issues. > >> Also it will make user interface simple and consistent. Other current > >> tooling (systemtap, for instance) also do not allow probing individual > >> markers when there are multiple markers with the same name. > > In any case if others agree with your change in UI flow too then it's fine > > by me, > > but please make it robust, i.e. if perf record sees partially enabled > > probes it > > should still continue. > @Masami, can you please provide your thoughts as well. Yeah, if technically possible to allow both variants, we should leave it up to users to decide what is best? I.e. most people will do auto-probing, not using 'perf probe' at all, documentation should state the pitfalls in doing so. So, after writing the above, perhaps we should warn the user that pre-existing probes are being used, as this will be the odd case? The normal flow will be just using perf record with SDT probes, that will auto-probe them and remove on exit, or better drop a reference to them, as simultaneous use also needs to be covered? - Arnaldo
Re: [RFC] perf/sdt: Directly record SDT event with 'perf record'
Thanks Ingo, On Monday 20 February 2017 02:12 PM, Ingo Molnar wrote: > * Ravi Bangoria wrote: > >> What should be the behavior of the tool? Should it record only one >> 'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it >> should record all the SDT events from libpthread? We can choose either >> of two but both the cases are ambiguous. > They are not ambiguous really if coded right: just pick one of the outcomes > and > maybe print a warning to inform the user that something weird is going on > because > not all markers are enabled? > > As a user I'd expect 'perf record' to enable all markers and print a warning > that > the markers were in a partial state. This would result in consistent > behaviour. Yes, makes sense. > Does it make sense to only enable some of the markers that alias on the same > name? > If not then maybe disallow that in perf probe - or change perf probe to do > the > same thing as perf record. 'perf probe' is doing that correctly. It fetches all events with given name from probe-cache and creates entries for them in uprobe_events. The problem is the 2-step process of adding probes and then recording, allowing users to select individual markers to record on. > > I.e. this is IMHO an artificial problem that users should not be exposed to > and > which can be solved by tooling. > > In particular if it's possible to enable only a part of the markers then perf > record not continuing would be a failure mode: if for example a previous perf > record session segfaulted (or ran out of RAM or was killed in the wrong > moment or > whatever) then it would not be possible to (easily) clean up the mess. Agreed. We need to make this more robust. > >> Not allowing 'perf probe' for SDT event will solve all such issues. >> Also it will make user interface simple and consistent. Other current >> tooling (systemtap, for instance) also do not allow probing individual >> markers when there are multiple markers with the same name. > In any case if others agree with your change in UI flow too then it's fine by > me, > but please make it robust, i.e. if perf record sees partially enabled probes > it > should still continue. @Masami, can you please provide your thoughts as well. Thanks, Ravi
Re: [RFC] perf/sdt: Directly record SDT event with 'perf record'
* Ravi Bangoria wrote: > Yes, initially I thought about allowing both, 'perf probe' and > 'perf record' for SDT event. But there are few complications with > it, esp. when multiple SDT events with same name exists. For ex, > > $ readelf -n /usr/lib64/libpthread-2.24.so | grep -A2 Provider > Provider: libpthread > Name: mutex_entry > Location: 0x9ddb, ... > -- > Provider: libpthread > Name: mutex_entry > Location: 0xbcbb, ... > > At the time of record, perf has to check first if there is any > matching entry exists in uprobe_events with that name. If found, > record it, if not, go look into probe cache. If events exists with > same name in probe cache, record all of them. Like, > > If probe point _is not_ created, > $ perf record -a -e sdt_libpthread:mutex_entry > /** Record both sdt_libpthread:mutex_entry **/ > > If probe point _is_ created manually, record that particular event, > $ perf probe -x /usr/lib64/libpthread-2.24.so sdt_libpthread:mutex_entry > Added new events: > sdt_libpthread:mutex_entry (on %mutex_entry in > /usr/lib64/libpthread-2.24.so) > sdt_libpthread:mutex_entry_1 (on %mutex_entry in > /usr/lib64/libpthread-2.24.so) > > $ perf record -a -e sdt_libpthread:mutex_entry > /** Record only first sdt_libpthread:mutex_entry **/ > > Here, same command gives different behaviour for different scenarios. > > Now consider a scenario when probe point exists for any one event: > > $ perf probe -d sdt_libpthread:mutex_entry_1 > $ perf probe --list > sdt_libpthread:mutex_entry (on pthread_mutex_lock+11 in > /usr/lib64/libpthread-2.24.so) > > And user tries to record it by, > $ perf record -a -e sdt_libpthread:* > > What should be the behavior of the tool? Should it record only one > 'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it > should record all the SDT events from libpthread? We can choose either > of two but both the cases are ambiguous. They are not ambiguous really if coded right: just pick one of the outcomes and maybe print a warning to inform the user that something weird is going on because not all markers are enabled? As a user I'd expect 'perf record' to enable all markers and print a warning that the markers were in a partial state. This would result in consistent behaviour. Does it make sense to only enable some of the markers that alias on the same name? If not then maybe disallow that in perf probe - or change perf probe to do the same thing as perf record. I.e. this is IMHO an artificial problem that users should not be exposed to and which can be solved by tooling. In particular if it's possible to enable only a part of the markers then perf record not continuing would be a failure mode: if for example a previous perf record session segfaulted (or ran out of RAM or was killed in the wrong moment or whatever) then it would not be possible to (easily) clean up the mess. > Not allowing 'perf probe' for SDT event will solve all such issues. > Also it will make user interface simple and consistent. Other current > tooling (systemtap, for instance) also do not allow probing individual > markers when there are multiple markers with the same name. In any case if others agree with your change in UI flow too then it's fine by me, but please make it robust, i.e. if perf record sees partially enabled probes it should still continue. Thanks, Ingo
Re: [RFC] perf/sdt: Directly record SDT event with 'perf record'
On Monday 20 February 2017 12:38 PM, Ingo Molnar wrote: > * Ravi Bangoria wrote: > >> All events from 'perf list', except SDT events, can be directly recorded >> with 'perf record'. But, the flow is little different for SDT events. >> Probe point for SDT event needs to be created using 'perf probe' before >> recording it using 'perf record'. >> >> As suggested by Ingo[1], it's better to make this process simple by >> creating probe points automatically with 'perf record' for SDT events. >> >> This patch disables 'perf probe' on SDT events to simplify usage. It >> enables recording SDT event only with 'perf record'. >> >> This removes all those 'multiple events with same name' issues by not >> allowing manual probe creation to user. When there are multiple events >> with same name, 'perf record' will record all of them (in line with >> other tools supporting SDT (systemtap)). >> >> I know 'perf probe' for SDT events has already became interface and >> people are using it. But, doing this change will make user interface very >> easy and also it will make tool behaviour consistent. Also, it won't >> require any changes in uprobe_events structure (suggested by Masami[2]). > So I like the automatism you implemented for 'perf record', but why not keep > the > 'perf probe' flow as well, if people got used to it? > > It's not like computer software is bad at sorting apart and handling the two > cases > properly, right? Thanks Ingo for the reply, Yes, initially I thought about allowing both, 'perf probe' and 'perf record' for SDT event. But there are few complications with it, esp. when multiple SDT events with same name exists. For ex, $ readelf -n /usr/lib64/libpthread-2.24.so | grep -A2 Provider Provider: libpthread Name: mutex_entry Location: 0x9ddb, ... -- Provider: libpthread Name: mutex_entry Location: 0xbcbb, ... At the time of record, perf has to check first if there is any matching entry exists in uprobe_events with that name. If found, record it, if not, go look into probe cache. If events exists with same name in probe cache, record all of them. Like, If probe point _is not_ created, $ perf record -a -e sdt_libpthread:mutex_entry /** Record both sdt_libpthread:mutex_entry **/ If probe point _is_ created manually, record that particular event, $ perf probe -x /usr/lib64/libpthread-2.24.so sdt_libpthread:mutex_entry Added new events: sdt_libpthread:mutex_entry (on %mutex_entry in /usr/lib64/libpthread-2.24.so) sdt_libpthread:mutex_entry_1 (on %mutex_entry in /usr/lib64/libpthread-2.24.so) $ perf record -a -e sdt_libpthread:mutex_entry /** Record only first sdt_libpthread:mutex_entry **/ Here, same command gives different behaviour for different scenarios. Now consider a scenario when probe point exists for any one event: $ perf probe -d sdt_libpthread:mutex_entry_1 $ perf probe --list sdt_libpthread:mutex_entry (on pthread_mutex_lock+11 in /usr/lib64/libpthread-2.24.so) And user tries to record it by, $ perf record -a -e sdt_libpthread:* What should be the behavior of the tool? Should it record only one 'sdt_libpthread:mutex_entry' which exists in uprobe_events? Or it should record all the SDT events from libpthread? We can choose either of two but both the cases are ambiguous. Not allowing 'perf probe' for SDT event will solve all such issues. Also it will make user interface simple and consistent. Other current tooling (systemtap, for instance) also do not allow probing individual markers when there are multiple markers with the same name. -Ravi
Re: [RFC] perf/sdt: Directly record SDT event with 'perf record'
* Ravi Bangoria wrote: > All events from 'perf list', except SDT events, can be directly recorded > with 'perf record'. But, the flow is little different for SDT events. > Probe point for SDT event needs to be created using 'perf probe' before > recording it using 'perf record'. > > As suggested by Ingo[1], it's better to make this process simple by > creating probe points automatically with 'perf record' for SDT events. > > This patch disables 'perf probe' on SDT events to simplify usage. It > enables recording SDT event only with 'perf record'. > > This removes all those 'multiple events with same name' issues by not > allowing manual probe creation to user. When there are multiple events > with same name, 'perf record' will record all of them (in line with > other tools supporting SDT (systemtap)). > > I know 'perf probe' for SDT events has already became interface and > people are using it. But, doing this change will make user interface very > easy and also it will make tool behaviour consistent. Also, it won't > require any changes in uprobe_events structure (suggested by Masami[2]). So I like the automatism you implemented for 'perf record', but why not keep the 'perf probe' flow as well, if people got used to it? It's not like computer software is bad at sorting apart and handling the two cases properly, right? Thanks, Ingo
[RFC] perf/sdt: Directly record SDT event with 'perf record'
All events from 'perf list', except SDT events, can be directly recorded
with 'perf record'. But, the flow is little different for SDT events.
Probe point for SDT event needs to be created using 'perf probe' before
recording it using 'perf record'.
As suggested by Ingo[1], it's better to make this process simple by
creating probe points automatically with 'perf record' for SDT events.
This patch disables 'perf probe' on SDT events to simplify usage. It
enables recording SDT event only with 'perf record'.
This removes all those 'multiple events with same name' issues by not
allowing manual probe creation to user. When there are multiple events
with same name, 'perf record' will record all of them (in line with
other tools supporting SDT (systemtap)).
I know 'perf probe' for SDT events has already became interface and
people are using it. But, doing this change will make user interface very
easy and also it will make tool behaviour consistent. Also, it won't
require any changes in uprobe_events structure (suggested by Masami[2]).
After patch:
$ perf list
...
sdt_libpthread:mutex_entry [SDT event]
...
$ perf probe -x /usr/lib64/libpthread-2.24.so %sdt_libpthread:mutex_entry
SDT events don't need to be put in place using 'perf probe' anymore.
You can directly record on SDT events using 'perf record'
Error: Command Parse Error.
$ perf record -a -e %sdt_libpthread:mutex_entry
Warning : Recording on 2 occurences of sdt_libpthread:mutex_entry
^C[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.190 MB perf.data (33 samples) ]
$ perf evlist
sdt_libpthread:mutex_entry
sdt_libpthread:mutex_entry_1
Note:
- This is no way a proper patch. Sending this RFC to get thoughts on the
idea.
- This is Hemant's patch[3] rebased to acme/perf/core, with few changes
to disable 'perf probe' on SDT event.
[1] https://lkml.org/lkml/2017/2/7/59
[2] https://lkml.org/lkml/2016/4/30/50
[3] https://lkml.org/lkml/2016/5/3/810
Signed-off-by: Ravi Bangoria
---
tools/perf/builtin-record.c| 21 +
tools/perf/perf.h | 1 +
tools/perf/util/parse-events.c | 53 ++-
tools/perf/util/parse-events.h | 1 +
tools/perf/util/probe-event.c | 43 ++-
tools/perf/util/probe-event.h | 4 ++
tools/perf/util/probe-file.c | 97 ++
tools/perf/util/probe-file.h | 8
8 files changed, 225 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 2ddf189..37722f0 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -39,6 +39,7 @@
#include "util/trigger.h"
#include "util/perf-hooks.h"
#include "asm/bug.h"
+#include "util/probe-file.h"
#include
#include
@@ -73,6 +74,7 @@ struct record {
booltimestamp_filename;
struct switch_outputswitch_output;
unsigned long long samples;
+ struct list_headsdt_event_list;
};
static volatile int auxtrace_record__snapshot_started;
@@ -1502,6 +1504,23 @@ static struct record record = {
},
};
+void sdt_event_list__add(struct list_head *sdt_event_list)
+{
+ if (list_empty(sdt_event_list))
+ return;
+ list_splice(sdt_event_list, &record.sdt_event_list);
+}
+
+bool is_cmd_record(void)
+{
+ return (record.evlist != NULL);
+}
+
+static void sdt_event_list__remove(struct list_head *sdt_event_list)
+{
+ return remove_sdt_event_list(sdt_event_list);
+}
+
const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
"\n\t\t\t\tDefault: fp";
@@ -1670,6 +1689,7 @@ int cmd_record(int argc, const char **argv, const char
*prefix __maybe_unused)
if (rec->evlist == NULL)
return -ENOMEM;
+ INIT_LIST_HEAD(&rec->sdt_event_list);
err = perf_config(perf_record_config, rec);
if (err)
return err;
@@ -1836,6 +1856,7 @@ int cmd_record(int argc, const char **argv, const char
*prefix __maybe_unused)
perf_evlist__delete(rec->evlist);
symbol__exit();
auxtrace_record__free(rec->itr);
+ sdt_event_list__remove(&rec->sdt_event_list);
return err;
}
diff --git a/tools/perf/perf.h b/tools/perf/perf.h
index 1c27d94..9d8e5fe 100644
--- a/tools/perf/perf.h
+++ b/tools/perf/perf.h
@@ -76,4 +76,5 @@ struct record_opts {
struct option;
extern const char * const *record_usage;
extern struct option *record_options;
+bool is_cmd_record(void);
#endif
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 07be076..3400a5a 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1726,13 +1726,64 @@ static void parse_events_print_error(struct
parse_events_error *err,
#undef MAX_WIDTH
+static int parse_sdt_event(const char *str, struct list_head **sdt_l
