Re: Re: [PATCH v9 12/22] tracing: Add hist trigger support for pausing and continuing a trace

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

On 2015/07/23 5:22, Tom Zanussi wrote:
> Hi Masami,
> 
> On Wed, 2015-07-22 at 17:20 +0900, Masami Hiramatsu wrote:
>> Hi Tom,
>>
>> On 2015/07/17 2:22, Tom Zanussi wrote:
>>> Allow users to append 'pause' or 'continue' to an existing trigger in
>>> order to have it paused or to have a paused trace continue.
>>>
>>> This expands the hist trigger syntax from this:
>>>     # echo hist:keys=xxx:vals=yyy:sort=zzz.descending \
>>>           [ if filter] > event/trigger
>>>
>>> to this:
>>>
>>>     # echo hist:keys=xxx:vals=yyy:sort=zzz.descending:pause or cont \
>>>           [ if filter] > event/trigger
>>
>> Since the only one hist trigger can be set on one event, it seems
>> that we don't need keys for pause/cont/clear (e.g. hist:pause is enough).
>> Anyway, I've found an odd behavior.
> 
> Right, because currently there is only one hist trigger per event, the
> key is ignored, which also accounts for the 'odd' behavior.
> 
> But rather than saying it's expected and document it, I think the
> conclusion from the other comments are that we'll be allowing multiple
> hist triggers per event, and in that case, they need to be uniquely
> identifiable by both key and filter.

Agreed.

BTW, as I've wrote in another reply, pause/cont/clear commands should be
for the "hist" file, not for trigger. So, below command will be better.

# echo pause:name=foo > event/hist
# echo clear:keys=xxx > event/hist

Thank you,



>> [root@localhost tracing]# echo 'hist:keys=parent_pid' > events/sched/sched_process_fork/trigger
>> [root@localhost tracing]# echo 'hist:keys=common_pid:pause' > events/sched/sched_process_fork/trigger
>> [root@localhost tracing]# cat events/sched/sched_process_fork/trigger
>> hist:keys=parent_pid:vals=hitcount:sort=hitcount:size=2048 [paused]
>>
>> So, the second "pause" command can work with different keys.
>> Moreover, I can remove it with different keys.
>>
> 
> Right, this goes away once we have code that deals with multiple
> histograms per event, which I'll go ahead and implement rather than
> document the confusion...
> 
> Tom
> 
>> [root@localhost tracing]# echo '!hist:keys=child_pid' > events/sched/sched_process_fork/trigger
>> [root@localhost tracing]# cat events/sched/sched_process_fork/trigger
>> # Available triggers:
>> # traceon traceoff snapshot stacktrace enable_event disable_event enable_hist disable_hist hist
>>
>> Thank you,
>>
>>>
>>> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
>>> ---
>>>  kernel/trace/trace.c             |  5 +++++
>>>  kernel/trace/trace_events_hist.c | 26 +++++++++++++++++++++++---
>>>  2 files changed, 28 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
>>> index 5dd1fc4..547bbc8 100644
>>> --- a/kernel/trace/trace.c
>>> +++ b/kernel/trace/trace.c
>>> @@ -3791,6 +3791,7 @@ static const char readme_msg[] =
>>>  	"\t            [:values=<field1[,field2,...]]\n"
>>>  	"\t            [:sort=field1,field2,...]\n"
>>>  	"\t            [:size=#entries]\n"
>>> +	"\t            [:pause][:continue]\n"
>>>  	"\t            [if <filter>]\n\n"
>>>  	"\t    When a matching event is hit, an entry is added to a hash\n"
>>>  	"\t    table using the key(s) and value(s) named.  Keys and values\n"
>>> @@ -3821,6 +3822,10 @@ static const char readme_msg[] =
>>>  	"\t    on.  The default if unspecified is 'hitcount' and the.\n"
>>>  	"\t    default sort order is 'ascending'.  To sort in the opposite\n"
>>>  	"\t    direction, append .descending' to the sort key.\n\n"
>>> +	"\t    The 'pause' param can be used to pause an existing hist\n"
>>> +	"\t    trigger or to start a hist trigger but not log any events\n"
>>> +	"\t    until told to do so.  'continue' can be used to start or\n"
>>> +	"\t    restart a paused hist trigger.\n\n"
>>>  #endif
>>>  ;
>>>  
>>> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
>>> index 6bf224f..3ae58e7 100644
>>> --- a/kernel/trace/trace_events_hist.c
>>> +++ b/kernel/trace/trace_events_hist.c
>>> @@ -78,6 +78,8 @@ struct hist_trigger_attrs {
>>>  	char		*keys_str;
>>>  	char		*vals_str;
>>>  	char		*sort_key_str;
>>> +	bool		pause;
>>> +	bool		cont;
>>>  	unsigned int	map_bits;
>>>  };
>>>  
>>> @@ -184,6 +186,11 @@ static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str)
>>>  			attrs->vals_str = kstrdup(str, GFP_KERNEL);
>>>  		else if (!strncmp(str, "sort", strlen("sort")))
>>>  			attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
>>> +		else if (!strncmp(str, "pause", strlen("pause")))
>>> +			attrs->pause = true;
>>> +		else if (!strncmp(str, "continue", strlen("continue")) ||
>>> +			 !strncmp(str, "cont", strlen("cont")))
>>> +			attrs->cont = true;
>>>  		else if (!strncmp(str, "size", strlen("size"))) {
>>>  			int map_bits = parse_map_size(str);
>>>  
>>> @@ -843,7 +850,10 @@ static int event_hist_trigger_print(struct seq_file *m,
>>>  	if (data->filter_str)
>>>  		seq_printf(m, " if %s", data->filter_str);
>>>  
>>> -	seq_puts(m, " [active]");
>>> +	if (data->paused)
>>> +		seq_puts(m, " [paused]");
>>> +	else
>>> +		seq_puts(m, " [active]");
>>>  
>>>  	seq_putc(m, '\n');
>>>  
>>> @@ -882,16 +892,25 @@ static int hist_register_trigger(char *glob, struct event_trigger_ops *ops,
>>>  				 struct event_trigger_data *data,
>>>  				 struct trace_event_file *file)
>>>  {
>>> +	struct hist_trigger_data *hist_data = data->private_data;
>>>  	struct event_trigger_data *test;
>>>  	int ret = 0;
>>>  
>>>  	list_for_each_entry_rcu(test, &file->triggers, list) {
>>>  		if (test->cmd_ops->trigger_type == ETT_EVENT_HIST) {
>>> -			ret = -EEXIST;
>>> +			if (hist_data->attrs->pause)
>>> +				test->paused = true;
>>> +			else if (hist_data->attrs->cont)
>>> +				test->paused = false;
>>> +			else
>>> +				ret = -EEXIST;
>>>  			goto out;
>>>  		}
>>>  	}
>>>  
>>> +	if (hist_data->attrs->pause)
>>> +		data->paused = true;
>>> +
>>>  	if (data->ops->init) {
>>>  		ret = data->ops->init(data->ops, data);
>>>  		if (ret < 0)
>>> @@ -984,7 +1003,8 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
>>>  	 * triggers registered a failure too.
>>>  	 */
>>>  	if (!ret) {
>>> -		ret = -ENOENT;
>>> +		if (!(attrs->pause || attrs->cont))
>>> +			ret = -ENOENT;
>>>  		goto out_free;
>>>  	} else if (ret < 0)
>>>  		goto out_free;
>>>
>>
>>
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


-- 
Masami HIRAMATSU
Linux Technology Research Center, System Productivity Research Dept.
Center for Technology Innovation - Systems Engineering
Hitachi, Ltd., Research & Development Group
E-mail: masami.hiramatsu.pt@hitachi.com