Re: [dpdk-dev] [PATCH v4 3/6] testpmd: add jump action

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

On Wed, Apr 18, 2018 at 10:04:20PM +0100, Declan Doherty wrote:
> Add support for specificaiton of new JUMP action to testpmd's flow

Typo on "specificaiton".

> cli, and update the testpmd documentation to describe this new
> action.
> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>

Beside minor nits below, this patch is OK. I suggest to merge it in the
previous one ("Add jump action type to rte_flow").

> ---
>  app/test-pmd/cmdline_flow.c                 | 23 +++++++++++++++++++++++
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  4 ++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index 59f3b3b57..93e9a240d 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -157,6 +157,8 @@ enum index {
>  	ACTION_END,
>  	ACTION_VOID,
>  	ACTION_PASSTHRU,
> +	ACTION_JUMP,
> +	ACTION_JUMP_GROUP,
>  	ACTION_MARK,
>  	ACTION_MARK_ID,
>  	ACTION_FLAG,
> @@ -590,6 +592,7 @@ static const enum index next_action[] = {
>  	ACTION_END,
>  	ACTION_VOID,
>  	ACTION_PASSTHRU,
> +	ACTION_JUMP,
>  	ACTION_MARK,
>  	ACTION_FLAG,
>  	ACTION_QUEUE,
> @@ -640,6 +643,12 @@ static const enum index action_meter[] = {
>  	ZERO,
>  };
>  
> +static const enum index action_jump[] = {
> +	ACTION_JUMP_GROUP,
> +	ACTION_NEXT,
> +	ZERO,
> +};
> +
>  static int parse_init(struct context *, const struct token *,
>  		      const char *, unsigned int,
>  		      void *, unsigned int);
> @@ -1506,6 +1515,20 @@ static const struct token token_list[] = {
>  		.next = NEXT(NEXT_ENTRY(ACTION_NEXT)),
>  		.call = parse_vc,
>  	},
> +	[ACTION_JUMP] = {
> +		.name = "jump",
> +		.help = "redirect packets to a given group",

How about: packets => traffic

> +		.priv = PRIV_ACTION(JUMP, sizeof(struct rte_flow_action_jump)),
> +		.next = NEXT(action_jump),
> +		.call = parse_vc,
> +	},
> +	[ACTION_JUMP_GROUP] = {
> +		.name = "group",
> +		.help = "group to redirect packets to",
> +		.next = NEXT(action_jump, NEXT_ENTRY(UNSIGNED)),
> +		.args = ARGS(ARGS_ENTRY(struct rte_flow_action_jump, group)),
> +		.call = parse_vc_conf,
> +	},
>  	[ACTION_MARK] = {
>  		.name = "mark",
>  		.help = "attach 32 bit value to packets",
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index a766ac795..7ecd602da 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -3386,6 +3386,10 @@ This section lists supported actions and their attributes, if any.
>  
>  - ``passthru``: let subsequent rule process matched packets.
>  
> +- ``jump``: redirect packet to group on device

Missing "."

> +
> +  - ``group {unsigned}``: group to redirect to

Ditto.

> +
>  - ``mark``: attach 32 bit value to packets.
>  
>    - ``id {unsigned}``: 32 bit value to return with packets.
> -- 
> 2.14.3
> 

-- 
Adrien Mazarguil
6WIND