Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1338422 > unrolled thread

Re: [PATCH v13 06/29] tracing: Add needs_rec flag to event triggers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v13 06/29] tracing: Add needs_rec flag to event triggers Steven Rostedt <rostedt@goodmis.org> - 2016-02-19 20:40 +0100
    Re: [PATCH v13 06/29] tracing: Add needs_rec flag to event triggers Steven Rostedt <rostedt@goodmis.org> - 2016-02-19 23:40 +0100
      Re: [PATCH v13 06/29] tracing: Add needs_rec flag to event triggers Tom Zanussi <tom.zanussi@linux.intel.com> - 2016-02-20 00:40 +0100
        Re: [PATCH v13 06/29] tracing: Add needs_rec flag to event triggers Steven Rostedt <rostedt@goodmis.org> - 2016-02-20 02:30 +0100

#1338422 — Re: [PATCH v13 06/29] tracing: Add needs_rec flag to event triggers

On Thu, 10 Dec 2015 12:50:48 -0600
Tom Zanussi <tom.zanussi@linux.intel.com> wrote:

> Add a new needs_rec flag for triggers that require unconditional
> access to trace records in order to function.
> 
> Normally a trigger requires access to the contents of a trace record
> only if it has a filter associated with it (since filters need the
> contents of a record in order to make a filtering decision).  Some
> types of triggers, such as 'hist' triggers, require access to trace
> record contents independent of the presence of filters, so add a new
> flag for those triggers.
> 
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> Tested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Reviewed-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  kernel/trace/trace.h                | 7 +++++++
>  kernel/trace/trace_events_trigger.c | 3 ++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 5e23a97..300add5 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1293,6 +1293,12 @@ struct event_trigger_ops {
>   *	itself logs to the trace buffer, this flag should be set,
>   *	otherwise it can be left unspecified.
>   *
> + * @needs_rec: A flag that says whether or not this command needs
> + *	access to the trace record in order to perform its function,
> + *	regardless of whether or not it has a filter associated with
> + *	it (filters make a trigger require access to the trace record
> + *	but are not always present).
> + *
>   * All the methods below, except for @set_filter(), must be
>   * implemented.
>   *
> @@ -1333,6 +1339,7 @@ struct event_command {
>  	char			*name;
>  	enum event_trigger_type	trigger_type;
>  	bool			post_trigger;
> +	bool			needs_rec;

Note, from what I understand, gcc sucks with bool in structures. Best
thing to do here is to create a "int flags" field, and check the result
with masks.

You don't need to update this patch (I'm still working on the series),
but a patch on top of these may be necessary. I could add the patch too.

-- Steve


>  	int			(*func)(struct event_command *cmd_ops,
>  					struct trace_event_file *file,
>  					char *glob, char *cmd, char *params);
> diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> index 8375d29..2f881e9 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -498,7 +498,8 @@ void update_cond_flag(struct trace_event_file *file)
>  	bool set_cond = false;
>  
>  	list_for_each_entry_rcu(data, &file->triggers, list) {
> -		if (data->filter || data->cmd_ops->post_trigger) {
> +		if (data->filter || data->cmd_ops->post_trigger ||
> +		    data->cmd_ops->needs_rec) {
>  			set_cond = true;
>  			break;
>  		}

[toc] | [next] | [standalone]

#1338541

On Fri, 19 Feb 2016 14:30:13 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> > @@ -1333,6 +1339,7 @@ struct event_command {
> >  	char			*name;
> >  	enum event_trigger_type	trigger_type;
> >  	bool			post_trigger;
> > +	bool			needs_rec;  
> 
> Note, from what I understand, gcc sucks with bool in structures. Best
> thing to do here is to create a "int flags" field, and check the result
> with masks.
> 
> You don't need to update this patch (I'm still working on the series),
> but a patch on top of these may be necessary. I could add the patch too.
> 

I've added this patch after patch this patch. What do you think?

-- Steve

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 789e88ab5acd..f60c0444ef1e 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1272,30 +1272,7 @@ struct event_trigger_ops {
  *	values are defined by adding new values to the trigger_type
  *	enum in include/linux/trace_events.h.
  *
- * @post_trigger: A flag that says whether or not this command needs
- *	to have its action delayed until after the current event has
- *	been closed.  Some triggers need to avoid being invoked while
- *	an event is currently in the process of being logged, since
- *	the trigger may itself log data into the trace buffer.  Thus
- *	we make sure the current event is committed before invoking
- *	those triggers.  To do that, the trigger invocation is split
- *	in two - the first part checks the filter using the current
- *	trace record; if a command has the @post_trigger flag set, it
- *	sets a bit for itself in the return value, otherwise it
- *	directly invokes the trigger.  Once all commands have been
- *	either invoked or set their return flag, the current record is
- *	either committed or discarded.  At that point, if any commands
- *	have deferred their triggers, those commands are finally
- *	invoked following the close of the current event.  In other
- *	words, if the event_trigger_ops @func() probe implementation
- *	itself logs to the trace buffer, this flag should be set,
- *	otherwise it can be left unspecified.
- *
- * @needs_rec: A flag that says whether or not this command needs
- *	access to the trace record in order to perform its function,
- *	regardless of whether or not it has a filter associated with
- *	it (filters make a trigger require access to the trace record
- *	but are not always present).
+ * @flags: See the enum event_command_flags below.
  *
  * All the methods below, except for @set_filter() and @unreg_all(),
  * must be implemented.
@@ -1340,8 +1317,7 @@ struct event_command {
 	struct list_head	list;
 	char			*name;
 	enum event_trigger_type	trigger_type;
-	bool			post_trigger;
-	bool			needs_rec;
+	int			flags;
 	int			(*func)(struct event_command *cmd_ops,
 					struct trace_event_file *file,
 					char *glob, char *cmd, char *params);
@@ -1360,6 +1336,49 @@ struct event_command {
 	struct event_trigger_ops *(*get_trigger_ops)(char *cmd, char *param);
 };
 
+/**
+ * enum event_command_flags - flags for struct event_command
+ *
+ * @POST_TRIGGER: A flag that says whether or not this command needs
+ *	to have its action delayed until after the current event has
+ *	been closed.  Some triggers need to avoid being invoked while
+ *	an event is currently in the process of being logged, since
+ *	the trigger may itself log data into the trace buffer.  Thus
+ *	we make sure the current event is committed before invoking
+ *	those triggers.  To do that, the trigger invocation is split
+ *	in two - the first part checks the filter using the current
+ *	trace record; if a command has the @post_trigger flag set, it
+ *	sets a bit for itself in the return value, otherwise it
+ *	directly invokes the trigger.  Once all commands have been
+ *	either invoked or set their return flag, the current record is
+ *	either committed or discarded.  At that point, if any commands
+ *	have deferred their triggers, those commands are finally
+ *	invoked following the close of the current event.  In other
+ *	words, if the event_trigger_ops @func() probe implementation
+ *	itself logs to the trace buffer, this flag should be set,
+ *	otherwise it can be left unspecified.
+ *
+ * @NEEDS_REC: A flag that says whether or not this command needs
+ *	access to the trace record in order to perform its function,
+ *	regardless of whether or not it has a filter associated with
+ *	it (filters make a trigger require access to the trace record
+ *	but are not always present).
+ */
+enum event_command_flags {
+	EVENT_CMD_FL_POST_TRIGGER	= 1,
+	EVENT_CMD_FL_NEEDS_REC		= 2,
+};
+
+static inline bool event_command_post_trigger(struct event_command *cmd_ops)
+{
+	return cmd_ops->flags & EVENT_CMD_FL_POST_TRIGGER;
+}
+
+static inline bool event_command_needs_rec(struct event_command *cmd_ops)
+{
+	return cmd_ops->flags & EVENT_CMD_FL_NEEDS_REC;
+}
+
 extern int trace_event_enable_disable(struct trace_event_file *file,
 				      int enable, int soft_disable);
 extern int tracing_alloc_snapshot(void);
diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
index cbb7ee531983..d67992f3bb0e 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -81,7 +81,7 @@ event_triggers_call(struct trace_event_file *file, void *rec)
 		filter = rcu_dereference_sched(data->filter);
 		if (filter && !filter_match_preds(filter, rec))
 			continue;
-		if (data->cmd_ops->post_trigger) {
+		if (event_command_post_trigger(data->cmd_ops)) {
 			tt |= data->cmd_ops->trigger_type;
 			continue;
 		}
@@ -506,8 +506,8 @@ void update_cond_flag(struct trace_event_file *file)
 	bool set_cond = false;
 
 	list_for_each_entry_rcu(data, &file->triggers, list) {
-		if (data->filter || data->cmd_ops->post_trigger ||
-		    data->cmd_ops->needs_rec) {
+		if (data->filter || event_command_post_trigger(data->cmd_ops) ||
+		    event_command_needs_rec(data->cmd_ops)) {
 			set_cond = true;
 			break;
 		}
@@ -1035,7 +1035,7 @@ stacktrace_get_trigger_ops(char *cmd, char *param)
 static struct event_command trigger_stacktrace_cmd = {
 	.name			= "stacktrace",
 	.trigger_type		= ETT_STACKTRACE,
-	.post_trigger		= true,
+	.flags			= EVENT_CMD_FL_POST_TRIGGER,
 	.func			= event_trigger_callback,
 	.reg			= register_trigger,
 	.unreg			= unregister_trigger,

[toc] | [prev] | [next] | [standalone]

#1338579

On Fri, 2016-02-19 at 17:35 -0500, Steven Rostedt wrote:
> On Fri, 19 Feb 2016 14:30:13 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > @@ -1333,6 +1339,7 @@ struct event_command {
> > >  	char			*name;
> > >  	enum event_trigger_type	trigger_type;
> > >  	bool			post_trigger;
> > > +	bool			needs_rec;  
> > 
> > Note, from what I understand, gcc sucks with bool in structures. Best
> > thing to do here is to create a "int flags" field, and check the result
> > with masks.
> > 
> > You don't need to update this patch (I'm still working on the series),
> > but a patch on top of these may be necessary. I could add the patch too.
> > 
> 
> I've added this patch after patch this patch. What do you think?
> 

Looks great, thanks!

Do you want me to respin adding this and the fixes for the previous
comments before continuing?

Tom

> -- Steve
> 
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 789e88ab5acd..f60c0444ef1e 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1272,30 +1272,7 @@ struct event_trigger_ops {
>   *	values are defined by adding new values to the trigger_type
>   *	enum in include/linux/trace_events.h.
>   *
> - * @post_trigger: A flag that says whether or not this command needs
> - *	to have its action delayed until after the current event has
> - *	been closed.  Some triggers need to avoid being invoked while
> - *	an event is currently in the process of being logged, since
> - *	the trigger may itself log data into the trace buffer.  Thus
> - *	we make sure the current event is committed before invoking
> - *	those triggers.  To do that, the trigger invocation is split
> - *	in two - the first part checks the filter using the current
> - *	trace record; if a command has the @post_trigger flag set, it
> - *	sets a bit for itself in the return value, otherwise it
> - *	directly invokes the trigger.  Once all commands have been
> - *	either invoked or set their return flag, the current record is
> - *	either committed or discarded.  At that point, if any commands
> - *	have deferred their triggers, those commands are finally
> - *	invoked following the close of the current event.  In other
> - *	words, if the event_trigger_ops @func() probe implementation
> - *	itself logs to the trace buffer, this flag should be set,
> - *	otherwise it can be left unspecified.
> - *
> - * @needs_rec: A flag that says whether or not this command needs
> - *	access to the trace record in order to perform its function,
> - *	regardless of whether or not it has a filter associated with
> - *	it (filters make a trigger require access to the trace record
> - *	but are not always present).
> + * @flags: See the enum event_command_flags below.
>   *
>   * All the methods below, except for @set_filter() and @unreg_all(),
>   * must be implemented.
> @@ -1340,8 +1317,7 @@ struct event_command {
>  	struct list_head	list;
>  	char			*name;
>  	enum event_trigger_type	trigger_type;
> -	bool			post_trigger;
> -	bool			needs_rec;
> +	int			flags;
>  	int			(*func)(struct event_command *cmd_ops,
>  					struct trace_event_file *file,
>  					char *glob, char *cmd, char *params);
> @@ -1360,6 +1336,49 @@ struct event_command {
>  	struct event_trigger_ops *(*get_trigger_ops)(char *cmd, char *param);
>  };
>  
> +/**
> + * enum event_command_flags - flags for struct event_command
> + *
> + * @POST_TRIGGER: A flag that says whether or not this command needs
> + *	to have its action delayed until after the current event has
> + *	been closed.  Some triggers need to avoid being invoked while
> + *	an event is currently in the process of being logged, since
> + *	the trigger may itself log data into the trace buffer.  Thus
> + *	we make sure the current event is committed before invoking
> + *	those triggers.  To do that, the trigger invocation is split
> + *	in two - the first part checks the filter using the current
> + *	trace record; if a command has the @post_trigger flag set, it
> + *	sets a bit for itself in the return value, otherwise it
> + *	directly invokes the trigger.  Once all commands have been
> + *	either invoked or set their return flag, the current record is
> + *	either committed or discarded.  At that point, if any commands
> + *	have deferred their triggers, those commands are finally
> + *	invoked following the close of the current event.  In other
> + *	words, if the event_trigger_ops @func() probe implementation
> + *	itself logs to the trace buffer, this flag should be set,
> + *	otherwise it can be left unspecified.
> + *
> + * @NEEDS_REC: A flag that says whether or not this command needs
> + *	access to the trace record in order to perform its function,
> + *	regardless of whether or not it has a filter associated with
> + *	it (filters make a trigger require access to the trace record
> + *	but are not always present).
> + */
> +enum event_command_flags {
> +	EVENT_CMD_FL_POST_TRIGGER	= 1,
> +	EVENT_CMD_FL_NEEDS_REC		= 2,
> +};
> +
> +static inline bool event_command_post_trigger(struct event_command *cmd_ops)
> +{
> +	return cmd_ops->flags & EVENT_CMD_FL_POST_TRIGGER;
> +}
> +
> +static inline bool event_command_needs_rec(struct event_command *cmd_ops)
> +{
> +	return cmd_ops->flags & EVENT_CMD_FL_NEEDS_REC;
> +}
> +
>  extern int trace_event_enable_disable(struct trace_event_file *file,
>  				      int enable, int soft_disable);
>  extern int tracing_alloc_snapshot(void);
> diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> index cbb7ee531983..d67992f3bb0e 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -81,7 +81,7 @@ event_triggers_call(struct trace_event_file *file, void *rec)
>  		filter = rcu_dereference_sched(data->filter);
>  		if (filter && !filter_match_preds(filter, rec))
>  			continue;
> -		if (data->cmd_ops->post_trigger) {
> +		if (event_command_post_trigger(data->cmd_ops)) {
>  			tt |= data->cmd_ops->trigger_type;
>  			continue;
>  		}
> @@ -506,8 +506,8 @@ void update_cond_flag(struct trace_event_file *file)
>  	bool set_cond = false;
>  
>  	list_for_each_entry_rcu(data, &file->triggers, list) {
> -		if (data->filter || data->cmd_ops->post_trigger ||
> -		    data->cmd_ops->needs_rec) {
> +		if (data->filter || event_command_post_trigger(data->cmd_ops) ||
> +		    event_command_needs_rec(data->cmd_ops)) {
>  			set_cond = true;
>  			break;
>  		}
> @@ -1035,7 +1035,7 @@ stacktrace_get_trigger_ops(char *cmd, char *param)
>  static struct event_command trigger_stacktrace_cmd = {
>  	.name			= "stacktrace",
>  	.trigger_type		= ETT_STACKTRACE,
> -	.post_trigger		= true,
> +	.flags			= EVENT_CMD_FL_POST_TRIGGER,
>  	.func			= event_trigger_callback,
>  	.reg			= register_trigger,
>  	.unreg			= unregister_trigger,

[toc] | [prev] | [next] | [standalone]

#1338610

On Fri, 19 Feb 2016 17:31:11 -0600
Tom Zanussi <tom.zanussi@linux.intel.com> wrote:
 
> 
> Do you want me to respin adding this and the fixes for the previous
> comments before continuing?

Yeah, respin, but start with patch 9 of this series. I've already
looked at and even pulled into a local tree the first 8 patches.

-- Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web