Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753462AbbGWOGo (ORCPT ); Thu, 23 Jul 2015 10:06:44 -0400 Received: from mail4.hitachi.co.jp ([133.145.228.5]:52144 "EHLO mail4.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753113AbbGWOGc (ORCPT ); Thu, 23 Jul 2015 10:06:32 -0400 Message-ID: <55B0F4DF.9050801@hitachi.com> Date: Thu, 23 Jul 2015 23:06:23 +0900 From: Masami Hiramatsu Organization: Hitachi, Ltd., Japan User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Tom Zanussi CC: rostedt@goodmis.org, daniel.wagner@bmw-carit.de, namhyung@kernel.org, josh@joshtriplett.org, andi@firstfloor.org, linux-kernel@vger.kernel.org Subject: Re: Re: [PATCH v9 12/22] tracing: Add hist trigger support for pausing and continuing a trace References: <55AF523C.10601@hitachi.com> <1437596557.22524.88.camel@tzanussi-mobl.amr.corp.intel.com> In-Reply-To: <1437596557.22524.88.camel@tzanussi-mobl.amr.corp.intel.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7065 Lines: 193 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 >>> --- >>> 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=>> "\t [:sort=field1,field2,...]\n" >>> "\t [:size=#entries]\n" >>> + "\t [:pause][:continue]\n" >>> "\t [if ]\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 -- 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/