* [PATCH v2 01/12] tracing/uprobes: Add busy check when cleanup all uprobes
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
@ 2018-11-05 9:00 ` Masami Hiramatsu
2018-12-04 17:43 ` Steven Rostedt
2018-11-05 9:00 ` [PATCH v2 02/12] tracing: Lock event_mutex before synth_event_mutex Masami Hiramatsu
` (11 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:00 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Add a busy check loop in cleanup_all_probes() before
trying to remove all events in uprobe_events as same as
kprobe_events does.
Without this change, writing null to uprobe_events will
try to remove events but if one of them is enabled, it
stopped there but some of events are already cleared.
With this change, writing null to uprobe_events make
sure all events are not enabled before removing events.
So, it clears all events, or return an error (-EBUSY)
with keeping all events.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
kernel/trace/trace_uprobe.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 31ea48eceda1..b708e4ff7ea7 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -587,12 +587,19 @@ static int cleanup_all_probes(void)
int ret = 0;
mutex_lock(&uprobe_lock);
+ /* Ensure no probe is in use. */
+ list_for_each_entry(tu, &uprobe_list, list)
+ if (trace_probe_is_enabled(&tu->tp)) {
+ ret = -EBUSY;
+ goto end;
+ }
while (!list_empty(&uprobe_list)) {
tu = list_entry(uprobe_list.next, struct trace_uprobe, list);
ret = unregister_trace_uprobe(tu);
if (ret)
break;
}
+end:
mutex_unlock(&uprobe_lock);
return ret;
}
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 01/12] tracing/uprobes: Add busy check when cleanup all uprobes
2018-11-05 9:00 ` [PATCH v2 01/12] tracing/uprobes: Add busy check when cleanup all uprobes Masami Hiramatsu
@ 2018-12-04 17:43 ` Steven Rostedt
2018-12-07 2:19 ` Masami Hiramatsu
0 siblings, 1 reply; 22+ messages in thread
From: Steven Rostedt @ 2018-12-04 17:43 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
On Mon, 5 Nov 2018 18:00:15 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> Add a busy check loop in cleanup_all_probes() before
> trying to remove all events in uprobe_events as same as
> kprobe_events does.
>
> Without this change, writing null to uprobe_events will
> try to remove events but if one of them is enabled, it
> stopped there but some of events are already cleared.
>
> With this change, writing null to uprobe_events make
> sure all events are not enabled before removing events.
> So, it clears all events, or return an error (-EBUSY)
> with keeping all events.
>
Hmm, should this patch be marked as stable?
-- Steve
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
> kernel/trace/trace_uprobe.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 31ea48eceda1..b708e4ff7ea7 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -587,12 +587,19 @@ static int cleanup_all_probes(void)
> int ret = 0;
>
> mutex_lock(&uprobe_lock);
> + /* Ensure no probe is in use. */
> + list_for_each_entry(tu, &uprobe_list, list)
> + if (trace_probe_is_enabled(&tu->tp)) {
> + ret = -EBUSY;
> + goto end;
> + }
> while (!list_empty(&uprobe_list)) {
> tu = list_entry(uprobe_list.next, struct trace_uprobe, list);
> ret = unregister_trace_uprobe(tu);
> if (ret)
> break;
> }
> +end:
> mutex_unlock(&uprobe_lock);
> return ret;
> }
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 01/12] tracing/uprobes: Add busy check when cleanup all uprobes
2018-12-04 17:43 ` Steven Rostedt
@ 2018-12-07 2:19 ` Masami Hiramatsu
0 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-12-07 2:19 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
On Tue, 4 Dec 2018 12:43:33 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 5 Nov 2018 18:00:15 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > Add a busy check loop in cleanup_all_probes() before
> > trying to remove all events in uprobe_events as same as
> > kprobe_events does.
> >
> > Without this change, writing null to uprobe_events will
> > try to remove events but if one of them is enabled, it
> > stopped there but some of events are already cleared.
> >
> > With this change, writing null to uprobe_events make
> > sure all events are not enabled before removing events.
> > So, it clears all events, or return an error (-EBUSY)
> > with keeping all events.
> >
>
> Hmm, should this patch be marked as stable?
Hmm, OK, let this go to stable. Since anyway, this will cause
a wired result on uprobe_events from user point of view.
Thank you!
>
> -- Steve
>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> > kernel/trace/trace_uprobe.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index 31ea48eceda1..b708e4ff7ea7 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -587,12 +587,19 @@ static int cleanup_all_probes(void)
> > int ret = 0;
> >
> > mutex_lock(&uprobe_lock);
> > + /* Ensure no probe is in use. */
> > + list_for_each_entry(tu, &uprobe_list, list)
> > + if (trace_probe_is_enabled(&tu->tp)) {
> > + ret = -EBUSY;
> > + goto end;
> > + }
> > while (!list_empty(&uprobe_list)) {
> > tu = list_entry(uprobe_list.next, struct trace_uprobe, list);
> > ret = unregister_trace_uprobe(tu);
> > if (ret)
> > break;
> > }
> > +end:
> > mutex_unlock(&uprobe_lock);
> > return ret;
> > }
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 02/12] tracing: Lock event_mutex before synth_event_mutex
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
2018-11-05 9:00 ` [PATCH v2 01/12] tracing/uprobes: Add busy check when cleanup all uprobes Masami Hiramatsu
@ 2018-11-05 9:00 ` Masami Hiramatsu
2018-11-05 9:01 ` [PATCH v2 03/12] tracing: Simplify creation and deletion of synthetic event Masami Hiramatsu
` (10 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:00 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
synthetic event is using synth_event_mutex for protecting
synth_event_list, and event_trigger_write() path acquires
locks as below order.
event_trigger_write(event_mutex)
->trigger_process_regex(trigger_cmd_mutex)
->event_hist_trigger_func(synth_event_mutex)
On the other hand, synthetic event creation and deletion paths
finally calls trace_add_event_call() and trace_remove_event_call()
which acquires event_mutex. In that case, if we keep the
synth_event_mutex locked while registering/unregistering synthetic
events, its dependency will be inversed.
To avoid this issue, current synthetic event is using 2 phases
process to create/delete events. For example, it searches existing
event under synth_event_mutex to checking event-name conflict, and
unlock synth_event_mutex, then register new event under event_mutex
locked. Finally, it locks synth_event_mutex and tries to add the
new event to the list. But it can introduce complexity and a chance
of name conflict.
To solve this simpler, this introduces trace_add_event_call_nolock()
and trace_remove_event_call_nolock() which don't acquires
event_mutex inside. synthetic event can lock event_mutex before
synth_event_mutex for solving lock dependency issue simpler.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
include/linux/trace_events.h | 2 ++
kernel/trace/trace_events.c | 34 ++++++++++++++++++++++++++++------
kernel/trace/trace_events_hist.c | 24 ++++++++++--------------
3 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 4130a5497d40..3aa05593a53f 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -529,6 +529,8 @@ extern int trace_event_raw_init(struct trace_event_call *call);
extern int trace_define_field(struct trace_event_call *call, const char *type,
const char *name, int offset, int size,
int is_signed, int filter_type);
+extern int trace_add_event_call_nolock(struct trace_event_call *call);
+extern int trace_remove_event_call_nolock(struct trace_event_call *call);
extern int trace_add_event_call(struct trace_event_call *call);
extern int trace_remove_event_call(struct trace_event_call *call);
extern int trace_event_get_offsets(struct trace_event_call *call);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index f94be0c2827b..a3b157f689ee 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2305,11 +2305,11 @@ __trace_early_add_new_event(struct trace_event_call *call,
struct ftrace_module_file_ops;
static void __add_event_to_tracers(struct trace_event_call *call);
-/* Add an additional event_call dynamically */
-int trace_add_event_call(struct trace_event_call *call)
+int trace_add_event_call_nolock(struct trace_event_call *call)
{
int ret;
- mutex_lock(&event_mutex);
+ lockdep_assert_held(&event_mutex);
+
mutex_lock(&trace_types_lock);
ret = __register_event(call, NULL);
@@ -2317,6 +2317,16 @@ int trace_add_event_call(struct trace_event_call *call)
__add_event_to_tracers(call);
mutex_unlock(&trace_types_lock);
+ return ret;
+}
+
+/* Add an additional event_call dynamically */
+int trace_add_event_call(struct trace_event_call *call)
+{
+ int ret;
+
+ mutex_lock(&event_mutex);
+ ret = trace_add_event_call_nolock(call);
mutex_unlock(&event_mutex);
return ret;
}
@@ -2366,17 +2376,29 @@ static int probe_remove_event_call(struct trace_event_call *call)
return 0;
}
-/* Remove an event_call */
-int trace_remove_event_call(struct trace_event_call *call)
+/* no event_mutex version */
+int trace_remove_event_call_nolock(struct trace_event_call *call)
{
int ret;
- mutex_lock(&event_mutex);
+ lockdep_assert_held(&event_mutex);
+
mutex_lock(&trace_types_lock);
down_write(&trace_event_sem);
ret = probe_remove_event_call(call);
up_write(&trace_event_sem);
mutex_unlock(&trace_types_lock);
+
+ return ret;
+}
+
+/* Remove an event_call */
+int trace_remove_event_call(struct trace_event_call *call)
+{
+ int ret;
+
+ mutex_lock(&event_mutex);
+ ret = trace_remove_event_call_nolock(call);
mutex_unlock(&event_mutex);
return ret;
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index eb908ef2ecec..1670c65389fe 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -912,7 +912,7 @@ static int register_synth_event(struct synth_event *event)
call->data = event;
call->tp = event->tp;
- ret = trace_add_event_call(call);
+ ret = trace_add_event_call_nolock(call);
if (ret) {
pr_warn("Failed to register synthetic event: %s\n",
trace_event_name(call));
@@ -936,7 +936,7 @@ static int unregister_synth_event(struct synth_event *event)
struct trace_event_call *call = &event->call;
int ret;
- ret = trace_remove_event_call(call);
+ ret = trace_remove_event_call_nolock(call);
return ret;
}
@@ -1013,12 +1013,10 @@ static void add_or_delete_synth_event(struct synth_event *event, int delete)
if (delete)
free_synth_event(event);
else {
- mutex_lock(&synth_event_mutex);
if (!find_synth_event(event->name))
list_add(&event->list, &synth_event_list);
else
free_synth_event(event);
- mutex_unlock(&synth_event_mutex);
}
}
@@ -1030,6 +1028,7 @@ static int create_synth_event(int argc, char **argv)
int i, consumed = 0, n_fields = 0, ret = 0;
char *name;
+ mutex_lock(&event_mutex);
mutex_lock(&synth_event_mutex);
/*
@@ -1102,8 +1101,6 @@ static int create_synth_event(int argc, char **argv)
goto err;
}
out:
- mutex_unlock(&synth_event_mutex);
-
if (event) {
if (delete_event) {
ret = unregister_synth_event(event);
@@ -1113,10 +1110,13 @@ static int create_synth_event(int argc, char **argv)
add_or_delete_synth_event(event, ret);
}
}
+ mutex_unlock(&synth_event_mutex);
+ mutex_unlock(&event_mutex);
return ret;
err:
mutex_unlock(&synth_event_mutex);
+ mutex_unlock(&event_mutex);
for (i = 0; i < n_fields; i++)
free_synth_field(fields[i]);
@@ -1127,12 +1127,10 @@ static int create_synth_event(int argc, char **argv)
static int release_all_synth_events(void)
{
- struct list_head release_events;
struct synth_event *event, *e;
int ret = 0;
- INIT_LIST_HEAD(&release_events);
-
+ mutex_lock(&event_mutex);
mutex_lock(&synth_event_mutex);
list_for_each_entry(event, &synth_event_list, list) {
@@ -1142,16 +1140,14 @@ static int release_all_synth_events(void)
}
}
- list_splice_init(&event->list, &release_events);
-
- mutex_unlock(&synth_event_mutex);
-
- list_for_each_entry_safe(event, e, &release_events, list) {
+ list_for_each_entry_safe(event, e, &synth_event_list, list) {
list_del(&event->list);
ret = unregister_synth_event(event);
add_or_delete_synth_event(event, !ret);
}
+ mutex_unlock(&synth_event_mutex);
+ mutex_unlock(&event_mutex);
return ret;
}
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 03/12] tracing: Simplify creation and deletion of synthetic event
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
2018-11-05 9:00 ` [PATCH v2 01/12] tracing/uprobes: Add busy check when cleanup all uprobes Masami Hiramatsu
2018-11-05 9:00 ` [PATCH v2 02/12] tracing: Lock event_mutex before synth_event_mutex Masami Hiramatsu
@ 2018-11-05 9:01 ` Masami Hiramatsu
2018-11-05 9:01 ` [PATCH v2 04/12] tracing: Integrate similar probe argument parsers Masami Hiramatsu
` (9 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:01 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Simplify creation and deletion code of synthetic event.
Since the event_mutex and synth_event_mutex ordering issue
is gone, we can skip existing event check when adding or
deleting event, and some redundant code in error path.
This changes release_all_synth_events() to abort the process
when it hits any error and returns the error code. It succeeds
only if it has no error.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
kernel/trace/trace_events_hist.c | 53 +++++++++++++-------------------------
1 file changed, 18 insertions(+), 35 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 1670c65389fe..0feb7f460123 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1008,18 +1008,6 @@ struct hist_var_data {
struct hist_trigger_data *hist_data;
};
-static void add_or_delete_synth_event(struct synth_event *event, int delete)
-{
- if (delete)
- free_synth_event(event);
- else {
- if (!find_synth_event(event->name))
- list_add(&event->list, &synth_event_list);
- else
- free_synth_event(event);
- }
-}
-
static int create_synth_event(int argc, char **argv)
{
struct synth_field *field, *fields[SYNTH_FIELDS_MAX];
@@ -1052,15 +1040,16 @@ static int create_synth_event(int argc, char **argv)
if (event) {
if (delete_event) {
if (event->ref) {
- event = NULL;
ret = -EBUSY;
goto out;
}
- list_del(&event->list);
- goto out;
- }
- event = NULL;
- ret = -EEXIST;
+ ret = unregister_synth_event(event);
+ if (!ret) {
+ list_del(&event->list);
+ free_synth_event(event);
+ }
+ } else
+ ret = -EEXIST;
goto out;
} else if (delete_event) {
ret = -ENOENT;
@@ -1100,29 +1089,21 @@ static int create_synth_event(int argc, char **argv)
event = NULL;
goto err;
}
+ ret = register_synth_event(event);
+ if (!ret)
+ list_add(&event->list, &synth_event_list);
+ else
+ free_synth_event(event);
out:
- if (event) {
- if (delete_event) {
- ret = unregister_synth_event(event);
- add_or_delete_synth_event(event, !ret);
- } else {
- ret = register_synth_event(event);
- add_or_delete_synth_event(event, ret);
- }
- }
mutex_unlock(&synth_event_mutex);
mutex_unlock(&event_mutex);
return ret;
err:
- mutex_unlock(&synth_event_mutex);
- mutex_unlock(&event_mutex);
-
for (i = 0; i < n_fields; i++)
free_synth_field(fields[i]);
- free_synth_event(event);
- return ret;
+ goto out;
}
static int release_all_synth_events(void)
@@ -1141,10 +1122,12 @@ static int release_all_synth_events(void)
}
list_for_each_entry_safe(event, e, &synth_event_list, list) {
- list_del(&event->list);
-
ret = unregister_synth_event(event);
- add_or_delete_synth_event(event, !ret);
+ if (!ret) {
+ list_del(&event->list);
+ free_synth_event(event);
+ } else
+ break;
}
mutex_unlock(&synth_event_mutex);
mutex_unlock(&event_mutex);
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 04/12] tracing: Integrate similar probe argument parsers
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
` (2 preceding siblings ...)
2018-11-05 9:01 ` [PATCH v2 03/12] tracing: Simplify creation and deletion of synthetic event Masami Hiramatsu
@ 2018-11-05 9:01 ` Masami Hiramatsu
2018-11-05 9:02 ` [PATCH v2 05/12] tracing: Add unified dynamic event framework Masami Hiramatsu
` (8 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:01 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Integrate similar argument parsers for kprobes and uprobes events
into traceprobe_parse_probe_arg().
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
kernel/trace/trace_kprobe.c | 48 ++-----------------------------------------
kernel/trace/trace_probe.c | 47 +++++++++++++++++++++++++++++++++++++++---
kernel/trace/trace_probe.h | 7 ++----
kernel/trace/trace_uprobe.c | 44 ++-------------------------------------
4 files changed, 50 insertions(+), 96 deletions(-)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index fec67188c4d2..d313bcc259dc 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -548,7 +548,6 @@ static int create_trace_kprobe(int argc, char **argv)
bool is_return = false, is_delete = false;
char *symbol = NULL, *event = NULL, *group = NULL;
int maxactive = 0;
- char *arg;
long offset = 0;
void *addr = NULL;
char buf[MAX_EVENT_NAME_LEN];
@@ -676,53 +675,10 @@ static int create_trace_kprobe(int argc, char **argv)
}
/* parse arguments */
- ret = 0;
for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) {
- struct probe_arg *parg = &tk->tp.args[i];
-
- /* Increment count for freeing args in error case */
- tk->tp.nr_args++;
-
- /* Parse argument name */
- arg = strchr(argv[i], '=');
- if (arg) {
- *arg++ = '\0';
- parg->name = kstrdup(argv[i], GFP_KERNEL);
- } else {
- arg = argv[i];
- /* If argument name is omitted, set "argN" */
- snprintf(buf, MAX_EVENT_NAME_LEN, "arg%d", i + 1);
- parg->name = kstrdup(buf, GFP_KERNEL);
- }
-
- if (!parg->name) {
- pr_info("Failed to allocate argument[%d] name.\n", i);
- ret = -ENOMEM;
- goto error;
- }
-
- if (!is_good_name(parg->name)) {
- pr_info("Invalid argument[%d] name: %s\n",
- i, parg->name);
- ret = -EINVAL;
- goto error;
- }
-
- if (traceprobe_conflict_field_name(parg->name,
- tk->tp.args, i)) {
- pr_info("Argument[%d] name '%s' conflicts with "
- "another field.\n", i, argv[i]);
- ret = -EINVAL;
- goto error;
- }
-
- /* Parse fetch argument */
- ret = traceprobe_parse_probe_arg(arg, &tk->tp.size, parg,
- flags);
- if (ret) {
- pr_info("Parse error at argument[%d]. (%d)\n", i, ret);
+ ret = traceprobe_parse_probe_arg(&tk->tp, i, argv[i], flags);
+ if (ret)
goto error;
- }
}
ret = register_trace_kprobe(tk);
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index bd30e9398d2a..449150c6a87f 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -348,7 +348,7 @@ static int __parse_bitfield_probe_arg(const char *bf,
}
/* String length checking wrapper */
-int traceprobe_parse_probe_arg(char *arg, ssize_t *size,
+static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
struct probe_arg *parg, unsigned int flags)
{
struct fetch_insn *code, *scode, *tmp = NULL;
@@ -491,8 +491,8 @@ int traceprobe_parse_probe_arg(char *arg, ssize_t *size,
}
/* Return 1 if name is reserved or already used by another argument */
-int traceprobe_conflict_field_name(const char *name,
- struct probe_arg *args, int narg)
+static int traceprobe_conflict_field_name(const char *name,
+ struct probe_arg *args, int narg)
{
int i;
@@ -507,6 +507,47 @@ int traceprobe_conflict_field_name(const char *name,
return 0;
}
+int traceprobe_parse_probe_arg(struct trace_probe *tp, int i, char *arg,
+ unsigned int flags)
+{
+ struct probe_arg *parg = &tp->args[i];
+ char *body;
+ int ret;
+
+ /* Increment count for freeing args in error case */
+ tp->nr_args++;
+
+ body = strchr(arg, '=');
+ if (body) {
+ parg->name = kmemdup_nul(arg, body - arg, GFP_KERNEL);
+ body++;
+ } else {
+ /* If argument name is omitted, set "argN" */
+ parg->name = kasprintf(GFP_KERNEL, "arg%d", i + 1);
+ body = arg;
+ }
+ if (!parg->name)
+ return -ENOMEM;
+
+ if (!is_good_name(parg->name)) {
+ pr_info("Invalid argument[%d] name: %s\n",
+ i, parg->name);
+ return -EINVAL;
+ }
+
+ if (traceprobe_conflict_field_name(parg->name, tp->args, i)) {
+ pr_info("Argument[%d]: '%s' conflicts with another field.\n",
+ i, parg->name);
+ return -EINVAL;
+ }
+
+ /* Parse fetch argument */
+ ret = traceprobe_parse_probe_arg_body(body, &tp->size, parg, flags);
+ if (ret)
+ pr_info("Parse error at argument[%d]. (%d)\n", i, ret);
+ return ret;
+}
+
void traceprobe_free_probe_arg(struct probe_arg *arg)
{
struct fetch_insn *code = arg->code;
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index 974afc1a3e73..feeec261b356 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -272,11 +272,8 @@ find_event_file_link(struct trace_probe *tp, struct trace_event_file *file)
#define TPARG_FL_FENTRY BIT(2)
#define TPARG_FL_MASK GENMASK(2, 0)
-extern int traceprobe_parse_probe_arg(char *arg, ssize_t *size,
- struct probe_arg *parg, unsigned int flags);
-
-extern int traceprobe_conflict_field_name(const char *name,
- struct probe_arg *args, int narg);
+extern int traceprobe_parse_probe_arg(struct trace_probe *tp, int i,
+ char *arg, unsigned int flags);
extern int traceprobe_update_arg(struct probe_arg *arg);
extern void traceprobe_free_probe_arg(struct probe_arg *arg);
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index b708e4ff7ea7..6eaaa2150685 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -517,51 +517,11 @@ static int create_trace_uprobe(int argc, char **argv)
}
/* parse arguments */
- ret = 0;
for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) {
- struct probe_arg *parg = &tu->tp.args[i];
-
- /* Increment count for freeing args in error case */
- tu->tp.nr_args++;
-
- /* Parse argument name */
- arg = strchr(argv[i], '=');
- if (arg) {
- *arg++ = '\0';
- parg->name = kstrdup(argv[i], GFP_KERNEL);
- } else {
- arg = argv[i];
- /* If argument name is omitted, set "argN" */
- snprintf(buf, MAX_EVENT_NAME_LEN, "arg%d", i + 1);
- parg->name = kstrdup(buf, GFP_KERNEL);
- }
-
- if (!parg->name) {
- pr_info("Failed to allocate argument[%d] name.\n", i);
- ret = -ENOMEM;
- goto error;
- }
-
- if (!is_good_name(parg->name)) {
- pr_info("Invalid argument[%d] name: %s\n", i, parg->name);
- ret = -EINVAL;
- goto error;
- }
-
- if (traceprobe_conflict_field_name(parg->name, tu->tp.args, i)) {
- pr_info("Argument[%d] name '%s' conflicts with "
- "another field.\n", i, argv[i]);
- ret = -EINVAL;
- goto error;
- }
-
- /* Parse fetch argument */
- ret = traceprobe_parse_probe_arg(arg, &tu->tp.size, parg,
+ ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i],
is_return ? TPARG_FL_RETURN : 0);
- if (ret) {
- pr_info("Parse error at argument[%d]. (%d)\n", i, ret);
+ if (ret)
goto error;
- }
}
ret = register_trace_uprobe(tu);
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 05/12] tracing: Add unified dynamic event framework
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
` (3 preceding siblings ...)
2018-11-05 9:01 ` [PATCH v2 04/12] tracing: Integrate similar probe argument parsers Masami Hiramatsu
@ 2018-11-05 9:02 ` Masami Hiramatsu
2018-11-05 9:02 ` [PATCH v2 06/12] tracing/kprobes: Use dyn_event framework for kprobe events Masami Hiramatsu
` (7 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:02 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Add unified dynamic event framework for ftrace kprobes, uprobes
and synthetic events. Those dynamic events can be co-exist on
same file because those syntax doesn't over-wrapped.
This introduces a framework part which provides a unified tracefs
interface and operations.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
Changes in v2:
- Fix to lock dyn_event_mutex when freeing all events.
- Generalize and export dyn_event_release_all().
- To fix dependency issue, lock event_mutex before dyn_event_mutex.
- Separate create and delete operation.
- If create operation returns -ECANCELED, try next operation.
- Pass const argv to create operation for safety.
- Add .match operation and generic delete API.
- Add precise description for data structures.
- Remove dyn_event_mutex, use event_mutex instead.
---
kernel/trace/Kconfig | 3 +
kernel/trace/Makefile | 1
kernel/trace/trace.c | 4 +
kernel/trace/trace_dynevent.c | 210 +++++++++++++++++++++++++++++++++++++++++
kernel/trace/trace_dynevent.h | 119 +++++++++++++++++++++++
5 files changed, 337 insertions(+)
create mode 100644 kernel/trace/trace_dynevent.c
create mode 100644 kernel/trace/trace_dynevent.h
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 5e3de28c7677..bf2e8a5a91f1 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -518,6 +518,9 @@ config BPF_EVENTS
help
This allows the user to attach BPF programs to kprobe events.
+config DYNAMIC_EVENTS
+ def_bool n
+
config PROBE_EVENTS
def_bool n
diff --git a/kernel/trace/Makefile b/kernel/trace/Makefile
index f81dadbc7c4a..9ff3c4fa91b6 100644
--- a/kernel/trace/Makefile
+++ b/kernel/trace/Makefile
@@ -78,6 +78,7 @@ endif
ifeq ($(CONFIG_TRACING),y)
obj-$(CONFIG_KGDB_KDB) += trace_kdb.o
endif
+obj-$(CONFIG_DYNAMIC_EVENTS) += trace_dynevent.o
obj-$(CONFIG_PROBE_EVENTS) += trace_probe.o
obj-$(CONFIG_UPROBE_EVENTS) += trace_uprobe.o
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index ff1c4b20cd0a..2886e92e8eab 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4604,6 +4604,10 @@ static const char readme_msg[] =
"\t\t\t traces\n"
#endif
#endif /* CONFIG_STACK_TRACER */
+#ifdef CONFIG_DYNAMIC_EVENTS
+ " dynamic_events\t\t- Add/remove/show the generic dynamic events\n"
+ "\t\t\t Write into this file to define/undefine new trace events.\n"
+#endif
#ifdef CONFIG_KPROBE_EVENTS
" kprobe_events\t\t- Add/remove/show the kernel dynamic events\n"
"\t\t\t Write into this file to define/undefine new trace events.\n"
diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
new file mode 100644
index 000000000000..f17a887abb66
--- /dev/null
+++ b/kernel/trace/trace_dynevent.c
@@ -0,0 +1,210 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic dynamic event control interface
+ *
+ * Copyright (C) 2018 Masami Hiramatsu <mhiramat@kernel.org>
+ */
+
+#include <linux/debugfs.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/mm.h>
+#include <linux/mutex.h>
+#include <linux/tracefs.h>
+
+#include "trace.h"
+#include "trace_dynevent.h"
+
+static DEFINE_MUTEX(dyn_event_ops_mutex);
+static LIST_HEAD(dyn_event_ops_list);
+
+int dyn_event_register(struct dyn_event_operations *ops)
+{
+ if (!ops || !ops->create || !ops->show || !ops->is_busy ||
+ !ops->free || !ops->match)
+ return -EINVAL;
+
+ INIT_LIST_HEAD(&ops->list);
+ mutex_lock(&dyn_event_ops_mutex);
+ list_add_tail(&ops->list, &dyn_event_ops_list);
+ mutex_unlock(&dyn_event_ops_mutex);
+ return 0;
+}
+
+int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
+{
+ struct dyn_event *pos, *n;
+ char *system = NULL, *event, *p;
+ int ret = -ENOENT;
+
+ if (argv[0][1] != ':')
+ return -EINVAL;
+
+ event = &argv[0][2];
+ p = strchr(event, '/');
+ if (p) {
+ system = event;
+ event = p + 1;
+ *p = '\0';
+ }
+ if (event[0] == '\0')
+ return -EINVAL;
+
+ mutex_lock(&event_mutex);
+ for_each_dyn_event_safe(pos, n) {
+ if (type && type != pos->ops)
+ continue;
+ if (pos->ops->match(system, event, pos)) {
+ ret = pos->ops->free(pos);
+ break;
+ }
+ }
+ mutex_unlock(&event_mutex);
+
+ return ret;
+}
+
+static int create_dyn_event(int argc, char **argv)
+{
+ struct dyn_event_operations *ops;
+ int ret;
+
+ if (argv[0][0] == '-')
+ return dyn_event_release(argc, argv, NULL);
+
+ mutex_lock(&dyn_event_ops_mutex);
+ list_for_each_entry(ops, &dyn_event_ops_list, list) {
+ ret = ops->create(argc, (const char **)argv);
+ if (!ret || ret != -ECANCELED)
+ break;
+ }
+ mutex_unlock(&dyn_event_ops_mutex);
+ if (ret == -ECANCELED)
+ ret = -EINVAL;
+
+ return ret;
+}
+
+/* Protected by event_mutex */
+LIST_HEAD(dyn_event_list);
+
+void *dyn_event_seq_start(struct seq_file *m, loff_t *pos)
+{
+ mutex_lock(&event_mutex);
+ return seq_list_start(&dyn_event_list, *pos);
+}
+
+void *dyn_event_seq_next(struct seq_file *m, void *v, loff_t *pos)
+{
+ return seq_list_next(v, &dyn_event_list, pos);
+}
+
+void dyn_event_seq_stop(struct seq_file *m, void *v)
+{
+ mutex_unlock(&event_mutex);
+}
+
+static int dyn_event_seq_show(struct seq_file *m, void *v)
+{
+ struct dyn_event *ev = v;
+
+ if (ev && ev->ops)
+ return ev->ops->show(m, ev);
+
+ return 0;
+}
+
+static const struct seq_operations dyn_event_seq_op = {
+ .start = dyn_event_seq_start,
+ .next = dyn_event_seq_next,
+ .stop = dyn_event_seq_stop,
+ .show = dyn_event_seq_show
+};
+
+/*
+ * dyn_events_release_all - Release all specific events
+ * @type: the dyn_event_operations * which filters releasing events
+ *
+ * This releases all events which ->ops matches @type. If @type is NULL,
+ * all events are released.
+ * Return -EBUSY if any of them are in use, and return other errors when
+ * it failed to free the given event. Except for -EBUSY, event releasing
+ * process will be aborted at that point and there may be some other
+ * releasable events on the list.
+ */
+int dyn_events_release_all(struct dyn_event_operations *type)
+{
+ struct dyn_event *ev, *tmp;
+ int ret = 0;
+
+ mutex_lock(&event_mutex);
+ for_each_dyn_event(ev) {
+ if (type && ev->ops != type)
+ continue;
+ if (ev->ops->is_busy(ev)) {
+ ret = -EBUSY;
+ goto out;
+ }
+ }
+ for_each_dyn_event_safe(ev, tmp) {
+ if (type && ev->ops != type)
+ continue;
+ ret = ev->ops->free(ev);
+ if (ret)
+ break;
+ }
+out:
+ mutex_unlock(&event_mutex);
+
+ return ret;
+}
+
+static int dyn_event_open(struct inode *inode, struct file *file)
+{
+ int ret;
+
+ if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) {
+ ret = dyn_events_release_all(NULL);
+ if (ret < 0)
+ return ret;
+ }
+
+ return seq_open(file, &dyn_event_seq_op);
+}
+
+static ssize_t dyn_event_write(struct file *file, const char __user *buffer,
+ size_t count, loff_t *ppos)
+{
+ return trace_parse_run_command(file, buffer, count, ppos,
+ create_dyn_event);
+}
+
+static const struct file_operations dynamic_events_ops = {
+ .owner = THIS_MODULE,
+ .open = dyn_event_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = seq_release,
+ .write = dyn_event_write,
+};
+
+/* Make a tracefs interface for controlling dynamic events */
+static __init int init_dynamic_event(void)
+{
+ struct dentry *d_tracer;
+ struct dentry *entry;
+
+ d_tracer = tracing_init_dentry();
+ if (IS_ERR(d_tracer))
+ return 0;
+
+ entry = tracefs_create_file("dynamic_events", 0644, d_tracer,
+ NULL, &dynamic_events_ops);
+
+ /* Event list interface */
+ if (!entry)
+ pr_warn("Could not create tracefs 'dynamic_events' entry\n");
+
+ return 0;
+}
+fs_initcall(init_dynamic_event);
diff --git a/kernel/trace/trace_dynevent.h b/kernel/trace/trace_dynevent.h
new file mode 100644
index 000000000000..8c334064e4d6
--- /dev/null
+++ b/kernel/trace/trace_dynevent.h
@@ -0,0 +1,119 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Common header file for generic dynamic events.
+ */
+
+#ifndef _TRACE_DYNEVENT_H
+#define _TRACE_DYNEVENT_H
+
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/seq_file.h>
+
+#include "trace.h"
+
+struct dyn_event;
+
+/**
+ * struct dyn_event_operations - Methods for each type of dynamic events
+ *
+ * These methods must be set for each type, since there is no default method.
+ * Before using this for dyn_event_init(), it must be registered by
+ * dyn_event_register().
+ *
+ * @create: Parse and create event method. This is invoked when user passes
+ * a event definition to dynamic_events interface. This must not destruct
+ * the arguments and return -ECANCELED if given arguments doesn't match its
+ * command prefix.
+ * @show: Showing method. This is invoked when user reads the event definitions
+ * via dynamic_events interface.
+ * @is_busy: Check whether given event is busy so that it can not be deleted.
+ * Return true if it is busy, otherwides false.
+ * @free: Delete the given event. Return 0 if success, otherwides error.
+ * @match: Check whether given event and system name match this event.
+ * Return true if it matches, otherwides false.
+ *
+ * Except for @create, these methods are called under holding event_mutex.
+ */
+struct dyn_event_operations {
+ struct list_head list;
+ int (*create)(int argc, const char *argv[]);
+ int (*show)(struct seq_file *m, struct dyn_event *ev);
+ bool (*is_busy)(struct dyn_event *ev);
+ int (*free)(struct dyn_event *ev);
+ bool (*match)(const char *system, const char *event,
+ struct dyn_event *ev);
+};
+
+/* Register new dyn_event type -- must be called at first */
+int dyn_event_register(struct dyn_event_operations *ops);
+
+/**
+ * struct dyn_event - Dynamic event list header
+ *
+ * The dyn_event structure encapsulates a list and a pointer to the operators
+ * for making a global list of dynamic events.
+ * User must includes this in each event structure, so that those events can
+ * be added/removed via dynamic_events interface.
+ */
+struct dyn_event {
+ struct list_head list;
+ struct dyn_event_operations *ops;
+};
+
+extern struct list_head dyn_event_list;
+
+static inline
+int dyn_event_init(struct dyn_event *ev, struct dyn_event_operations *ops)
+{
+ if (!ev || !ops)
+ return -EINVAL;
+
+ INIT_LIST_HEAD(&ev->list);
+ ev->ops = ops;
+ return 0;
+}
+
+static inline int dyn_event_add(struct dyn_event *ev)
+{
+ lockdep_assert_held(&event_mutex);
+
+ if (!ev || !ev->ops)
+ return -EINVAL;
+
+ list_add_tail(&ev->list, &dyn_event_list);
+ return 0;
+}
+
+static inline void dyn_event_remove(struct dyn_event *ev)
+{
+ lockdep_assert_held(&event_mutex);
+ list_del_init(&ev->list);
+}
+
+void *dyn_event_seq_start(struct seq_file *m, loff_t *pos);
+void *dyn_event_seq_next(struct seq_file *m, void *v, loff_t *pos);
+void dyn_event_seq_stop(struct seq_file *m, void *v);
+int dyn_events_release_all(struct dyn_event_operations *type);
+int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type);
+
+/*
+ * for_each_dyn_event - iterate over the dyn_event list
+ * @pos: the struct dyn_event * to use as a loop cursor
+ *
+ * This is just a basement of for_each macro. Wrap this for
+ * each actual event structure with ops filtering.
+ */
+#define for_each_dyn_event(pos) \
+ list_for_each_entry(pos, &dyn_event_list, list)
+
+/*
+ * for_each_dyn_event - iterate over the dyn_event list safely
+ * @pos: the struct dyn_event * to use as a loop cursor
+ * @n: the struct dyn_event * to use as temporary storage
+ */
+#define for_each_dyn_event_safe(pos, n) \
+ list_for_each_entry_safe(pos, n, &dyn_event_list, list)
+
+#endif
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 06/12] tracing/kprobes: Use dyn_event framework for kprobe events
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
` (4 preceding siblings ...)
2018-11-05 9:02 ` [PATCH v2 05/12] tracing: Add unified dynamic event framework Masami Hiramatsu
@ 2018-11-05 9:02 ` Masami Hiramatsu
2018-11-05 9:03 ` [PATCH v2 07/12] tracing/uprobes: Use dyn_event framework for uprobe events Masami Hiramatsu
` (6 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:02 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Use dyn_event framework for kprobe events. This shows
kprobe events on "tracing/dynamic_events" file.
User can also define new events via tracing/dynamic_events.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
Changes in v2:
- Use dyn_events_release_all() for clearing events.
- Use nolock event_call register/unregister and lock event_mutex
before dyn_event_mutex to avoid lock dependency error.
- Check probe cleanup result in self-check.
- Rewrite trace_kprobe_create not to destroy arguments.
- Add match operation and new APIs for deleting event.
- Return -ECANCEL if given definition is not for kprobes.
- Remove unused for_each_trace_kprobe_safe.
---
Documentation/trace/kprobetrace.rst | 3
kernel/trace/Kconfig | 1
kernel/trace/trace_kprobe.c | 319 +++++++++++++++++++----------------
kernel/trace/trace_probe.c | 27 +++
kernel/trace/trace_probe.h | 2
5 files changed, 207 insertions(+), 145 deletions(-)
diff --git a/Documentation/trace/kprobetrace.rst b/Documentation/trace/kprobetrace.rst
index 47e765c2f2c3..235ce2ab131a 100644
--- a/Documentation/trace/kprobetrace.rst
+++ b/Documentation/trace/kprobetrace.rst
@@ -20,6 +20,9 @@ current_tracer. Instead of that, add probe points via
/sys/kernel/debug/tracing/kprobe_events, and enable it via
/sys/kernel/debug/tracing/events/kprobes/<EVENT>/enable.
+You can also use /sys/kernel/debug/tracing/dynamic_events instead of
+kprobe_events. That interface will provide unified access to other
+dynamic events too.
Synopsis of kprobe_events
-------------------------
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index bf2e8a5a91f1..c0f6b0105609 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -461,6 +461,7 @@ config KPROBE_EVENTS
bool "Enable kprobes-based dynamic events"
select TRACING
select PROBE_EVENTS
+ select DYNAMIC_EVENTS
default y
help
This allows the user to add tracing events (similar to tracepoints)
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index d313bcc259dc..bdf8c2ad5152 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -12,6 +12,7 @@
#include <linux/rculist.h>
#include <linux/error-injection.h>
+#include "trace_dynevent.h"
#include "trace_kprobe_selftest.h"
#include "trace_probe.h"
#include "trace_probe_tmpl.h"
@@ -19,17 +20,51 @@
#define KPROBE_EVENT_SYSTEM "kprobes"
#define KRETPROBE_MAXACTIVE_MAX 4096
+static int trace_kprobe_create(int argc, const char **argv);
+static int trace_kprobe_show(struct seq_file *m, struct dyn_event *ev);
+static int trace_kprobe_release(struct dyn_event *ev);
+static bool trace_kprobe_is_busy(struct dyn_event *ev);
+static bool trace_kprobe_match(const char *system, const char *event,
+ struct dyn_event *ev);
+
+static struct dyn_event_operations trace_kprobe_ops = {
+ .create = trace_kprobe_create,
+ .show = trace_kprobe_show,
+ .is_busy = trace_kprobe_is_busy,
+ .free = trace_kprobe_release,
+ .match = trace_kprobe_match,
+};
+
/**
* Kprobe event core functions
*/
struct trace_kprobe {
- struct list_head list;
+ struct dyn_event devent;
struct kretprobe rp; /* Use rp.kp for kprobe use */
unsigned long __percpu *nhit;
const char *symbol; /* symbol name */
struct trace_probe tp;
};
+static bool is_trace_kprobe(struct dyn_event *ev)
+{
+ return ev->ops == &trace_kprobe_ops;
+}
+
+static struct trace_kprobe *to_trace_kprobe(struct dyn_event *ev)
+{
+ return container_of(ev, struct trace_kprobe, devent);
+}
+
+/**
+ * for_each_trace_kprobe - iterate over the trace_kprobe list
+ * @pos: the struct trace_kprobe * for each entry
+ * @dpos: the struct dyn_event * to use as a loop cursor
+ */
+#define for_each_trace_kprobe(pos, dpos) \
+ for_each_dyn_event(dpos) \
+ if (is_trace_kprobe(dpos) && (pos = to_trace_kprobe(dpos)))
+
#define SIZEOF_TRACE_KPROBE(n) \
(offsetof(struct trace_kprobe, tp.args) + \
(sizeof(struct probe_arg) * (n)))
@@ -81,6 +116,22 @@ static nokprobe_inline bool trace_kprobe_module_exist(struct trace_kprobe *tk)
return ret;
}
+static bool trace_kprobe_is_busy(struct dyn_event *ev)
+{
+ struct trace_kprobe *tk = to_trace_kprobe(ev);
+
+ return trace_probe_is_enabled(&tk->tp);
+}
+
+static bool trace_kprobe_match(const char *system, const char *event,
+ struct dyn_event *ev)
+{
+ struct trace_kprobe *tk = to_trace_kprobe(ev);
+
+ return strcmp(trace_event_name(&tk->tp.call), event) == 0 &&
+ (!system || strcmp(tk->tp.call.class->system, system) == 0);
+}
+
static nokprobe_inline unsigned long trace_kprobe_nhit(struct trace_kprobe *tk)
{
unsigned long nhit = 0;
@@ -128,9 +179,6 @@ bool trace_kprobe_error_injectable(struct trace_event_call *call)
static int register_kprobe_event(struct trace_kprobe *tk);
static int unregister_kprobe_event(struct trace_kprobe *tk);
-static DEFINE_MUTEX(probe_lock);
-static LIST_HEAD(probe_list);
-
static int kprobe_dispatcher(struct kprobe *kp, struct pt_regs *regs);
static int kretprobe_dispatcher(struct kretprobe_instance *ri,
struct pt_regs *regs);
@@ -192,7 +240,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group,
if (!tk->tp.class.system)
goto error;
- INIT_LIST_HEAD(&tk->list);
+ dyn_event_init(&tk->devent, &trace_kprobe_ops);
INIT_LIST_HEAD(&tk->tp.files);
return tk;
error:
@@ -207,6 +255,9 @@ static void free_trace_kprobe(struct trace_kprobe *tk)
{
int i;
+ if (!tk)
+ return;
+
for (i = 0; i < tk->tp.nr_args; i++)
traceprobe_free_probe_arg(&tk->tp.args[i]);
@@ -220,9 +271,10 @@ static void free_trace_kprobe(struct trace_kprobe *tk)
static struct trace_kprobe *find_trace_kprobe(const char *event,
const char *group)
{
+ struct dyn_event *pos;
struct trace_kprobe *tk;
- list_for_each_entry(tk, &probe_list, list)
+ for_each_trace_kprobe(tk, pos)
if (strcmp(trace_event_name(&tk->tp.call), event) == 0 &&
strcmp(tk->tp.call.class->system, group) == 0)
return tk;
@@ -321,7 +373,7 @@ disable_trace_kprobe(struct trace_kprobe *tk, struct trace_event_file *file)
* created with perf_event_open. We don't need to wait for these
* trace_kprobes
*/
- if (list_empty(&tk->list))
+ if (list_empty(&tk->devent.list))
wait = 0;
out:
if (wait) {
@@ -419,7 +471,7 @@ static void __unregister_trace_kprobe(struct trace_kprobe *tk)
}
}
-/* Unregister a trace_probe and probe_event: call with locking probe_lock */
+/* Unregister a trace_probe and probe_event */
static int unregister_trace_kprobe(struct trace_kprobe *tk)
{
/* Enabled event can not be unregistered */
@@ -431,7 +483,7 @@ static int unregister_trace_kprobe(struct trace_kprobe *tk)
return -EBUSY;
__unregister_trace_kprobe(tk);
- list_del(&tk->list);
+ dyn_event_remove(&tk->devent);
return 0;
}
@@ -442,7 +494,7 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
struct trace_kprobe *old_tk;
int ret;
- mutex_lock(&probe_lock);
+ mutex_lock(&event_mutex);
/* Delete old (same name) event if exist */
old_tk = find_trace_kprobe(trace_event_name(&tk->tp.call),
@@ -471,10 +523,10 @@ static int register_trace_kprobe(struct trace_kprobe *tk)
if (ret < 0)
unregister_kprobe_event(tk);
else
- list_add_tail(&tk->list, &probe_list);
+ dyn_event_add(&tk->devent);
end:
- mutex_unlock(&probe_lock);
+ mutex_unlock(&event_mutex);
return ret;
}
@@ -483,6 +535,7 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
unsigned long val, void *data)
{
struct module *mod = data;
+ struct dyn_event *pos;
struct trace_kprobe *tk;
int ret;
@@ -490,8 +543,8 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
return NOTIFY_DONE;
/* Update probes on coming module */
- mutex_lock(&probe_lock);
- list_for_each_entry(tk, &probe_list, list) {
+ mutex_lock(&event_mutex);
+ for_each_trace_kprobe(tk, pos) {
if (trace_kprobe_within_module(tk, mod)) {
/* Don't need to check busy - this should have gone. */
__unregister_trace_kprobe(tk);
@@ -502,7 +555,7 @@ static int trace_kprobe_module_callback(struct notifier_block *nb,
mod->name, ret);
}
}
- mutex_unlock(&probe_lock);
+ mutex_unlock(&event_mutex);
return NOTIFY_DONE;
}
@@ -520,7 +573,7 @@ static inline void sanitize_event_name(char *name)
*name = '_';
}
-static int create_trace_kprobe(int argc, char **argv)
+static int trace_kprobe_create(int argc, const char *argv[])
{
/*
* Argument syntax:
@@ -544,9 +597,10 @@ static int create_trace_kprobe(int argc, char **argv)
* FETCHARG:TYPE : use TYPE instead of unsigned long.
*/
struct trace_kprobe *tk;
- int i, ret = 0;
- bool is_return = false, is_delete = false;
- char *symbol = NULL, *event = NULL, *group = NULL;
+ int i, len, ret = 0;
+ bool is_return = false;
+ char *symbol = NULL, *tmp = NULL;
+ const char *event = NULL, *group = KPROBE_EVENT_SYSTEM;
int maxactive = 0;
long offset = 0;
void *addr = NULL;
@@ -554,26 +608,26 @@ static int create_trace_kprobe(int argc, char **argv)
unsigned int flags = TPARG_FL_KERNEL;
/* argc must be >= 1 */
- if (argv[0][0] == 'p')
- is_return = false;
- else if (argv[0][0] == 'r') {
+ if (argv[0][0] == 'r') {
is_return = true;
flags |= TPARG_FL_RETURN;
- } else if (argv[0][0] == '-')
- is_delete = true;
- else {
- pr_info("Probe definition must be started with 'p', 'r' or"
- " '-'.\n");
- return -EINVAL;
- }
+ } else if (argv[0][0] != 'p' || argc < 2)
+ return -ECANCELED;
event = strchr(&argv[0][1], ':');
- if (event) {
- event[0] = '\0';
+ if (event)
event++;
- }
+
if (is_return && isdigit(argv[0][1])) {
- ret = kstrtouint(&argv[0][1], 0, &maxactive);
+ if (event)
+ len = event - &argv[0][1] - 1;
+ else
+ len = strlen(&argv[0][1]);
+ if (len > MAX_EVENT_NAME_LEN - 1)
+ return -E2BIG;
+ memcpy(buf, &argv[0][1], len);
+ buf[len] = '\0';
+ ret = kstrtouint(buf, 0, &maxactive);
if (ret) {
pr_info("Failed to parse maxactive.\n");
return ret;
@@ -588,74 +642,37 @@ static int create_trace_kprobe(int argc, char **argv)
}
}
- if (event) {
- char *slash;
-
- slash = strchr(event, '/');
- if (slash) {
- group = event;
- event = slash + 1;
- slash[0] = '\0';
- if (strlen(group) == 0) {
- pr_info("Group name is not specified\n");
- return -EINVAL;
- }
- }
- if (strlen(event) == 0) {
- pr_info("Event name is not specified\n");
- return -EINVAL;
- }
- }
- if (!group)
- group = KPROBE_EVENT_SYSTEM;
-
- if (is_delete) {
- if (!event) {
- pr_info("Delete command needs an event name.\n");
- return -EINVAL;
- }
- mutex_lock(&probe_lock);
- tk = find_trace_kprobe(event, group);
- if (!tk) {
- mutex_unlock(&probe_lock);
- pr_info("Event %s/%s doesn't exist.\n", group, event);
- return -ENOENT;
- }
- /* delete an event */
- ret = unregister_trace_kprobe(tk);
- if (ret == 0)
- free_trace_kprobe(tk);
- mutex_unlock(&probe_lock);
- return ret;
- }
-
- if (argc < 2) {
- pr_info("Probe point is not specified.\n");
- return -EINVAL;
- }
-
/* try to parse an address. if that fails, try to read the
* input as a symbol. */
if (kstrtoul(argv[1], 0, (unsigned long *)&addr)) {
+ /* Check whether uprobe event specified */
+ if (strchr(argv[1], '/') && strchr(argv[1], ':'))
+ return -ECANCELED;
/* a symbol specified */
- symbol = argv[1];
+ symbol = kstrdup(argv[1], GFP_KERNEL);
+ if (!symbol)
+ return -ENOMEM;
/* TODO: support .init module functions */
ret = traceprobe_split_symbol_offset(symbol, &offset);
if (ret || offset < 0 || offset > UINT_MAX) {
pr_info("Failed to parse either an address or a symbol.\n");
- return ret;
+ goto out;
}
if (kprobe_on_func_entry(NULL, symbol, offset))
flags |= TPARG_FL_FENTRY;
if (offset && is_return && !(flags & TPARG_FL_FENTRY)) {
pr_info("Given offset is not valid for return probe.\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
}
argc -= 2; argv += 2;
- /* setup a probe */
- if (!event) {
+ if (event) {
+ ret = traceprobe_parse_event_name(&event, &group, buf);
+ if (ret)
+ goto out;
+ } else {
/* Make a new event name */
if (symbol)
snprintf(buf, MAX_EVENT_NAME_LEN, "%c_%s_%ld",
@@ -666,17 +683,27 @@ static int create_trace_kprobe(int argc, char **argv)
sanitize_event_name(buf);
event = buf;
}
+
+ /* setup a probe */
tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive,
argc, is_return);
if (IS_ERR(tk)) {
pr_info("Failed to allocate trace_probe.(%d)\n",
(int)PTR_ERR(tk));
- return PTR_ERR(tk);
+ ret = PTR_ERR(tk);
+ goto out;
}
/* parse arguments */
for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) {
- ret = traceprobe_parse_probe_arg(&tk->tp, i, argv[i], flags);
+ tmp = kstrdup(argv[i], GFP_KERNEL);
+ if (!tmp) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ ret = traceprobe_parse_probe_arg(&tk->tp, i, tmp, flags);
+ kfree(tmp);
if (ret)
goto error;
}
@@ -684,60 +711,39 @@ static int create_trace_kprobe(int argc, char **argv)
ret = register_trace_kprobe(tk);
if (ret)
goto error;
- return 0;
+out:
+ kfree(symbol);
+ return ret;
error:
free_trace_kprobe(tk);
- return ret;
+ goto out;
}
-static int release_all_trace_kprobes(void)
+static int create_or_delete_trace_kprobe(int argc, char **argv)
{
- struct trace_kprobe *tk;
- int ret = 0;
-
- mutex_lock(&probe_lock);
- /* Ensure no probe is in use. */
- list_for_each_entry(tk, &probe_list, list)
- if (trace_probe_is_enabled(&tk->tp)) {
- ret = -EBUSY;
- goto end;
- }
- /* TODO: Use batch unregistration */
- while (!list_empty(&probe_list)) {
- tk = list_entry(probe_list.next, struct trace_kprobe, list);
- ret = unregister_trace_kprobe(tk);
- if (ret)
- goto end;
- free_trace_kprobe(tk);
- }
-
-end:
- mutex_unlock(&probe_lock);
+ int ret;
- return ret;
-}
+ if (argv[0][0] == '-')
+ return dyn_event_release(argc, argv, &trace_kprobe_ops);
-/* Probes listing interfaces */
-static void *probes_seq_start(struct seq_file *m, loff_t *pos)
-{
- mutex_lock(&probe_lock);
- return seq_list_start(&probe_list, *pos);
+ ret = trace_kprobe_create(argc, (const char **)argv);
+ return ret == -ECANCELED ? -EINVAL : ret;
}
-static void *probes_seq_next(struct seq_file *m, void *v, loff_t *pos)
+static int trace_kprobe_release(struct dyn_event *ev)
{
- return seq_list_next(v, &probe_list, pos);
-}
+ struct trace_kprobe *tk = to_trace_kprobe(ev);
+ int ret = unregister_trace_kprobe(tk);
-static void probes_seq_stop(struct seq_file *m, void *v)
-{
- mutex_unlock(&probe_lock);
+ if (!ret)
+ free_trace_kprobe(tk);
+ return ret;
}
-static int probes_seq_show(struct seq_file *m, void *v)
+static int trace_kprobe_show(struct seq_file *m, struct dyn_event *ev)
{
- struct trace_kprobe *tk = v;
+ struct trace_kprobe *tk = to_trace_kprobe(ev);
int i;
seq_putc(m, trace_kprobe_is_return(tk) ? 'r' : 'p');
@@ -759,10 +765,20 @@ static int probes_seq_show(struct seq_file *m, void *v)
return 0;
}
+static int probes_seq_show(struct seq_file *m, void *v)
+{
+ struct dyn_event *ev = v;
+
+ if (!is_trace_kprobe(ev))
+ return 0;
+
+ return trace_kprobe_show(m, ev);
+}
+
static const struct seq_operations probes_seq_op = {
- .start = probes_seq_start,
- .next = probes_seq_next,
- .stop = probes_seq_stop,
+ .start = dyn_event_seq_start,
+ .next = dyn_event_seq_next,
+ .stop = dyn_event_seq_stop,
.show = probes_seq_show
};
@@ -771,7 +787,7 @@ static int probes_open(struct inode *inode, struct file *file)
int ret;
if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) {
- ret = release_all_trace_kprobes();
+ ret = dyn_events_release_all(&trace_kprobe_ops);
if (ret < 0)
return ret;
}
@@ -783,7 +799,7 @@ static ssize_t probes_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
return trace_parse_run_command(file, buffer, count, ppos,
- create_trace_kprobe);
+ create_or_delete_trace_kprobe);
}
static const struct file_operations kprobe_events_ops = {
@@ -798,8 +814,13 @@ static const struct file_operations kprobe_events_ops = {
/* Probes profiling interfaces */
static int probes_profile_seq_show(struct seq_file *m, void *v)
{
- struct trace_kprobe *tk = v;
+ struct dyn_event *ev = v;
+ struct trace_kprobe *tk;
+ if (!is_trace_kprobe(ev))
+ return 0;
+
+ tk = to_trace_kprobe(ev);
seq_printf(m, " %-44s %15lu %15lu\n",
trace_event_name(&tk->tp.call),
trace_kprobe_nhit(tk),
@@ -809,9 +830,9 @@ static int probes_profile_seq_show(struct seq_file *m, void *v)
}
static const struct seq_operations profile_seq_op = {
- .start = probes_seq_start,
- .next = probes_seq_next,
- .stop = probes_seq_stop,
+ .start = dyn_event_seq_start,
+ .next = dyn_event_seq_next,
+ .stop = dyn_event_seq_stop,
.show = probes_profile_seq_show
};
@@ -1332,7 +1353,7 @@ static int register_kprobe_event(struct trace_kprobe *tk)
kfree(call->print_fmt);
return -ENODEV;
}
- ret = trace_add_event_call(call);
+ ret = trace_add_event_call_nolock(call);
if (ret) {
pr_info("Failed to register kprobe event: %s\n",
trace_event_name(call));
@@ -1347,7 +1368,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk)
int ret;
/* tp->event is unregistered in trace_remove_event_call() */
- ret = trace_remove_event_call(&tk->tp.call);
+ ret = trace_remove_event_call_nolock(&tk->tp.call);
if (!ret)
kfree(tk->tp.call.print_fmt);
return ret;
@@ -1364,7 +1385,7 @@ create_local_trace_kprobe(char *func, void *addr, unsigned long offs,
char *event;
/*
- * local trace_kprobes are not added to probe_list, so they are never
+ * local trace_kprobes are not added to dyn_event, so they are never
* searched in find_trace_kprobe(). Therefore, there is no concern of
* duplicated name here.
*/
@@ -1422,6 +1443,11 @@ static __init int init_kprobe_trace(void)
{
struct dentry *d_tracer;
struct dentry *entry;
+ int ret;
+
+ ret = dyn_event_register(&trace_kprobe_ops);
+ if (ret)
+ return ret;
if (register_module_notifier(&trace_kprobe_module_nb))
return -EINVAL;
@@ -1479,9 +1505,8 @@ static __init int kprobe_trace_self_tests_init(void)
pr_info("Testing kprobe tracing: ");
- ret = trace_run_command("p:testprobe kprobe_trace_selftest_target "
- "$stack $stack0 +0($stack)",
- create_trace_kprobe);
+ ret = trace_run_command("p:testprobe kprobe_trace_selftest_target $stack $stack0 +0($stack)",
+ create_or_delete_trace_kprobe);
if (WARN_ON_ONCE(ret)) {
pr_warn("error on probing function entry.\n");
warn++;
@@ -1501,8 +1526,8 @@ static __init int kprobe_trace_self_tests_init(void)
}
}
- ret = trace_run_command("r:testprobe2 kprobe_trace_selftest_target "
- "$retval", create_trace_kprobe);
+ ret = trace_run_command("r:testprobe2 kprobe_trace_selftest_target $retval",
+ create_or_delete_trace_kprobe);
if (WARN_ON_ONCE(ret)) {
pr_warn("error on probing function return.\n");
warn++;
@@ -1572,20 +1597,24 @@ static __init int kprobe_trace_self_tests_init(void)
disable_trace_kprobe(tk, file);
}
- ret = trace_run_command("-:testprobe", create_trace_kprobe);
+ ret = trace_run_command("-:testprobe", create_or_delete_trace_kprobe);
if (WARN_ON_ONCE(ret)) {
pr_warn("error on deleting a probe.\n");
warn++;
}
- ret = trace_run_command("-:testprobe2", create_trace_kprobe);
+ ret = trace_run_command("-:testprobe2", create_or_delete_trace_kprobe);
if (WARN_ON_ONCE(ret)) {
pr_warn("error on deleting a probe.\n");
warn++;
}
end:
- release_all_trace_kprobes();
+ ret = dyn_events_release_all(&trace_kprobe_ops);
+ if (WARN_ON_ONCE(ret)) {
+ pr_warn("error on cleaning up probes.\n");
+ warn++;
+ }
/*
* Wait for the optimizer work to finish. Otherwise it might fiddle
* with probes in already freed __init text.
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 449150c6a87f..ff86417c0149 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -154,6 +154,33 @@ int traceprobe_split_symbol_offset(char *symbol, long *offset)
return 0;
}
+/* @buf must has MAX_EVENT_NAME_LEN size */
+int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
+ char *buf)
+{
+ const char *slash, *event = *pevent;
+
+ slash = strchr(event, '/');
+ if (slash) {
+ if (slash == event) {
+ pr_info("Group name is not specified\n");
+ return -EINVAL;
+ }
+ if (slash - event + 1 > MAX_EVENT_NAME_LEN) {
+ pr_info("Group name is too long\n");
+ return -E2BIG;
+ }
+ strlcpy(buf, event, slash - event + 1);
+ *pgroup = buf;
+ *pevent = slash + 1;
+ }
+ if (strlen(event) == 0) {
+ pr_info("Event name is not specified\n");
+ return -EINVAL;
+ }
+ return 0;
+}
+
#define PARAM_MAX_STACK (THREAD_SIZE / sizeof(unsigned long))
static int parse_probe_vars(char *arg, const struct fetch_type *t,
diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
index feeec261b356..8a63f8bc01bc 100644
--- a/kernel/trace/trace_probe.h
+++ b/kernel/trace/trace_probe.h
@@ -279,6 +279,8 @@ extern int traceprobe_update_arg(struct probe_arg *arg);
extern void traceprobe_free_probe_arg(struct probe_arg *arg);
extern int traceprobe_split_symbol_offset(char *symbol, long *offset);
+extern int traceprobe_parse_event_name(const char **pevent,
+ const char **pgroup, char *buf);
extern int traceprobe_set_print_fmt(struct trace_probe *tp, bool is_return);
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 07/12] tracing/uprobes: Use dyn_event framework for uprobe events
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
` (5 preceding siblings ...)
2018-11-05 9:02 ` [PATCH v2 06/12] tracing/kprobes: Use dyn_event framework for kprobe events Masami Hiramatsu
@ 2018-11-05 9:03 ` Masami Hiramatsu
2018-11-05 9:03 ` [PATCH v2 08/12] tracing: Use dyn_event framework for synthetic events Masami Hiramatsu
` (5 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:03 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Use dyn_event framework for uprobe events. This shows
uprobe events on "dynamic_events" file.
User can also define new uprobe events via dynamic_events.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
Changes in v2:
- Use dyn_events_release_all() for clearing events.
- Use nolock event_call register/unregister and lock event_mutex
before dyn_event_mutex to avoid lock dependency error.
- Select CONFIG_DYNAMIC_EVENTS in Kconfig
- Add match operation and use new API for event deletion
- Return -ECANCELED if given event is not for uprobe.
- Remove unused for_each_trace_uprobe_safe.
---
Documentation/trace/uprobetracer.rst | 4
kernel/trace/Kconfig | 1
kernel/trace/trace_uprobe.c | 278 ++++++++++++++++++----------------
3 files changed, 153 insertions(+), 130 deletions(-)
diff --git a/Documentation/trace/uprobetracer.rst b/Documentation/trace/uprobetracer.rst
index d0822811527a..4c3bfde2ba47 100644
--- a/Documentation/trace/uprobetracer.rst
+++ b/Documentation/trace/uprobetracer.rst
@@ -18,6 +18,10 @@ current_tracer. Instead of that, add probe points via
However unlike kprobe-event tracer, the uprobe event interface expects the
user to calculate the offset of the probepoint in the object.
+You can also use /sys/kernel/debug/tracing/dynamic_events instead of
+uprobe_events. That interface will provide unified access to other
+dynamic events too.
+
Synopsis of uprobe_tracer
-------------------------
::
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index c0f6b0105609..2cab3c5dfe2c 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -501,6 +501,7 @@ config UPROBE_EVENTS
depends on PERF_EVENTS
select UPROBES
select PROBE_EVENTS
+ select DYNAMIC_EVENTS
select TRACING
default y
help
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 6eaaa2150685..4a7b21c891f3 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -7,6 +7,7 @@
*/
#define pr_fmt(fmt) "trace_kprobe: " fmt
+#include <linux/ctype.h>
#include <linux/module.h>
#include <linux/uaccess.h>
#include <linux/uprobes.h>
@@ -14,6 +15,7 @@
#include <linux/string.h>
#include <linux/rculist.h>
+#include "trace_dynevent.h"
#include "trace_probe.h"
#include "trace_probe_tmpl.h"
@@ -37,11 +39,26 @@ struct trace_uprobe_filter {
struct list_head perf_events;
};
+static int trace_uprobe_create(int argc, const char **argv);
+static int trace_uprobe_show(struct seq_file *m, struct dyn_event *ev);
+static int trace_uprobe_release(struct dyn_event *ev);
+static bool trace_uprobe_is_busy(struct dyn_event *ev);
+static bool trace_uprobe_match(const char *system, const char *event,
+ struct dyn_event *ev);
+
+static struct dyn_event_operations trace_uprobe_ops = {
+ .create = trace_uprobe_create,
+ .show = trace_uprobe_show,
+ .is_busy = trace_uprobe_is_busy,
+ .free = trace_uprobe_release,
+ .match = trace_uprobe_match,
+};
+
/*
* uprobe event core functions
*/
struct trace_uprobe {
- struct list_head list;
+ struct dyn_event devent;
struct trace_uprobe_filter filter;
struct uprobe_consumer consumer;
struct path path;
@@ -53,6 +70,25 @@ struct trace_uprobe {
struct trace_probe tp;
};
+static bool is_trace_uprobe(struct dyn_event *ev)
+{
+ return ev->ops == &trace_uprobe_ops;
+}
+
+static struct trace_uprobe *to_trace_uprobe(struct dyn_event *ev)
+{
+ return container_of(ev, struct trace_uprobe, devent);
+}
+
+/**
+ * for_each_trace_uprobe - iterate over the trace_uprobe list
+ * @pos: the struct trace_uprobe * for each entry
+ * @dpos: the struct dyn_event * to use as a loop cursor
+ */
+#define for_each_trace_uprobe(pos, dpos) \
+ for_each_dyn_event(dpos) \
+ if (is_trace_uprobe(dpos) && (pos = to_trace_uprobe(dpos)))
+
#define SIZEOF_TRACE_UPROBE(n) \
(offsetof(struct trace_uprobe, tp.args) + \
(sizeof(struct probe_arg) * (n)))
@@ -60,9 +96,6 @@ struct trace_uprobe {
static int register_uprobe_event(struct trace_uprobe *tu);
static int unregister_uprobe_event(struct trace_uprobe *tu);
-static DEFINE_MUTEX(uprobe_lock);
-static LIST_HEAD(uprobe_list);
-
struct uprobe_dispatch_data {
struct trace_uprobe *tu;
unsigned long bp_addr;
@@ -209,6 +242,22 @@ static inline bool is_ret_probe(struct trace_uprobe *tu)
return tu->consumer.ret_handler != NULL;
}
+static bool trace_uprobe_is_busy(struct dyn_event *ev)
+{
+ struct trace_uprobe *tu = to_trace_uprobe(ev);
+
+ return trace_probe_is_enabled(&tu->tp);
+}
+
+static bool trace_uprobe_match(const char *system, const char *event,
+ struct dyn_event *ev)
+{
+ struct trace_uprobe *tu = to_trace_uprobe(ev);
+
+ return strcmp(trace_event_name(&tu->tp.call), event) == 0 &&
+ (!system || strcmp(tu->tp.call.class->system, system) == 0);
+}
+
/*
* Allocate new trace_uprobe and initialize it (including uprobes).
*/
@@ -236,7 +285,7 @@ alloc_trace_uprobe(const char *group, const char *event, int nargs, bool is_ret)
if (!tu->tp.class.system)
goto error;
- INIT_LIST_HEAD(&tu->list);
+ dyn_event_init(&tu->devent, &trace_uprobe_ops);
INIT_LIST_HEAD(&tu->tp.files);
tu->consumer.handler = uprobe_dispatcher;
if (is_ret)
@@ -255,6 +304,9 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
{
int i;
+ if (!tu)
+ return;
+
for (i = 0; i < tu->tp.nr_args; i++)
traceprobe_free_probe_arg(&tu->tp.args[i]);
@@ -267,9 +319,10 @@ static void free_trace_uprobe(struct trace_uprobe *tu)
static struct trace_uprobe *find_probe_event(const char *event, const char *group)
{
+ struct dyn_event *pos;
struct trace_uprobe *tu;
- list_for_each_entry(tu, &uprobe_list, list)
+ for_each_trace_uprobe(tu, pos)
if (strcmp(trace_event_name(&tu->tp.call), event) == 0 &&
strcmp(tu->tp.call.class->system, group) == 0)
return tu;
@@ -277,7 +330,7 @@ static struct trace_uprobe *find_probe_event(const char *event, const char *grou
return NULL;
}
-/* Unregister a trace_uprobe and probe_event: call with locking uprobe_lock */
+/* Unregister a trace_uprobe and probe_event */
static int unregister_trace_uprobe(struct trace_uprobe *tu)
{
int ret;
@@ -286,7 +339,7 @@ static int unregister_trace_uprobe(struct trace_uprobe *tu)
if (ret)
return ret;
- list_del(&tu->list);
+ dyn_event_remove(&tu->devent);
free_trace_uprobe(tu);
return 0;
}
@@ -302,13 +355,14 @@ static int unregister_trace_uprobe(struct trace_uprobe *tu)
*/
static struct trace_uprobe *find_old_trace_uprobe(struct trace_uprobe *new)
{
+ struct dyn_event *pos;
struct trace_uprobe *tmp, *old = NULL;
struct inode *new_inode = d_real_inode(new->path.dentry);
old = find_probe_event(trace_event_name(&new->tp.call),
new->tp.call.class->system);
- list_for_each_entry(tmp, &uprobe_list, list) {
+ for_each_trace_uprobe(tmp, pos) {
if ((old ? old != tmp : true) &&
new_inode == d_real_inode(tmp->path.dentry) &&
new->offset == tmp->offset &&
@@ -326,7 +380,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
struct trace_uprobe *old_tu;
int ret;
- mutex_lock(&uprobe_lock);
+ mutex_lock(&event_mutex);
/* register as an event */
old_tu = find_old_trace_uprobe(tu);
@@ -348,10 +402,10 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
goto end;
}
- list_add_tail(&tu->list, &uprobe_list);
+ dyn_event_add(&tu->devent);
end:
- mutex_unlock(&uprobe_lock);
+ mutex_unlock(&event_mutex);
return ret;
}
@@ -362,91 +416,49 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
*
* - Remove uprobe: -:[GRP/]EVENT
*/
-static int create_trace_uprobe(int argc, char **argv)
+static int trace_uprobe_create(int argc, const char **argv)
{
struct trace_uprobe *tu;
- char *arg, *event, *group, *filename, *rctr, *rctr_end;
+ const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
+ char *arg, *filename, *rctr, *rctr_end, *tmp;
char buf[MAX_EVENT_NAME_LEN];
struct path path;
unsigned long offset, ref_ctr_offset;
- bool is_delete, is_return;
+ bool is_return = false;
int i, ret;
ret = 0;
- is_delete = false;
- is_return = false;
- event = NULL;
- group = NULL;
ref_ctr_offset = 0;
/* argc must be >= 1 */
- if (argv[0][0] == '-')
- is_delete = true;
- else if (argv[0][0] == 'r')
+ if (argv[0][0] == 'r')
is_return = true;
- else if (argv[0][0] != 'p') {
- pr_info("Probe definition must be started with 'p', 'r' or '-'.\n");
- return -EINVAL;
- }
+ else if (argv[0][0] != 'p' || argc < 2)
+ return -ECANCELED;
- if (argv[0][1] == ':') {
+ if (argv[0][1] == ':')
event = &argv[0][2];
- arg = strchr(event, '/');
- if (arg) {
- group = event;
- event = arg + 1;
- event[-1] = '\0';
+ if (!strchr(argv[1], '/'))
+ return -ECANCELED;
- if (strlen(group) == 0) {
- pr_info("Group name is not specified\n");
- return -EINVAL;
- }
- }
- if (strlen(event) == 0) {
- pr_info("Event name is not specified\n");
- return -EINVAL;
- }
- }
- if (!group)
- group = UPROBE_EVENT_SYSTEM;
-
- if (is_delete) {
- int ret;
-
- if (!event) {
- pr_info("Delete command needs an event name.\n");
- return -EINVAL;
- }
- mutex_lock(&uprobe_lock);
- tu = find_probe_event(event, group);
-
- if (!tu) {
- mutex_unlock(&uprobe_lock);
- pr_info("Event %s/%s doesn't exist.\n", group, event);
- return -ENOENT;
- }
- /* delete an event */
- ret = unregister_trace_uprobe(tu);
- mutex_unlock(&uprobe_lock);
- return ret;
- }
+ filename = kstrdup(argv[1], GFP_KERNEL);
+ if (!filename)
+ return -ENOMEM;
- if (argc < 2) {
- pr_info("Probe point is not specified.\n");
- return -EINVAL;
- }
/* Find the last occurrence, in case the path contains ':' too. */
- arg = strrchr(argv[1], ':');
- if (!arg)
- return -EINVAL;
+ arg = strrchr(filename, ':');
+ if (!arg || !isdigit(arg[1])) {
+ kfree(filename);
+ return -ECANCELED;
+ }
*arg++ = '\0';
- filename = argv[1];
ret = kern_path(filename, LOOKUP_FOLLOW, &path);
- if (ret)
+ if (ret) {
+ kfree(filename);
return ret;
-
+ }
if (!d_is_reg(path.dentry)) {
ret = -EINVAL;
goto fail_address_parse;
@@ -480,7 +492,11 @@ static int create_trace_uprobe(int argc, char **argv)
argv += 2;
/* setup a probe */
- if (!event) {
+ if (event) {
+ ret = traceprobe_parse_event_name(&event, &group, buf);
+ if (ret)
+ goto fail_address_parse;
+ } else {
char *tail;
char *ptr;
@@ -508,18 +524,19 @@ static int create_trace_uprobe(int argc, char **argv)
tu->offset = offset;
tu->ref_ctr_offset = ref_ctr_offset;
tu->path = path;
- tu->filename = kstrdup(filename, GFP_KERNEL);
-
- if (!tu->filename) {
- pr_info("Failed to allocate filename.\n");
- ret = -ENOMEM;
- goto error;
- }
+ tu->filename = filename;
/* parse arguments */
for (i = 0; i < argc && i < MAX_TRACE_ARGS; i++) {
- ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i],
+ tmp = kstrdup(argv[i], GFP_KERNEL);
+ if (!tmp) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ ret = traceprobe_parse_probe_arg(&tu->tp, i, tmp,
is_return ? TPARG_FL_RETURN : 0);
+ kfree(tmp);
if (ret)
goto error;
}
@@ -535,55 +552,35 @@ static int create_trace_uprobe(int argc, char **argv)
fail_address_parse:
path_put(&path);
+ kfree(filename);
pr_info("Failed to parse address or file.\n");
return ret;
}
-static int cleanup_all_probes(void)
+static int create_or_delete_trace_uprobe(int argc, char **argv)
{
- struct trace_uprobe *tu;
- int ret = 0;
+ int ret;
- mutex_lock(&uprobe_lock);
- /* Ensure no probe is in use. */
- list_for_each_entry(tu, &uprobe_list, list)
- if (trace_probe_is_enabled(&tu->tp)) {
- ret = -EBUSY;
- goto end;
- }
- while (!list_empty(&uprobe_list)) {
- tu = list_entry(uprobe_list.next, struct trace_uprobe, list);
- ret = unregister_trace_uprobe(tu);
- if (ret)
- break;
- }
-end:
- mutex_unlock(&uprobe_lock);
- return ret;
-}
+ if (argv[0][0] == '-')
+ return dyn_event_release(argc, argv, &trace_uprobe_ops);
-/* Probes listing interfaces */
-static void *probes_seq_start(struct seq_file *m, loff_t *pos)
-{
- mutex_lock(&uprobe_lock);
- return seq_list_start(&uprobe_list, *pos);
+ ret = trace_uprobe_create(argc, (const char **)argv);
+ return ret == -ECANCELED ? -EINVAL : ret;
}
-static void *probes_seq_next(struct seq_file *m, void *v, loff_t *pos)
+static int trace_uprobe_release(struct dyn_event *ev)
{
- return seq_list_next(v, &uprobe_list, pos);
-}
+ struct trace_uprobe *tu = to_trace_uprobe(ev);
-static void probes_seq_stop(struct seq_file *m, void *v)
-{
- mutex_unlock(&uprobe_lock);
+ return unregister_trace_uprobe(tu);
}
-static int probes_seq_show(struct seq_file *m, void *v)
+/* Probes listing interfaces */
+static int trace_uprobe_show(struct seq_file *m, struct dyn_event *ev)
{
- struct trace_uprobe *tu = v;
+ struct trace_uprobe *tu = to_trace_uprobe(ev);
char c = is_ret_probe(tu) ? 'r' : 'p';
int i;
@@ -601,11 +598,21 @@ static int probes_seq_show(struct seq_file *m, void *v)
return 0;
}
+static int probes_seq_show(struct seq_file *m, void *v)
+{
+ struct dyn_event *ev = v;
+
+ if (!is_trace_uprobe(ev))
+ return 0;
+
+ return trace_uprobe_show(m, ev);
+}
+
static const struct seq_operations probes_seq_op = {
- .start = probes_seq_start,
- .next = probes_seq_next,
- .stop = probes_seq_stop,
- .show = probes_seq_show
+ .start = dyn_event_seq_start,
+ .next = dyn_event_seq_next,
+ .stop = dyn_event_seq_stop,
+ .show = probes_seq_show
};
static int probes_open(struct inode *inode, struct file *file)
@@ -613,7 +620,7 @@ static int probes_open(struct inode *inode, struct file *file)
int ret;
if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) {
- ret = cleanup_all_probes();
+ ret = dyn_events_release_all(&trace_uprobe_ops);
if (ret)
return ret;
}
@@ -624,7 +631,8 @@ static int probes_open(struct inode *inode, struct file *file)
static ssize_t probes_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
- return trace_parse_run_command(file, buffer, count, ppos, create_trace_uprobe);
+ return trace_parse_run_command(file, buffer, count, ppos,
+ create_or_delete_trace_uprobe);
}
static const struct file_operations uprobe_events_ops = {
@@ -639,17 +647,22 @@ static const struct file_operations uprobe_events_ops = {
/* Probes profiling interfaces */
static int probes_profile_seq_show(struct seq_file *m, void *v)
{
- struct trace_uprobe *tu = v;
+ struct dyn_event *ev = v;
+ struct trace_uprobe *tu;
+
+ if (!is_trace_uprobe(ev))
+ return 0;
+ tu = to_trace_uprobe(ev);
seq_printf(m, " %s %-44s %15lu\n", tu->filename,
trace_event_name(&tu->tp.call), tu->nhit);
return 0;
}
static const struct seq_operations profile_seq_op = {
- .start = probes_seq_start,
- .next = probes_seq_next,
- .stop = probes_seq_stop,
+ .start = dyn_event_seq_start,
+ .next = dyn_event_seq_next,
+ .stop = dyn_event_seq_stop,
.show = probes_profile_seq_show
};
@@ -1307,7 +1320,7 @@ static int register_uprobe_event(struct trace_uprobe *tu)
return -ENODEV;
}
- ret = trace_add_event_call(call);
+ ret = trace_add_event_call_nolock(call);
if (ret) {
pr_info("Failed to register uprobe event: %s\n",
@@ -1324,7 +1337,7 @@ static int unregister_uprobe_event(struct trace_uprobe *tu)
int ret;
/* tu->event is unregistered in trace_remove_event_call() */
- ret = trace_remove_event_call(&tu->tp.call);
+ ret = trace_remove_event_call_nolock(&tu->tp.call);
if (ret)
return ret;
kfree(tu->tp.call.print_fmt);
@@ -1351,7 +1364,7 @@ create_local_trace_uprobe(char *name, unsigned long offs,
}
/*
- * local trace_kprobes are not added to probe_list, so they are never
+ * local trace_kprobes are not added to dyn_event, so they are never
* searched in find_trace_kprobe(). Therefore, there is no concern of
* duplicated name "DUMMY_EVENT" here.
*/
@@ -1399,6 +1412,11 @@ void destroy_local_trace_uprobe(struct trace_event_call *event_call)
static __init int init_uprobe_trace(void)
{
struct dentry *d_tracer;
+ int ret;
+
+ ret = dyn_event_register(&trace_uprobe_ops);
+ if (ret)
+ return ret;
d_tracer = tracing_init_dentry();
if (IS_ERR(d_tracer))
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 08/12] tracing: Use dyn_event framework for synthetic events
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
` (6 preceding siblings ...)
2018-11-05 9:03 ` [PATCH v2 07/12] tracing/uprobes: Use dyn_event framework for uprobe events Masami Hiramatsu
@ 2018-11-05 9:03 ` Masami Hiramatsu
2018-11-05 9:04 ` [PATCH v2 09/12] tracing: Remove unneeded synth_event_mutex Masami Hiramatsu
` (4 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:03 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Use dyn_event framework for synthetic events. This shows
synthetic events on "tracing/dynamic_events" file in addition
to tracing/synthetic_events interface.
User can also define new events via tracing/dynamic_events
with "s:" prefix. So, the new syntax is below;
s:[synthetic/]EVENT_NAME TYPE ARG; [TYPE ARG;]...
To remove events via tracing/dynamic_events, you can use
"-:" prefix as same as other events.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
kernel/trace/Kconfig | 1
kernel/trace/trace.c | 8 +
kernel/trace/trace_events_hist.c | 265 ++++++++++++++++++++++++--------------
3 files changed, 176 insertions(+), 98 deletions(-)
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 2cab3c5dfe2c..fa8b1fe824f3 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -635,6 +635,7 @@ config HIST_TRIGGERS
depends on ARCH_HAVE_NMI_SAFE_CMPXCHG
select TRACING_MAP
select TRACING
+ select DYNAMIC_EVENTS
default n
help
Hist triggers allow one or more arbitrary trace event fields
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 2886e92e8eab..5e71d0c3373c 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4620,6 +4620,9 @@ static const char readme_msg[] =
"\t accepts: event-definitions (one definition per line)\n"
"\t Format: p[:[<group>/]<event>] <place> [<args>]\n"
"\t r[maxactive][:[<group>/]<event>] <place> [<args>]\n"
+#ifdef CONFIG_HIST_TRIGGERS
+ "\t s:[synthetic/]<event> <field> [<field>]\n"
+#endif
"\t -:[<group>/]<event>\n"
#ifdef CONFIG_KPROBE_EVENTS
"\t place: [<module>:]<symbol>[+<offset>]|<memaddr>\n"
@@ -4638,6 +4641,11 @@ static const char readme_msg[] =
"\t type: s8/16/32/64, u8/16/32/64, x8/16/32/64, string, symbol,\n"
"\t b<bit-width>@<bit-offset>/<container-size>,\n"
"\t <type>\\[<array-size>\\]\n"
+#ifdef CONFIG_HIST_TRIGGERS
+ "\t field: <stype> <name>;\n"
+ "\t stype: u8/u16/u32/u64, s8/s16/s32/s64, pid_t,\n"
+ "\t [unsigned] char/int/long\n"
+#endif
#endif
" events/\t\t- Directory containing all trace event subsystems:\n"
" enable\t\t- Write 0/1 to enable/disable tracing of all events\n"
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 0feb7f460123..414aabd67d1f 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -15,6 +15,7 @@
#include "tracing_map.h"
#include "trace.h"
+#include "trace_dynevent.h"
#define SYNTH_SYSTEM "synthetic"
#define SYNTH_FIELDS_MAX 16
@@ -292,6 +293,21 @@ struct hist_trigger_data {
unsigned int n_max_var_str;
};
+static int synth_event_create(int argc, const char **argv);
+static int synth_event_show(struct seq_file *m, struct dyn_event *ev);
+static int synth_event_release(struct dyn_event *ev);
+static bool synth_event_is_busy(struct dyn_event *ev);
+static bool synth_event_match(const char *system, const char *event,
+ struct dyn_event *ev);
+
+static struct dyn_event_operations synth_event_ops = {
+ .create = synth_event_create,
+ .show = synth_event_show,
+ .is_busy = synth_event_is_busy,
+ .free = synth_event_release,
+ .match = synth_event_match,
+};
+
struct synth_field {
char *type;
char *name;
@@ -301,7 +317,7 @@ struct synth_field {
};
struct synth_event {
- struct list_head list;
+ struct dyn_event devent;
int ref;
char *name;
struct synth_field **fields;
@@ -312,6 +328,32 @@ struct synth_event {
struct tracepoint *tp;
};
+static bool is_synth_event(struct dyn_event *ev)
+{
+ return ev->ops == &synth_event_ops;
+}
+
+static struct synth_event *to_synth_event(struct dyn_event *ev)
+{
+ return container_of(ev, struct synth_event, devent);
+}
+
+static bool synth_event_is_busy(struct dyn_event *ev)
+{
+ struct synth_event *event = to_synth_event(ev);
+
+ return event->ref != 0;
+}
+
+static bool synth_event_match(const char *system, const char *event,
+ struct dyn_event *ev)
+{
+ struct synth_event *sev = to_synth_event(ev);
+
+ return strcmp(sev->name, event) == 0 &&
+ (!system || strcmp(system, SYNTH_SYSTEM) == 0);
+}
+
struct action_data;
typedef void (*action_fn_t) (struct hist_trigger_data *hist_data,
@@ -402,7 +444,6 @@ static bool have_hist_err(void)
return false;
}
-static LIST_HEAD(synth_event_list);
static DEFINE_MUTEX(synth_event_mutex);
struct synth_trace_event {
@@ -738,14 +779,12 @@ static void free_synth_field(struct synth_field *field)
kfree(field);
}
-static struct synth_field *parse_synth_field(int argc, char **argv,
+static struct synth_field *parse_synth_field(int argc, const char **argv,
int *consumed)
{
struct synth_field *field;
- const char *prefix = NULL;
- char *field_type = argv[0], *field_name;
+ const char *prefix = NULL, *field_type = argv[0], *field_name, *array;
int len, ret = 0;
- char *array;
if (field_type[0] == ';')
field_type++;
@@ -762,20 +801,31 @@ static struct synth_field *parse_synth_field(int argc, char **argv,
*consumed = 2;
}
- len = strlen(field_name);
- if (field_name[len - 1] == ';')
- field_name[len - 1] = '\0';
-
field = kzalloc(sizeof(*field), GFP_KERNEL);
if (!field)
return ERR_PTR(-ENOMEM);
- len = strlen(field_type) + 1;
+ len = strlen(field_name);
array = strchr(field_name, '[');
if (array)
+ len -= strlen(array);
+ else if (field_name[len - 1] == ';')
+ len--;
+
+ field->name = kmemdup_nul(field_name, len, GFP_KERNEL);
+ if (!field->name) {
+ ret = -ENOMEM;
+ goto free;
+ }
+
+ if (field_type[0] == ';')
+ field_type++;
+ len = strlen(field_type) + 1;
+ if (array)
len += strlen(array);
if (prefix)
len += strlen(prefix);
+
field->type = kzalloc(len, GFP_KERNEL);
if (!field->type) {
ret = -ENOMEM;
@@ -786,7 +836,8 @@ static struct synth_field *parse_synth_field(int argc, char **argv,
strcat(field->type, field_type);
if (array) {
strcat(field->type, array);
- *array = '\0';
+ if (field->type[len - 1] == ';')
+ field->type[len - 1] = '\0';
}
field->size = synth_field_size(field->type);
@@ -800,11 +851,6 @@ static struct synth_field *parse_synth_field(int argc, char **argv,
field->is_signed = synth_field_signed(field->type);
- field->name = kstrdup(field_name, GFP_KERNEL);
- if (!field->name) {
- ret = -ENOMEM;
- goto free;
- }
out:
return field;
free:
@@ -868,9 +914,13 @@ static inline void trace_synth(struct synth_event *event, u64 *var_ref_vals,
static struct synth_event *find_synth_event(const char *name)
{
+ struct dyn_event *pos;
struct synth_event *event;
- list_for_each_entry(event, &synth_event_list, list) {
+ for_each_dyn_event(pos) {
+ if (!is_synth_event(pos))
+ continue;
+ event = to_synth_event(pos);
if (strcmp(event->name, name) == 0)
return event;
}
@@ -921,7 +971,7 @@ static int register_synth_event(struct synth_event *event)
ret = set_synth_event_print_fmt(call);
if (ret < 0) {
- trace_remove_event_call(call);
+ trace_remove_event_call_nolock(call);
goto err;
}
out:
@@ -959,7 +1009,7 @@ static void free_synth_event(struct synth_event *event)
kfree(event);
}
-static struct synth_event *alloc_synth_event(char *event_name, int n_fields,
+static struct synth_event *alloc_synth_event(const char *name, int n_fields,
struct synth_field **fields)
{
struct synth_event *event;
@@ -971,7 +1021,7 @@ static struct synth_event *alloc_synth_event(char *event_name, int n_fields,
goto out;
}
- event->name = kstrdup(event_name, GFP_KERNEL);
+ event->name = kstrdup(name, GFP_KERNEL);
if (!event->name) {
kfree(event);
event = ERR_PTR(-ENOMEM);
@@ -985,6 +1035,8 @@ static struct synth_event *alloc_synth_event(char *event_name, int n_fields,
goto out;
}
+ dyn_event_init(&event->devent, &synth_event_ops);
+
for (i = 0; i < n_fields; i++)
event->fields[i] = fields[i];
@@ -1008,16 +1060,11 @@ struct hist_var_data {
struct hist_trigger_data *hist_data;
};
-static int create_synth_event(int argc, char **argv)
+static int __create_synth_event(int argc, const char *name, const char **argv)
{
struct synth_field *field, *fields[SYNTH_FIELDS_MAX];
struct synth_event *event = NULL;
- bool delete_event = false;
int i, consumed = 0, n_fields = 0, ret = 0;
- char *name;
-
- mutex_lock(&event_mutex);
- mutex_lock(&synth_event_mutex);
/*
* Argument syntax:
@@ -1025,43 +1072,20 @@ static int create_synth_event(int argc, char **argv)
* - Remove synthetic event: !<event_name> field[;field] ...
* where 'field' = type field_name
*/
- if (argc < 1) {
- ret = -EINVAL;
- goto out;
- }
- name = argv[0];
- if (name[0] == '!') {
- delete_event = true;
- name++;
- }
+ if (name[0] == '\0' || argc < 1)
+ return -EINVAL;
+
+ mutex_lock(&event_mutex);
+ mutex_lock(&synth_event_mutex);
event = find_synth_event(name);
if (event) {
- if (delete_event) {
- if (event->ref) {
- ret = -EBUSY;
- goto out;
- }
- ret = unregister_synth_event(event);
- if (!ret) {
- list_del(&event->list);
- free_synth_event(event);
- }
- } else
- ret = -EEXIST;
- goto out;
- } else if (delete_event) {
- ret = -ENOENT;
+ ret = -EEXIST;
goto out;
}
- if (argc < 2) {
- ret = -EINVAL;
- goto out;
- }
-
- for (i = 1; i < argc - 1; i++) {
+ for (i = 0; i < argc - 1; i++) {
if (strcmp(argv[i], ";") == 0)
continue;
if (n_fields == SYNTH_FIELDS_MAX) {
@@ -1091,7 +1115,7 @@ static int create_synth_event(int argc, char **argv)
}
ret = register_synth_event(event);
if (!ret)
- list_add(&event->list, &synth_event_list);
+ dyn_event_add(&event->devent);
else
free_synth_event(event);
out:
@@ -1106,57 +1130,77 @@ static int create_synth_event(int argc, char **argv)
goto out;
}
-static int release_all_synth_events(void)
+static int create_or_delete_synth_event(int argc, char **argv)
{
- struct synth_event *event, *e;
- int ret = 0;
-
- mutex_lock(&event_mutex);
- mutex_lock(&synth_event_mutex);
-
- list_for_each_entry(event, &synth_event_list, list) {
- if (event->ref) {
- mutex_unlock(&synth_event_mutex);
- return -EBUSY;
- }
- }
+ const char *name = argv[0];
+ struct synth_event *event = NULL;
+ int ret;
- list_for_each_entry_safe(event, e, &synth_event_list, list) {
- ret = unregister_synth_event(event);
- if (!ret) {
- list_del(&event->list);
- free_synth_event(event);
+ /* trace_run_command() ensures argc != 0 */
+ if (name[0] == '!') {
+ mutex_lock(&event_mutex);
+ mutex_lock(&synth_event_mutex);
+ event = find_synth_event(name + 1);
+ if (event) {
+ if (event->ref)
+ ret = -EBUSY;
+ else {
+ ret = unregister_synth_event(event);
+ if (!ret) {
+ dyn_event_remove(&event->devent);
+ free_synth_event(event);
+ }
+ }
} else
- break;
+ ret = -ENOENT;
+ mutex_unlock(&synth_event_mutex);
+ mutex_unlock(&event_mutex);
+ return ret;
}
- mutex_unlock(&synth_event_mutex);
- mutex_unlock(&event_mutex);
- return ret;
+ ret = __create_synth_event(argc - 1, name, (const char **)argv + 1);
+ return ret == -ECANCELED ? -EINVAL : ret;
}
-
-static void *synth_events_seq_start(struct seq_file *m, loff_t *pos)
+static int synth_event_create(int argc, const char **argv)
{
- mutex_lock(&synth_event_mutex);
+ const char *name = argv[0];
+ int len;
- return seq_list_start(&synth_event_list, *pos);
-}
+ if (name[0] != 's' || name[1] != ':')
+ return -ECANCELED;
+ name += 2;
-static void *synth_events_seq_next(struct seq_file *m, void *v, loff_t *pos)
-{
- return seq_list_next(v, &synth_event_list, pos);
+ /* This interface accepts group name prefix */
+ if (strchr(name, '/')) {
+ len = sizeof(SYNTH_SYSTEM "/") - 1;
+ if (strncmp(name, SYNTH_SYSTEM "/", len))
+ return -EINVAL;
+ name += len;
+ }
+ return __create_synth_event(argc - 1, name, argv + 1);
}
-static void synth_events_seq_stop(struct seq_file *m, void *v)
+static int synth_event_release(struct dyn_event *ev)
{
- mutex_unlock(&synth_event_mutex);
+ struct synth_event *event = to_synth_event(ev);
+ int ret;
+
+ if (event->ref)
+ return -EBUSY;
+
+ ret = unregister_synth_event(event);
+ if (ret)
+ return ret;
+
+ dyn_event_remove(ev);
+ free_synth_event(event);
+ return 0;
}
-static int synth_events_seq_show(struct seq_file *m, void *v)
+static int __synth_event_show(struct seq_file *m, struct synth_event *event)
{
struct synth_field *field;
- struct synth_event *event = v;
unsigned int i;
seq_printf(m, "%s\t", event->name);
@@ -1174,11 +1218,30 @@ static int synth_events_seq_show(struct seq_file *m, void *v)
return 0;
}
+static int synth_event_show(struct seq_file *m, struct dyn_event *ev)
+{
+ struct synth_event *event = to_synth_event(ev);
+
+ seq_printf(m, "s:%s/", event->class.system);
+
+ return __synth_event_show(m, event);
+}
+
+static int synth_events_seq_show(struct seq_file *m, void *v)
+{
+ struct dyn_event *ev = v;
+
+ if (!is_synth_event(ev))
+ return 0;
+
+ return __synth_event_show(m, to_synth_event(ev));
+}
+
static const struct seq_operations synth_events_seq_op = {
- .start = synth_events_seq_start,
- .next = synth_events_seq_next,
- .stop = synth_events_seq_stop,
- .show = synth_events_seq_show
+ .start = dyn_event_seq_start,
+ .next = dyn_event_seq_next,
+ .stop = dyn_event_seq_stop,
+ .show = synth_events_seq_show,
};
static int synth_events_open(struct inode *inode, struct file *file)
@@ -1186,7 +1249,7 @@ static int synth_events_open(struct inode *inode, struct file *file)
int ret;
if ((file->f_mode & FMODE_WRITE) && (file->f_flags & O_TRUNC)) {
- ret = release_all_synth_events();
+ ret = dyn_events_release_all(&synth_event_ops);
if (ret < 0)
return ret;
}
@@ -1199,7 +1262,7 @@ static ssize_t synth_events_write(struct file *file,
size_t count, loff_t *ppos)
{
return trace_parse_run_command(file, buffer, count, ppos,
- create_synth_event);
+ create_or_delete_synth_event);
}
static const struct file_operations synth_events_fops = {
@@ -5791,6 +5854,12 @@ static __init int trace_events_hist_init(void)
struct dentry *d_tracer;
int err = 0;
+ err = dyn_event_register(&synth_event_ops);
+ if (err) {
+ pr_warn("Could not register synth_event_ops\n");
+ return err;
+ }
+
d_tracer = tracing_init_dentry();
if (IS_ERR(d_tracer)) {
err = PTR_ERR(d_tracer);
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 09/12] tracing: Remove unneeded synth_event_mutex
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
` (7 preceding siblings ...)
2018-11-05 9:03 ` [PATCH v2 08/12] tracing: Use dyn_event framework for synthetic events Masami Hiramatsu
@ 2018-11-05 9:04 ` Masami Hiramatsu
2018-11-05 9:04 ` [PATCH v2 10/12] tracing: Remove orphaned trace_add/remove_event_call functions Masami Hiramatsu
` (3 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:04 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Rmove unneeded synth_event_mutex. This mutex protects the reference
count in synth_event, however, those operational points are already
protected by event_mutex.
1. In __create_synth_event() and create_or_delete_synth_event(),
those synth_event_mutex clearly obtained right after event_mutex.
2. event_hist_trigger_func() is trigger_hist_cmd.func() which is
called by trigger_process_regex(), which is a part of
event_trigger_regex_write() and this function takes event_mutex.
3. hist_unreg_all() is trigger_hist_cmd.unreg_all() which is called
by event_trigger_regex_open() and it takes event_mutex.
4. onmatch_destroy() and onmatch_create() have long call tree,
but both are finally invoked from event_trigger_regex_write()
and event_trace_del_tracer(), former takes event_mutex, and latter
ensures called under event_mutex locked.
Finally, I ensured there is no resource conflict. For safety,
I added lockdep_assert_held(&event_mutex) for each function.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
kernel/trace/trace_events_hist.c | 30 +++++++-----------------------
1 file changed, 7 insertions(+), 23 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 414aabd67d1f..21e4954375a1 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -444,8 +444,6 @@ static bool have_hist_err(void)
return false;
}
-static DEFINE_MUTEX(synth_event_mutex);
-
struct synth_trace_event {
struct trace_entry ent;
u64 fields[];
@@ -1077,7 +1075,6 @@ static int __create_synth_event(int argc, const char *name, const char **argv)
return -EINVAL;
mutex_lock(&event_mutex);
- mutex_lock(&synth_event_mutex);
event = find_synth_event(name);
if (event) {
@@ -1119,7 +1116,6 @@ static int __create_synth_event(int argc, const char *name, const char **argv)
else
free_synth_event(event);
out:
- mutex_unlock(&synth_event_mutex);
mutex_unlock(&event_mutex);
return ret;
@@ -1139,7 +1135,6 @@ static int create_or_delete_synth_event(int argc, char **argv)
/* trace_run_command() ensures argc != 0 */
if (name[0] == '!') {
mutex_lock(&event_mutex);
- mutex_lock(&synth_event_mutex);
event = find_synth_event(name + 1);
if (event) {
if (event->ref)
@@ -1153,7 +1148,6 @@ static int create_or_delete_synth_event(int argc, char **argv)
}
} else
ret = -ENOENT;
- mutex_unlock(&synth_event_mutex);
mutex_unlock(&event_mutex);
return ret;
}
@@ -3535,7 +3529,7 @@ static void onmatch_destroy(struct action_data *data)
{
unsigned int i;
- mutex_lock(&synth_event_mutex);
+ lockdep_assert_held(&event_mutex);
kfree(data->onmatch.match_event);
kfree(data->onmatch.match_event_system);
@@ -3548,8 +3542,6 @@ static void onmatch_destroy(struct action_data *data)
data->onmatch.synth_event->ref--;
kfree(data);
-
- mutex_unlock(&synth_event_mutex);
}
static void destroy_field_var(struct field_var *field_var)
@@ -3700,15 +3692,14 @@ static int onmatch_create(struct hist_trigger_data *hist_data,
struct synth_event *event;
int ret = 0;
- mutex_lock(&synth_event_mutex);
+ lockdep_assert_held(&event_mutex);
+
event = find_synth_event(data->onmatch.synth_event_name);
if (!event) {
hist_err("onmatch: Couldn't find synthetic event: ", data->onmatch.synth_event_name);
- mutex_unlock(&synth_event_mutex);
return -EINVAL;
}
event->ref++;
- mutex_unlock(&synth_event_mutex);
var_ref_idx = hist_data->n_var_refs;
@@ -3782,9 +3773,7 @@ static int onmatch_create(struct hist_trigger_data *hist_data,
out:
return ret;
err:
- mutex_lock(&synth_event_mutex);
event->ref--;
- mutex_unlock(&synth_event_mutex);
goto out;
}
@@ -5492,6 +5481,8 @@ static void hist_unreg_all(struct trace_event_file *file)
struct synth_event *se;
const char *se_name;
+ lockdep_assert_held(&event_mutex);
+
if (hist_file_check_refs(file))
return;
@@ -5501,12 +5492,10 @@ static void hist_unreg_all(struct trace_event_file *file)
list_del_rcu(&test->list);
trace_event_trigger_enable_disable(file, 0);
- mutex_lock(&synth_event_mutex);
se_name = trace_event_name(file->event_call);
se = find_synth_event(se_name);
if (se)
se->ref--;
- mutex_unlock(&synth_event_mutex);
update_cond_flag(file);
if (hist_data->enable_timestamps)
@@ -5532,6 +5521,8 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
char *trigger, *p;
int ret = 0;
+ lockdep_assert_held(&event_mutex);
+
if (glob && strlen(glob)) {
last_cmd_set(param);
hist_err_clear();
@@ -5622,14 +5613,10 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
}
cmd_ops->unreg(glob+1, trigger_ops, trigger_data, file);
-
- mutex_lock(&synth_event_mutex);
se_name = trace_event_name(file->event_call);
se = find_synth_event(se_name);
if (se)
se->ref--;
- mutex_unlock(&synth_event_mutex);
-
ret = 0;
goto out_free;
}
@@ -5665,13 +5652,10 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
if (ret)
goto out_unreg;
- mutex_lock(&synth_event_mutex);
se_name = trace_event_name(file->event_call);
se = find_synth_event(se_name);
if (se)
se->ref++;
- mutex_unlock(&synth_event_mutex);
-
/* Just return zero, not the number of registered triggers */
ret = 0;
out:
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 10/12] tracing: Remove orphaned trace_add/remove_event_call functions
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
` (8 preceding siblings ...)
2018-11-05 9:04 ` [PATCH v2 09/12] tracing: Remove unneeded synth_event_mutex Masami Hiramatsu
@ 2018-11-05 9:04 ` Masami Hiramatsu
2018-12-04 18:51 ` Steven Rostedt
2018-11-05 9:04 ` [PATCH v2 11/12] tracing: Add generic event-name based remove event method Masami Hiramatsu
` (2 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:04 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Remove trace_add_event_call() and trace_remove_event_call()
functions since those are not used anymore.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
include/linux/trace_events.h | 2 --
kernel/trace/trace_events.c | 26 ++------------------------
2 files changed, 2 insertions(+), 26 deletions(-)
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 3aa05593a53f..7a3147ead6bf 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -531,8 +531,6 @@ extern int trace_define_field(struct trace_event_call *call, const char *type,
int is_signed, int filter_type);
extern int trace_add_event_call_nolock(struct trace_event_call *call);
extern int trace_remove_event_call_nolock(struct trace_event_call *call);
-extern int trace_add_event_call(struct trace_event_call *call);
-extern int trace_remove_event_call(struct trace_event_call *call);
extern int trace_event_get_offsets(struct trace_event_call *call);
#define is_signed_type(type) (((type)(-1)) < (type)1)
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index a3b157f689ee..16cff7c0fd40 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2305,6 +2305,7 @@ __trace_early_add_new_event(struct trace_event_call *call,
struct ftrace_module_file_ops;
static void __add_event_to_tracers(struct trace_event_call *call);
+/* Add an additional event_call dynamically */
int trace_add_event_call_nolock(struct trace_event_call *call)
{
int ret;
@@ -2320,17 +2321,6 @@ int trace_add_event_call_nolock(struct trace_event_call *call)
return ret;
}
-/* Add an additional event_call dynamically */
-int trace_add_event_call(struct trace_event_call *call)
-{
- int ret;
-
- mutex_lock(&event_mutex);
- ret = trace_add_event_call_nolock(call);
- mutex_unlock(&event_mutex);
- return ret;
-}
-
/*
* Must be called under locking of trace_types_lock, event_mutex and
* trace_event_sem.
@@ -2376,7 +2366,7 @@ static int probe_remove_event_call(struct trace_event_call *call)
return 0;
}
-/* no event_mutex version */
+/* Remove an event_call */
int trace_remove_event_call_nolock(struct trace_event_call *call)
{
int ret;
@@ -2392,18 +2382,6 @@ int trace_remove_event_call_nolock(struct trace_event_call *call)
return ret;
}
-/* Remove an event_call */
-int trace_remove_event_call(struct trace_event_call *call)
-{
- int ret;
-
- mutex_lock(&event_mutex);
- ret = trace_remove_event_call_nolock(call);
- mutex_unlock(&event_mutex);
-
- return ret;
-}
-
#define for_each_event(event, start, end) \
for (event = start; \
(unsigned long)event < (unsigned long)end; \
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 10/12] tracing: Remove orphaned trace_add/remove_event_call functions
2018-11-05 9:04 ` [PATCH v2 10/12] tracing: Remove orphaned trace_add/remove_event_call functions Masami Hiramatsu
@ 2018-12-04 18:51 ` Steven Rostedt
2018-12-07 2:22 ` Masami Hiramatsu
0 siblings, 1 reply; 22+ messages in thread
From: Steven Rostedt @ 2018-12-04 18:51 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
On Mon, 5 Nov 2018 18:04:29 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> Remove trace_add_event_call() and trace_remove_event_call()
> functions since those are not used anymore.
>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Hi Masami,
I've applied the series locally (need to test it) except for this
patch. Honestly, I hate the "_nolock" name, and it makes no sense when
1) they still grab locks
2) there's no version without "_nolock"
I added this patch in its place:
-- Steve
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Date: Tue, 4 Dec 2018 13:35:45 -0500
Subject: [PATCH] tracing: Consolidate trace_add/remove_event_call back to the
nolock functions
The trace_add/remove_event_call_nolock() functions were added to allow
the tace_add/remove_event_call() code be called when the event_mutex
lock was already taken. Now that all callers are done within the
event_mutex, there's no reason to have two different interfaces.
Remove the current wrapper trace_add/remove_event_call()s and rename the
_nolock versions back to the original names.
Link: http://lkml.kernel.org/r/154140866955.17322.2081425494660638846.stgit@devbox
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
include/linux/trace_events.h | 2 --
kernel/trace/trace_events.c | 30 ++++--------------------------
kernel/trace/trace_events_hist.c | 6 +++---
kernel/trace/trace_kprobe.c | 4 ++--
kernel/trace/trace_uprobe.c | 4 ++--
5 files changed, 11 insertions(+), 35 deletions(-)
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 3aa05593a53f..4130a5497d40 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -529,8 +529,6 @@ extern int trace_event_raw_init(struct trace_event_call *call);
extern int trace_define_field(struct trace_event_call *call, const char *type,
const char *name, int offset, int size,
int is_signed, int filter_type);
-extern int trace_add_event_call_nolock(struct trace_event_call *call);
-extern int trace_remove_event_call_nolock(struct trace_event_call *call);
extern int trace_add_event_call(struct trace_event_call *call);
extern int trace_remove_event_call(struct trace_event_call *call);
extern int trace_event_get_offsets(struct trace_event_call *call);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index a3b157f689ee..bd0162c0467c 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2305,7 +2305,8 @@ __trace_early_add_new_event(struct trace_event_call *call,
struct ftrace_module_file_ops;
static void __add_event_to_tracers(struct trace_event_call *call);
-int trace_add_event_call_nolock(struct trace_event_call *call)
+/* Add an additional event_call dynamically */
+int trace_add_event_call(struct trace_event_call *call)
{
int ret;
lockdep_assert_held(&event_mutex);
@@ -2320,17 +2321,6 @@ int trace_add_event_call_nolock(struct trace_event_call *call)
return ret;
}
-/* Add an additional event_call dynamically */
-int trace_add_event_call(struct trace_event_call *call)
-{
- int ret;
-
- mutex_lock(&event_mutex);
- ret = trace_add_event_call_nolock(call);
- mutex_unlock(&event_mutex);
- return ret;
-}
-
/*
* Must be called under locking of trace_types_lock, event_mutex and
* trace_event_sem.
@@ -2376,8 +2366,8 @@ static int probe_remove_event_call(struct trace_event_call *call)
return 0;
}
-/* no event_mutex version */
-int trace_remove_event_call_nolock(struct trace_event_call *call)
+/* Remove an event_call */
+int trace_remove_event_call(struct trace_event_call *call)
{
int ret;
@@ -2392,18 +2382,6 @@ int trace_remove_event_call_nolock(struct trace_event_call *call)
return ret;
}
-/* Remove an event_call */
-int trace_remove_event_call(struct trace_event_call *call)
-{
- int ret;
-
- mutex_lock(&event_mutex);
- ret = trace_remove_event_call_nolock(call);
- mutex_unlock(&event_mutex);
-
- return ret;
-}
-
#define for_each_event(event, start, end) \
for (event = start; \
(unsigned long)event < (unsigned long)end; \
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 21e4954375a1..82e72c48a5a9 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -960,7 +960,7 @@ static int register_synth_event(struct synth_event *event)
call->data = event;
call->tp = event->tp;
- ret = trace_add_event_call_nolock(call);
+ ret = trace_add_event_call(call);
if (ret) {
pr_warn("Failed to register synthetic event: %s\n",
trace_event_name(call));
@@ -969,7 +969,7 @@ static int register_synth_event(struct synth_event *event)
ret = set_synth_event_print_fmt(call);
if (ret < 0) {
- trace_remove_event_call_nolock(call);
+ trace_remove_event_call(call);
goto err;
}
out:
@@ -984,7 +984,7 @@ static int unregister_synth_event(struct synth_event *event)
struct trace_event_call *call = &event->call;
int ret;
- ret = trace_remove_event_call_nolock(call);
+ ret = trace_remove_event_call(call);
return ret;
}
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index bdf8c2ad5152..0e0f7b8024fb 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1353,7 +1353,7 @@ static int register_kprobe_event(struct trace_kprobe *tk)
kfree(call->print_fmt);
return -ENODEV;
}
- ret = trace_add_event_call_nolock(call);
+ ret = trace_add_event_call(call);
if (ret) {
pr_info("Failed to register kprobe event: %s\n",
trace_event_name(call));
@@ -1368,7 +1368,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk)
int ret;
/* tp->event is unregistered in trace_remove_event_call() */
- ret = trace_remove_event_call_nolock(&tk->tp.call);
+ ret = trace_remove_event_call(&tk->tp.call);
if (!ret)
kfree(tk->tp.call.print_fmt);
return ret;
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 4a7b21c891f3..e335576b9411 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1320,7 +1320,7 @@ static int register_uprobe_event(struct trace_uprobe *tu)
return -ENODEV;
}
- ret = trace_add_event_call_nolock(call);
+ ret = trace_add_event_call(call);
if (ret) {
pr_info("Failed to register uprobe event: %s\n",
@@ -1337,7 +1337,7 @@ static int unregister_uprobe_event(struct trace_uprobe *tu)
int ret;
/* tu->event is unregistered in trace_remove_event_call() */
- ret = trace_remove_event_call_nolock(&tu->tp.call);
+ ret = trace_remove_event_call(&tu->tp.call);
if (ret)
return ret;
kfree(tu->tp.call.print_fmt);
--
2.19.2
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 10/12] tracing: Remove orphaned trace_add/remove_event_call functions
2018-12-04 18:51 ` Steven Rostedt
@ 2018-12-07 2:22 ` Masami Hiramatsu
0 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-12-07 2:22 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
On Tue, 4 Dec 2018 13:51:20 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 5 Nov 2018 18:04:29 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > Remove trace_add_event_call() and trace_remove_event_call()
> > functions since those are not used anymore.
> >
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
>
> Hi Masami,
>
> I've applied the series locally (need to test it) except for this
> patch. Honestly, I hate the "_nolock" name, and it makes no sense when
>
> 1) they still grab locks
> 2) there's no version without "_nolock"
Agreed.
>
> I added this patch in its place:
That looks good to me too. I thought to add similar patch, but just waited
for the comment.
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
Thank you!
>
> -- Steve
>
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Date: Tue, 4 Dec 2018 13:35:45 -0500
> Subject: [PATCH] tracing: Consolidate trace_add/remove_event_call back to the
> nolock functions
>
> The trace_add/remove_event_call_nolock() functions were added to allow
> the tace_add/remove_event_call() code be called when the event_mutex
> lock was already taken. Now that all callers are done within the
> event_mutex, there's no reason to have two different interfaces.
>
> Remove the current wrapper trace_add/remove_event_call()s and rename the
> _nolock versions back to the original names.
>
> Link: http://lkml.kernel.org/r/154140866955.17322.2081425494660638846.stgit@devbox
>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> include/linux/trace_events.h | 2 --
> kernel/trace/trace_events.c | 30 ++++--------------------------
> kernel/trace/trace_events_hist.c | 6 +++---
> kernel/trace/trace_kprobe.c | 4 ++--
> kernel/trace/trace_uprobe.c | 4 ++--
> 5 files changed, 11 insertions(+), 35 deletions(-)
>
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index 3aa05593a53f..4130a5497d40 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -529,8 +529,6 @@ extern int trace_event_raw_init(struct trace_event_call *call);
> extern int trace_define_field(struct trace_event_call *call, const char *type,
> const char *name, int offset, int size,
> int is_signed, int filter_type);
> -extern int trace_add_event_call_nolock(struct trace_event_call *call);
> -extern int trace_remove_event_call_nolock(struct trace_event_call *call);
> extern int trace_add_event_call(struct trace_event_call *call);
> extern int trace_remove_event_call(struct trace_event_call *call);
> extern int trace_event_get_offsets(struct trace_event_call *call);
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index a3b157f689ee..bd0162c0467c 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -2305,7 +2305,8 @@ __trace_early_add_new_event(struct trace_event_call *call,
> struct ftrace_module_file_ops;
> static void __add_event_to_tracers(struct trace_event_call *call);
>
> -int trace_add_event_call_nolock(struct trace_event_call *call)
> +/* Add an additional event_call dynamically */
> +int trace_add_event_call(struct trace_event_call *call)
> {
> int ret;
> lockdep_assert_held(&event_mutex);
> @@ -2320,17 +2321,6 @@ int trace_add_event_call_nolock(struct trace_event_call *call)
> return ret;
> }
>
> -/* Add an additional event_call dynamically */
> -int trace_add_event_call(struct trace_event_call *call)
> -{
> - int ret;
> -
> - mutex_lock(&event_mutex);
> - ret = trace_add_event_call_nolock(call);
> - mutex_unlock(&event_mutex);
> - return ret;
> -}
> -
> /*
> * Must be called under locking of trace_types_lock, event_mutex and
> * trace_event_sem.
> @@ -2376,8 +2366,8 @@ static int probe_remove_event_call(struct trace_event_call *call)
> return 0;
> }
>
> -/* no event_mutex version */
> -int trace_remove_event_call_nolock(struct trace_event_call *call)
> +/* Remove an event_call */
> +int trace_remove_event_call(struct trace_event_call *call)
> {
> int ret;
>
> @@ -2392,18 +2382,6 @@ int trace_remove_event_call_nolock(struct trace_event_call *call)
> return ret;
> }
>
> -/* Remove an event_call */
> -int trace_remove_event_call(struct trace_event_call *call)
> -{
> - int ret;
> -
> - mutex_lock(&event_mutex);
> - ret = trace_remove_event_call_nolock(call);
> - mutex_unlock(&event_mutex);
> -
> - return ret;
> -}
> -
> #define for_each_event(event, start, end) \
> for (event = start; \
> (unsigned long)event < (unsigned long)end; \
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 21e4954375a1..82e72c48a5a9 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -960,7 +960,7 @@ static int register_synth_event(struct synth_event *event)
> call->data = event;
> call->tp = event->tp;
>
> - ret = trace_add_event_call_nolock(call);
> + ret = trace_add_event_call(call);
> if (ret) {
> pr_warn("Failed to register synthetic event: %s\n",
> trace_event_name(call));
> @@ -969,7 +969,7 @@ static int register_synth_event(struct synth_event *event)
>
> ret = set_synth_event_print_fmt(call);
> if (ret < 0) {
> - trace_remove_event_call_nolock(call);
> + trace_remove_event_call(call);
> goto err;
> }
> out:
> @@ -984,7 +984,7 @@ static int unregister_synth_event(struct synth_event *event)
> struct trace_event_call *call = &event->call;
> int ret;
>
> - ret = trace_remove_event_call_nolock(call);
> + ret = trace_remove_event_call(call);
>
> return ret;
> }
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index bdf8c2ad5152..0e0f7b8024fb 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -1353,7 +1353,7 @@ static int register_kprobe_event(struct trace_kprobe *tk)
> kfree(call->print_fmt);
> return -ENODEV;
> }
> - ret = trace_add_event_call_nolock(call);
> + ret = trace_add_event_call(call);
> if (ret) {
> pr_info("Failed to register kprobe event: %s\n",
> trace_event_name(call));
> @@ -1368,7 +1368,7 @@ static int unregister_kprobe_event(struct trace_kprobe *tk)
> int ret;
>
> /* tp->event is unregistered in trace_remove_event_call() */
> - ret = trace_remove_event_call_nolock(&tk->tp.call);
> + ret = trace_remove_event_call(&tk->tp.call);
> if (!ret)
> kfree(tk->tp.call.print_fmt);
> return ret;
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 4a7b21c891f3..e335576b9411 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -1320,7 +1320,7 @@ static int register_uprobe_event(struct trace_uprobe *tu)
> return -ENODEV;
> }
>
> - ret = trace_add_event_call_nolock(call);
> + ret = trace_add_event_call(call);
>
> if (ret) {
> pr_info("Failed to register uprobe event: %s\n",
> @@ -1337,7 +1337,7 @@ static int unregister_uprobe_event(struct trace_uprobe *tu)
> int ret;
>
> /* tu->event is unregistered in trace_remove_event_call() */
> - ret = trace_remove_event_call_nolock(&tu->tp.call);
> + ret = trace_remove_event_call(&tu->tp.call);
> if (ret)
> return ret;
> kfree(tu->tp.call.print_fmt);
> --
> 2.19.2
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 11/12] tracing: Add generic event-name based remove event method
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
` (9 preceding siblings ...)
2018-11-05 9:04 ` [PATCH v2 10/12] tracing: Remove orphaned trace_add/remove_event_call functions Masami Hiramatsu
@ 2018-11-05 9:04 ` Masami Hiramatsu
2018-11-05 9:05 ` [PATCH v2 12/12] selftests/ftrace: Add testcases for dynamic event Masami Hiramatsu
2018-11-28 7:31 ` [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
12 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:04 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Add a generic method to remove event from dynamic event
list. This is same as other system under ftrace. You
just need to pass the event name with '!', e.g.
# echo p:new_grp/new_event _do_fork > dynamic_events
This creates an event, and
# echo '!p:new_grp/new_event _do_fork' > dynamic_events
Or,
# echo '!p:new_grp/new_event' > dynamic_events
will remove new_grp/new_event event.
Note that this doesn't check the event prefix (e.g. "p:")
strictly, because the "group/event" name must be unique.
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
Changes in v2:
- Instead of checking the given entire line strictly,
simply checking the event and group name.
---
kernel/trace/trace_dynevent.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/trace_dynevent.c b/kernel/trace/trace_dynevent.c
index f17a887abb66..dd1f43588d70 100644
--- a/kernel/trace/trace_dynevent.c
+++ b/kernel/trace/trace_dynevent.c
@@ -37,10 +37,17 @@ int dyn_event_release(int argc, char **argv, struct dyn_event_operations *type)
char *system = NULL, *event, *p;
int ret = -ENOENT;
- if (argv[0][1] != ':')
- return -EINVAL;
+ if (argv[0][0] == '-') {
+ if (argv[0][1] != ':')
+ return -EINVAL;
+ event = &argv[0][2];
+ } else {
+ event = strchr(argv[0], ':');
+ if (!event)
+ return -EINVAL;
+ event++;
+ }
- event = &argv[0][2];
p = strchr(event, '/');
if (p) {
system = event;
@@ -69,7 +76,7 @@ static int create_dyn_event(int argc, char **argv)
struct dyn_event_operations *ops;
int ret;
- if (argv[0][0] == '-')
+ if (argv[0][0] == '-' || argv[0][0] == '!')
return dyn_event_release(argc, argv, NULL);
mutex_lock(&dyn_event_ops_mutex);
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 12/12] selftests/ftrace: Add testcases for dynamic event
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
` (10 preceding siblings ...)
2018-11-05 9:04 ` [PATCH v2 11/12] tracing: Add generic event-name based remove event method Masami Hiramatsu
@ 2018-11-05 9:05 ` Masami Hiramatsu
2018-11-28 7:31 ` [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
12 siblings, 0 replies; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-05 9:05 UTC (permalink / raw)
To: Steven Rostedt; +Cc: linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Add common testcases for dynamic_events interface.
- Add/remove kprobe events via dynamic_events
- Add/remove synthetic events via dynamic_events
- Selective clear events (clear events other interfaces)
- Genelic clear events ("!LINE" syntax)
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
.../ftrace/test.d/dynevent/add_remove_kprobe.tc | 30 ++++++++++++
.../ftrace/test.d/dynevent/add_remove_synth.tc | 27 +++++++++++
.../ftrace/test.d/dynevent/clear_select_events.tc | 50 ++++++++++++++++++++
.../ftrace/test.d/dynevent/generic_clear_event.tc | 49 ++++++++++++++++++++
4 files changed, 156 insertions(+)
create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/add_remove_synth.tc
create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/clear_select_events.tc
create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/generic_clear_event.tc
diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
new file mode 100644
index 000000000000..c6d8387dbbb8
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
@@ -0,0 +1,30 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: Generic dynamic event - add/remove kprobe events
+
+[ -f dynamic_events ] || exit_unsupported
+
+grep -q "place: \[<module>:\]<symbol>" README || exit_unsupported
+grep -q "place (kretprobe): \[<module>:\]<symbol>" README || exit_unsupported
+
+echo 0 > events/enable
+echo > dynamic_events
+
+PLACE=_do_fork
+
+echo "p:myevent1 $PLACE" >> dynamic_events
+echo "r:myevent2 $PLACE" >> dynamic_events
+
+grep -q myevent1 dynamic_events
+grep -q myevent2 dynamic_events
+test -d events/kprobes/myevent1
+test -d events/kprobes/myevent2
+
+echo "-:myevent2" >> dynamic_events
+
+grep -q myevent1 dynamic_events
+! grep -q myevent2 dynamic_events
+
+echo > dynamic_events
+
+clear_trace
diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_synth.tc b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_synth.tc
new file mode 100644
index 000000000000..62b77b5941d0
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/add_remove_synth.tc
@@ -0,0 +1,27 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: Generic dynamic event - add/remove synthetic events
+
+[ -f dynamic_events ] || exit_unsupported
+
+grep -q "s:\[synthetic/\]" README || exit_unsupported
+
+echo 0 > events/enable
+echo > dynamic_events
+
+echo "s:latency1 u64 lat; pid_t pid;" >> dynamic_events
+echo "s:latency2 u64 lat; pid_t pid;" >> dynamic_events
+
+grep -q latency1 dynamic_events
+grep -q latency2 dynamic_events
+test -d events/synthetic/latency1
+test -d events/synthetic/latency2
+
+echo "-:synthetic/latency2" >> dynamic_events
+
+grep -q latency1 dynamic_events
+! grep -q latency2 dynamic_events
+
+echo > dynamic_events
+
+clear_trace
diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/clear_select_events.tc b/tools/testing/selftests/ftrace/test.d/dynevent/clear_select_events.tc
new file mode 100644
index 000000000000..e0842109cb57
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/clear_select_events.tc
@@ -0,0 +1,50 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: Generic dynamic event - selective clear (compatibility)
+
+[ -f dynamic_events ] || exit_unsupported
+
+grep -q "place: \[<module>:\]<symbol>" README || exit_unsupported
+grep -q "place (kretprobe): \[<module>:\]<symbol>" README || exit_unsupported
+
+grep -q "s:\[synthetic/\]" README || exit_unsupported
+
+[ -f synthetic_events ] || exit_unsupported
+[ -f kprobe_events ] || exit_unsupported
+
+echo 0 > events/enable
+echo > dynamic_events
+
+PLACE=_do_fork
+
+setup_events() {
+echo "p:myevent1 $PLACE" >> dynamic_events
+echo "s:latency1 u64 lat; pid_t pid;" >> dynamic_events
+echo "r:myevent2 $PLACE" >> dynamic_events
+echo "s:latency2 u64 lat; pid_t pid;" >> dynamic_events
+
+grep -q myevent1 dynamic_events
+grep -q myevent2 dynamic_events
+grep -q latency1 dynamic_events
+grep -q latency2 dynamic_events
+}
+
+setup_events
+echo > synthetic_events
+
+grep -q myevent1 dynamic_events
+grep -q myevent2 dynamic_events
+! grep -q latency1 dynamic_events
+! grep -q latency2 dynamic_events
+
+echo > dynamic_events
+
+setup_events
+echo > kprobe_events
+
+! grep -q myevent1 dynamic_events
+! grep -q myevent2 dynamic_events
+grep -q latency1 dynamic_events
+grep -q latency2 dynamic_events
+
+echo > dynamic_events
diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/generic_clear_event.tc b/tools/testing/selftests/ftrace/test.d/dynevent/generic_clear_event.tc
new file mode 100644
index 000000000000..901922e97878
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/dynevent/generic_clear_event.tc
@@ -0,0 +1,49 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: Generic dynamic event - generic clear event
+
+[ -f dynamic_events ] || exit_unsupported
+
+grep -q "place: \[<module>:\]<symbol>" README || exit_unsupported
+grep -q "place (kretprobe): \[<module>:\]<symbol>" README || exit_unsupported
+
+grep -q "s:\[synthetic/\]" README || exit_unsupported
+
+echo 0 > events/enable
+echo > dynamic_events
+
+PLACE=_do_fork
+
+setup_events() {
+echo "p:myevent1 $PLACE" >> dynamic_events
+echo "s:latency1 u64 lat; pid_t pid;" >> dynamic_events
+echo "r:myevent2 $PLACE" >> dynamic_events
+echo "s:latency2 u64 lat; pid_t pid;" >> dynamic_events
+
+grep -q myevent1 dynamic_events
+grep -q myevent2 dynamic_events
+grep -q latency1 dynamic_events
+grep -q latency2 dynamic_events
+}
+
+setup_events
+
+echo "!p:myevent1 $PLACE" >> dynamic_events
+! grep -q myevent1 dynamic_events
+grep -q myevent2 dynamic_events
+grep -q latency1 dynamic_events
+grep -q latency2 dynamic_events
+
+echo "!s:latency1 u64 lat; pid_t pid;" >> dynamic_events
+grep -q myevent2 dynamic_events
+! grep -q latency1 dynamic_events
+grep -q latency2 dynamic_events
+
+echo "!r:myevent2 $PLACE" >> dynamic_events
+! grep -q myevent2 dynamic_events
+grep -q latency2 dynamic_events
+
+echo "!s:latency2 u64 lat; pid_t pid;" >> dynamic_events
+! grep -q latency2 dynamic_events
+
+echo > dynamic_events
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 00/12] tracing: Unifying dynamic event interface
2018-11-05 8:59 [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
` (11 preceding siblings ...)
2018-11-05 9:05 ` [PATCH v2 12/12] selftests/ftrace: Add testcases for dynamic event Masami Hiramatsu
@ 2018-11-28 7:31 ` Masami Hiramatsu
2018-11-28 23:42 ` Tom Zanussi
12 siblings, 1 reply; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-28 7:31 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Steven Rostedt, linux-kernel, Ingo Molnar, Tom Zanussi, Ravi Bangoria
Ping?
Hi Tom,
This series, especially [09/12] tracing: Remove unneeded synth_event_mutex
will effect your current working series. Please tell me your opinion.
Thank you,
On Mon, 5 Nov 2018 17:59:46 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> Hi,
>
> This is v2 series of unifying dynamic event interface on ftrace.
> Currently ftrace has 3 dynamic event interfaces, kprobes, uprobes
> and synthetic. This series unifies those dynamic event interfaces
> to "dynamic_events" so that we can add other dynamic events easily
> on same interface, e.g. function events.
> The older interfaces are left on the tracefs for backward
> compatibility.
>
> dynamic_events syntax has no difference from kprobe_events and
> uprobe_events. You can use same syntax for dynamic_events interface.
> For synthetic events, similar to the probe events, dynamic_events
> adds "s:[GROUP/]" prefix, where the "GROUP/" must be "synthetic/".
>
> s:[synthetic/]<event-name> type arg [type arg]...
>
> E.g.
>
> $ echo 'wakeup_latency u64 lat pid_t pid char' > synthetic_events
>
> is same as
>
> $ echo 's:wakeup_latency u64 lat pid_t pid char' > dynamic_events
>
> Or
>
> $ echo 's:synthetic/wakeup_latency u64 lat pid_t pid char' > dynamic_events
>
> This series modifies synthetic event interface behavior a bit,
> reorder lock dependency and related cleanups so that we can integrate
> the synthetic event to dynamic_events interface.
>
> In this version, I changed the generic '!' erase command, which
> now supports entire line style like other interfaces. So you can
> delete events via dynamic_events as below
>
> $ cat dynamic_events | while read line; \
> do echo "!$line" >> dynamic_events; done
>
> Also, the big change will be removing dyn_event_mutex and
> synth_event_mutex because all those parts are protected by
> event_mutex.
>
> Changes from v2 are here;
>
> New patches:
> - Reorder event_mutex and synth_event_mutex to solve
> AB-BA deadlock correctly. ([2/12])
> - Simplify creation and deletion of synthetic event. ([3/12])
> - Retern -ENOENT if there is no synthetic event when deleting ([4/12])
> - Integrate similar probe argument parsers ([5/12])
> - Use dyn_event framework for synthetic events ([9/12])
> - Remove synth_event_mutex ([10/12])
> - Remove unused APIs ([11/12])
>
> Modified patches:
> [6/12] - [8/12]
> - Generalize delete event and export as dyn_event_release_all().
> - Add match operation for find deleting event.
> - Reorder event_mutex and dyn_event_mutex to solve lock dependency
> issue.
> - Pass const char **argv for create operation and use -ECANCELED to
> signal for trying next dyn_event_operations.
> - Remove dyn_event_mutex.
>
> [12/12]
> - Accept entire line, but instead of checking the given entire line
> strictly, simply checking the event and group name.
>
> Tom, thanks for your Ack for v1 series. Since I changed many things
> from v1 (not only minor change), I decided to not add your Ack for
> this version. Anyway, what I've added in this version are related to
> synthetic events. I need your review for those.
> (especially removing synth_event_mutex)
>
> You can try it from my git tree.
>
> https://github.com/mhiramat/linux/tree/unify-dynamic-events-v2
>
> Thank you,
>
> ---
>
> Masami Hiramatsu (12):
> tracing/uprobes: Add busy check when cleanup all uprobes
> tracing: Lock event_mutex before synth_event_mutex
> tracing: Simplify creation and deletion of synthetic event
> tracing: Integrate similar probe argument parsers
> tracing: Add unified dynamic event framework
> tracing/kprobes: Use dyn_event framework for kprobe events
> tracing/uprobes: Use dyn_event framework for uprobe events
> tracing: Use dyn_event framework for synthetic events
> tracing: Remove unneeded synth_event_mutex
> tracing: Remove orphaned trace_add/remove_event_call functions
> tracing: Add generic event-name based remove event method
> selftests/ftrace: Add testcases for dynamic event
>
>
> Documentation/trace/kprobetrace.rst | 3
> Documentation/trace/uprobetracer.rst | 4
> include/linux/trace_events.h | 4
> kernel/trace/Kconfig | 6
> kernel/trace/Makefile | 1
> kernel/trace/trace.c | 12 +
> kernel/trace/trace_dynevent.c | 217 ++++++++++++
> kernel/trace/trace_dynevent.h | 119 +++++++
> kernel/trace/trace_events.c | 12 -
> kernel/trace/trace_events_hist.c | 322 ++++++++++--------
> kernel/trace/trace_kprobe.c | 357 ++++++++++----------
> kernel/trace/trace_probe.c | 74 ++++
> kernel/trace/trace_probe.h | 9 -
> kernel/trace/trace_uprobe.c | 305 ++++++++---------
> .../ftrace/test.d/dynevent/add_remove_kprobe.tc | 30 ++
> .../ftrace/test.d/dynevent/add_remove_synth.tc | 27 ++
> .../ftrace/test.d/dynevent/clear_select_events.tc | 50 +++
> .../ftrace/test.d/dynevent/generic_clear_event.tc | 49 +++
> 18 files changed, 1094 insertions(+), 507 deletions(-)
> create mode 100644 kernel/trace/trace_dynevent.c
> create mode 100644 kernel/trace/trace_dynevent.h
> create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
> create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/add_remove_synth.tc
> create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/clear_select_events.tc
> create mode 100644 tools/testing/selftests/ftrace/test.d/dynevent/generic_clear_event.tc
>
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 00/12] tracing: Unifying dynamic event interface
2018-11-28 7:31 ` [PATCH v2 00/12] tracing: Unifying dynamic event interface Masami Hiramatsu
@ 2018-11-28 23:42 ` Tom Zanussi
2018-11-29 3:46 ` Steven Rostedt
2018-11-29 5:20 ` Masami Hiramatsu
0 siblings, 2 replies; 22+ messages in thread
From: Tom Zanussi @ 2018-11-28 23:42 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: Steven Rostedt, linux-kernel, Ingo Molnar, Ravi Bangoria
Hi Masami,
On Wed, 2018-11-28 at 16:31 +0900, Masami Hiramatsu wrote:
> Ping?
>
> Hi Tom,
>
> This series, especially [09/12] tracing: Remove unneeded
> synth_event_mutex
> will effect your current working series. Please tell me your opinion.
>
Sorry for the delay in reviewing this - I completely forgot about it in
my inbox.
It's all very nice, and the mutex updates along with the dyn_event
management make that part of the code much cleaner, thanks!
In any case, you can have my
Reviewed-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Tested-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Thanks,
Tom
> Thank you,
>
> On Mon, 5 Nov 2018 17:59:46 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> > Hi,
> >
> > This is v2 series of unifying dynamic event interface on ftrace.
> > Currently ftrace has 3 dynamic event interfaces, kprobes, uprobes
> > and synthetic. This series unifies those dynamic event interfaces
> > to "dynamic_events" so that we can add other dynamic events easily
> > on same interface, e.g. function events.
> > The older interfaces are left on the tracefs for backward
> > compatibility.
> >
> > dynamic_events syntax has no difference from kprobe_events and
> > uprobe_events. You can use same syntax for dynamic_events
> > interface.
> > For synthetic events, similar to the probe events, dynamic_events
> > adds "s:[GROUP/]" prefix, where the "GROUP/" must be "synthetic/".
> >
> > s:[synthetic/]<event-name> type arg [type arg]...
> >
> > E.g.
> >
> > $ echo 'wakeup_latency u64 lat pid_t pid char' > synthetic_events
> >
> > is same as
> >
> > $ echo 's:wakeup_latency u64 lat pid_t pid char' > dynamic_events
> >
> > Or
> >
> > $ echo 's:synthetic/wakeup_latency u64 lat pid_t pid char' >
> > dynamic_events
> >
> > This series modifies synthetic event interface behavior a bit,
> > reorder lock dependency and related cleanups so that we can
> > integrate
> > the synthetic event to dynamic_events interface.
> >
> > In this version, I changed the generic '!' erase command, which
> > now supports entire line style like other interfaces. So you can
> > delete events via dynamic_events as below
> >
> > $ cat dynamic_events | while read line; \
> > do echo "!$line" >> dynamic_events; done
> >
> > Also, the big change will be removing dyn_event_mutex and
> > synth_event_mutex because all those parts are protected by
> > event_mutex.
> >
> > Changes from v2 are here;
> >
> > New patches:
> > - Reorder event_mutex and synth_event_mutex to solve
> > AB-BA deadlock correctly. ([2/12])
> > - Simplify creation and deletion of synthetic event. ([3/12])
> > - Retern -ENOENT if there is no synthetic event when deleting
> > ([4/12])
> > - Integrate similar probe argument parsers ([5/12])
> > - Use dyn_event framework for synthetic events ([9/12])
> > - Remove synth_event_mutex ([10/12])
> > - Remove unused APIs ([11/12])
> >
> > Modified patches:
> > [6/12] - [8/12]
> > - Generalize delete event and export as dyn_event_release_all().
> > - Add match operation for find deleting event.
> > - Reorder event_mutex and dyn_event_mutex to solve lock dependency
> > issue.
> > - Pass const char **argv for create operation and use -ECANCELED
> > to
> > signal for trying next dyn_event_operations.
> > - Remove dyn_event_mutex.
> >
> > [12/12]
> > - Accept entire line, but instead of checking the given entire
> > line
> > strictly, simply checking the event and group name.
> >
> > Tom, thanks for your Ack for v1 series. Since I changed many things
> > from v1 (not only minor change), I decided to not add your Ack for
> > this version. Anyway, what I've added in this version are related
> > to
> > synthetic events. I need your review for those.
> > (especially removing synth_event_mutex)
> >
> > You can try it from my git tree.
> >
> > https://github.com/mhiramat/linux/tree/unify-dynamic-events-v2
> >
> > Thank you,
> >
> > ---
> >
> > Masami Hiramatsu (12):
> > tracing/uprobes: Add busy check when cleanup all uprobes
> > tracing: Lock event_mutex before synth_event_mutex
> > tracing: Simplify creation and deletion of synthetic event
> > tracing: Integrate similar probe argument parsers
> > tracing: Add unified dynamic event framework
> > tracing/kprobes: Use dyn_event framework for kprobe events
> > tracing/uprobes: Use dyn_event framework for uprobe events
> > tracing: Use dyn_event framework for synthetic events
> > tracing: Remove unneeded synth_event_mutex
> > tracing: Remove orphaned trace_add/remove_event_call
> > functions
> > tracing: Add generic event-name based remove event method
> > selftests/ftrace: Add testcases for dynamic event
> >
> >
> > Documentation/trace/kprobetrace.rst | 3
> > Documentation/trace/uprobetracer.rst | 4
> > include/linux/trace_events.h | 4
> > kernel/trace/Kconfig | 6
> > kernel/trace/Makefile | 1
> > kernel/trace/trace.c | 12 +
> > kernel/trace/trace_dynevent.c | 217
> > ++++++++++++
> > kernel/trace/trace_dynevent.h | 119 +++++++
> > kernel/trace/trace_events.c | 12 -
> > kernel/trace/trace_events_hist.c | 322
> > ++++++++++--------
> > kernel/trace/trace_kprobe.c | 357
> > ++++++++++----------
> > kernel/trace/trace_probe.c | 74 ++++
> > kernel/trace/trace_probe.h | 9 -
> > kernel/trace/trace_uprobe.c | 305
> > ++++++++---------
> > .../ftrace/test.d/dynevent/add_remove_kprobe.tc | 30 ++
> > .../ftrace/test.d/dynevent/add_remove_synth.tc | 27 ++
> > .../ftrace/test.d/dynevent/clear_select_events.tc | 50 +++
> > .../ftrace/test.d/dynevent/generic_clear_event.tc | 49 +++
> > 18 files changed, 1094 insertions(+), 507 deletions(-)
> > create mode 100644 kernel/trace/trace_dynevent.c
> > create mode 100644 kernel/trace/trace_dynevent.h
> > create mode 100644
> > tools/testing/selftests/ftrace/test.d/dynevent/add_remove_kprobe.tc
> > create mode 100644
> > tools/testing/selftests/ftrace/test.d/dynevent/add_remove_synth.tc
> > create mode 100644
> > tools/testing/selftests/ftrace/test.d/dynevent/clear_select_events.
> > tc
> > create mode 100644
> > tools/testing/selftests/ftrace/test.d/dynevent/generic_clear_event.
> > tc
> >
> > --
> > Masami Hiramatsu (Linaro) <mhiramat@kernel.org>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 00/12] tracing: Unifying dynamic event interface
2018-11-28 23:42 ` Tom Zanussi
@ 2018-11-29 3:46 ` Steven Rostedt
2018-11-29 5:20 ` Masami Hiramatsu
1 sibling, 0 replies; 22+ messages in thread
From: Steven Rostedt @ 2018-11-29 3:46 UTC (permalink / raw)
To: Tom Zanussi; +Cc: Masami Hiramatsu, linux-kernel, Ingo Molnar, Ravi Bangoria
On Wed, 28 Nov 2018 17:42:22 -0600
Tom Zanussi <tom.zanussi@linux.intel.com> wrote:
> Hi Masami,
>
> On Wed, 2018-11-28 at 16:31 +0900, Masami Hiramatsu wrote:
> > Ping?
> >
> > Hi Tom,
> >
> > This series, especially [09/12] tracing: Remove unneeded
> > synth_event_mutex
> > will effect your current working series. Please tell me your opinion.
> >
>
> Sorry for the delay in reviewing this - I completely forgot about it in
> my inbox.
>
> It's all very nice, and the mutex updates along with the dyn_event
> management make that part of the code much cleaner, thanks!
>
> In any case, you can have my
>
> Reviewed-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> Tested-by: Tom Zanussi <tom.zanussi@linux.intel.com>
>
>
Thanks Tom! I'll take a look at this tomorrow.
-- Steve
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 00/12] tracing: Unifying dynamic event interface
2018-11-28 23:42 ` Tom Zanussi
2018-11-29 3:46 ` Steven Rostedt
@ 2018-11-29 5:20 ` Masami Hiramatsu
2018-11-29 15:08 ` Tom Zanussi
1 sibling, 1 reply; 22+ messages in thread
From: Masami Hiramatsu @ 2018-11-29 5:20 UTC (permalink / raw)
To: Tom Zanussi; +Cc: Steven Rostedt, linux-kernel, Ingo Molnar, Ravi Bangoria
Hi Tom,
On Wed, 28 Nov 2018 17:42:22 -0600
Tom Zanussi <tom.zanussi@linux.intel.com> wrote:
> Hi Masami,
>
> On Wed, 2018-11-28 at 16:31 +0900, Masami Hiramatsu wrote:
> > Ping?
> >
> > Hi Tom,
> >
> > This series, especially [09/12] tracing: Remove unneeded
> > synth_event_mutex
> > will effect your current working series. Please tell me your opinion.
> >
>
> Sorry for the delay in reviewing this - I completely forgot about it in
> my inbox.
>
> It's all very nice, and the mutex updates along with the dyn_event
> management make that part of the code much cleaner, thanks!
>
> In any case, you can have my
>
> Reviewed-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> Tested-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Thank you!
And as I pointed, synth_event_mutex is removed by this series, it may
affect your hist trigger series too.
Thanks,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 00/12] tracing: Unifying dynamic event interface
2018-11-29 5:20 ` Masami Hiramatsu
@ 2018-11-29 15:08 ` Tom Zanussi
0 siblings, 0 replies; 22+ messages in thread
From: Tom Zanussi @ 2018-11-29 15:08 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: Steven Rostedt, linux-kernel, Ingo Molnar, Ravi Bangoria
Hi Masami,
On Thu, 2018-11-29 at 14:20 +0900, Masami Hiramatsu wrote:
> Hi Tom,
>
> On Wed, 28 Nov 2018 17:42:22 -0600
> Tom Zanussi <tom.zanussi@linux.intel.com> wrote:
>
> > Hi Masami,
> >
> > On Wed, 2018-11-28 at 16:31 +0900, Masami Hiramatsu wrote:
> > > Ping?
> > >
> > > Hi Tom,
> > >
> > > This series, especially [09/12] tracing: Remove unneeded
> > > synth_event_mutex
> > > will effect your current working series. Please tell me your
> > > opinion.
> > >
> >
> > Sorry for the delay in reviewing this - I completely forgot about
> > it in
> > my inbox.
> >
> > It's all very nice, and the mutex updates along with the dyn_event
> > management make that part of the code much cleaner, thanks!
> >
> > In any case, you can have my
> >
> > Reviewed-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> > Tested-by: Tom Zanussi <tom.zanussi@linux.intel.com>
>
> Thank you!
> And as I pointed, synth_event_mutex is removed by this series, it may
> affect your hist trigger series too.
>
Yes, thanks for pointing that out - I'll rebase mine on top of this.
Tom
^ permalink raw reply [flat|nested] 22+ messages in thread