LKML Archive on lore.kernel.org
 help / Atom feed
From: Namhyung Kim <namhyung@kernel.org>
To: Tom Zanussi <zanussi@kernel.org>
Cc: rostedt@goodmis.org, tglx@linutronix.de, mhiramat@kernel.org,
	vedang.patel@intel.com, bigeasy@linutronix.de,
	joel@joelfernandes.org, mathieu.desnoyers@efficios.com,
	julia@ni.com, linux-kernel@vger.kernel.org,
	linux-rt-users@vger.kernel.org, kernel-team@lge.com
Subject: Re: [PATCH v3 1/7] tracing: Refactor hist trigger action code
Date: Fri, 10 Aug 2018 15:58:58 +0900
Message-ID: <20180810065858.GB479@sejong> (raw)
In-Reply-To: <9d8aaf30dc0c8d48885b0936a038ddd770a55caa.1533753152.git.tom.zanussi@linux.intel.com>

Hi Tom,

On Thu, Aug 09, 2018 at 09:34:11AM -0500, Tom Zanussi wrote:
> From: Tom Zanussi <tom.zanussi@linux.intel.com>
> 
> The hist trigger action code currently implements two essentially
> hard-coded pairs of 'actions' - onmax(), which tracks a variable and
> saves some event fields when a max is hit, and onmatch(), which is
> hard-coded to generate a synthetic event.
> 
> These hardcoded pairs (track max/save fields and detect match/generate
> synthetic event) should really be decoupled into separate components
> that can then be arbitrarily combined.  The first component of each
> pair (track max/detect match) is called a 'handler' in the new code,
> while the second component (save fields/generate synthetic event) is
> called an 'action' in this scheme.
> 
> This change refactors the action code to reflect this split by adding
> two handlers, HANDLER_ONMATCH and HANDLER_ONMAX, along with two
> actions, ACTION_SAVE and ACTION_TRACE.
> 
> The new code combines them to produce the existing ONMATCH/TRACE and
> ONMAX/SAVE functionality, but doesn't implement the other combinations
> now possible.  Future patches will expand these to further useful
> cases, such as ONMAX/TRACE, as well as add additional handlers and
> actions such as ONCHANGE and SNAPSHOT.
> 
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> ---

[SNIP]
> @@ -3421,10 +3419,69 @@ static int parse_action_params(char *params, struct action_data *data)
>  	return ret;
>  }
>  
> -static struct action_data *onmax_parse(char *str)
> +static int action_parse(char *str, struct action_data *data,
> +			enum handler_id handler)
> +{
> +	char *action_name;
> +	int ret = 0;
> +
> +	strsep(&str, ".");
> +	if (!str) {
> +		hist_err("action parsing: No action found", "");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	action_name = strsep(&str, "(");
> +	if (!action_name || !str) {
> +		hist_err("action parsing: No action found", "");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (strncmp(action_name, "save", strlen("save")) == 0) {
> +		char *params = strsep(&str, ")");
> +
> +		if (!params) {
> +			hist_err("action parsing: No params found for %s", "save");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		ret = parse_action_params(params, data);
> +		if (ret)
> +			goto out;
> +
> +		if (handler == HANDLER_ONMAX)
> +			data->fn = onmax_save;
> +
> +		data->action = ACTION_SAVE;
> +	} else {
> +		char *params = strsep(&str, ")");
> +
> +		if (params) {
> +			ret = parse_action_params(params, data);
> +			if (ret)
> +				goto out;
> +		}
> +
> +		data->fn = action_trace;
> +		data->action = ACTION_TRACE;

This will set action name as (synthetic) event name, right?  I think
it's natural to have "trace" for it with this change.  Maybe it's time
to change the syntax like below?

  onmatch(SYS.EVENT).trace(EVENT, FIELD, ...)

Of course it should support old syntax too..


> +	}
> +
> +	data->action_name = kstrdup(action_name, GFP_KERNEL);
> +	if (!data->action_name) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> + out:
> +	return ret;
> +}

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-09 14:34 [PATCH v3 0/7] tracing: Hist trigger snapshot and onchange additions Tom Zanussi
2018-08-09 14:34 ` [PATCH v3 1/7] tracing: Refactor hist trigger action code Tom Zanussi
2018-08-10  6:58   ` Namhyung Kim [this message]
2018-08-10 14:05     ` Tom Zanussi
2018-08-09 14:34 ` [PATCH v3 2/7] tracing: Split up onmatch action data Tom Zanussi
2018-08-09 14:34 ` [PATCH v3 3/7] tracing: Generalize hist trigger onmax and save action Tom Zanussi
2018-08-09 14:34 ` [PATCH v3 4/7] tracing: Add conditional snapshot Tom Zanussi
2018-08-10  7:02   ` Namhyung Kim
2018-08-10 14:05     ` Tom Zanussi
2018-08-09 14:34 ` [PATCH v3 5/7] tracing: Move hist trigger key printing into a separate function Tom Zanussi
2018-08-09 14:34 ` [PATCH v3 6/7] tracing: Add snapshot action Tom Zanussi
2018-08-10  7:04   ` Namhyung Kim
2018-08-10 14:07     ` Tom Zanussi
2018-08-09 14:34 ` [PATCH v3 7/7] tracing: Add hist trigger onchange() handler Tom Zanussi
2018-08-09 14:47 ` [PATCH v3 0/7] tracing: Hist trigger snapshot and onchange additions Steven Rostedt
2018-08-09 14:51   ` Tom Zanussi
2018-08-09 14:55     ` Steven Rostedt
2018-08-09 14:56       ` Tom Zanussi

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180810065858.GB479@sejong \
    --to=namhyung@kernel.org \
    --cc=bigeasy@linutronix.de \
    --cc=joel@joelfernandes.org \
    --cc=julia@ni.com \
    --cc=kernel-team@lge.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mhiramat@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=vedang.patel@intel.com \
    --cc=zanussi@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox