RE: [PATCH v4] app/testpmd: enable cli for programmable action

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Wednesday, October 11, 2023 9:20 PM
> To: Ferruh Yigit <ferruh.yigit@amd.com>; Singh, Aman Deep
> <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> orika@nvidia.com
> Subject: RE: [PATCH v4] app/testpmd: enable cli for programmable action
> 
> 
> 
> > -----Original Message-----
> > From: Ferruh Yigit <ferruh.yigit@amd.com>
> > Sent: Wednesday, October 11, 2023 6:21 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Singh, Aman Deep
> > <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> > Cc: dev@dpdk.org; Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com>; orika@nvidia.com
> > Subject: Re: [PATCH v4] app/testpmd: enable cli for programmable
> > action
> >
> > On 10/11/2023 3:24 AM, Zhang, Qi Z wrote:
> > >
> > >> -----Original Message-----
> > >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> > >> Sent: Tuesday, October 10, 2023 6:49 PM
> > >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Singh, Aman Deep
> > >> <aman.deep.singh@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>
> > >> Cc: dev@dpdk.org; Dumitrescu, Cristian
> > >> <cristian.dumitrescu@intel.com>; orika@nvidia.com
> > >> Subject: Re: [PATCH v4] app/testpmd: enable cli for programmable
> > >> action
> > >>
> > >> On 10/7/2023 11:47 AM, Qi Zhang wrote:
> > >>> Parsing command line for rte_flow_action_prog.
> > >>>
> > >>> Syntax:
> > >>>
> > >>> "prog name <name> [arguments <arg_name_0> <arg_value_0> \
> > >>> <arg_name_1> <arg_value1> ... end]"
> > >>>
> > >>
> > >> Can you please put full rte flow command in the commit log? Like
> > >> what is the 'pattern' for above command?
> > >
> > > The pattern part should be independent of the action part,
> > >
> > > though for our P4 device, we will prefer use rte_flow_flex_item,
> > > something
> > like:
> > >
> > > flow create 0 pattern flex item is xxx pattern is xxx / flex item is
> > > xxx pattern
> > is / actions prog name ......
> > >
> > > but it does not limit PMD to support flow like below
> > >
> >
> > I think agreement was to use flex pattern, and my understand is
> > "struct rte_flow_item_flex" will be used to present the table_id.
> >
> > Without not using flex, how driver will detect which table to update?
> >
> >
> > > flow create 0 pattern eth / ipv4 src is 1.1.1.1 / actions prog name ......
> > >
> > > So I think it may not be necessary to highlight the pattern format here.
> > >
> >
> > Complete samples helps a lot to user, can you please include the full
> > rte flow command, you can have flex pattern sample and if you want add
> > more samples with other patterns but we need to clarify it first.
> 
> Agree, I have added full sample on v6 in testpmd document as below:
> 
> ...
>     A rule use Programmable Action to perform a customized tunnel header
> encap for specific IP packets
> 
>     testpmd> flow create 0 ingress pattern eth / ipv4 src is 1.2.3.4 / end actions
> prog name cust_tun_encap arguments tunn_id 55AA meta0 2E meta1 9000
> end / end"
> ...
> 
> The reason I did not include a sample with a flex item is that, in the context of
> our P4 device, there is no requirement to utilize the
> 'rte_flow_item_flex_handle' within 'rte_flow_item_flex.'
> However, the current testpmd does not offer support for this configuration.
> 
> Therefore, it may be necessary to introduce a new syntax, such as ... pattern
> flex pattern is xxxx / flex pattern is xxx / end ..., which would also map to
> 'rte_flow_item_flex'.
> 
> >
> >
> > >>
> > >>
> > >>> Use parse_string0 to parse name string.
> > >>> Use parse_hex to parse hex string.
> > >>> Use struct action_prog_data to store parsed result.
> > >>>
> > >>> Example:
> > >>>
> > >>> Action with 2 arguments:
> > >>>
> > >>> "prog name action0 arguments field0 03FF field1 55AA end"
> > >>>
> > >>> Action without argument:
> > >>>
> > >>> "prog name action1"
> > >>>
> > >>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > >>>
> > >>
> > >> Is there an existing driver implementation, checking it helps to
> > >> understand feature implementation?
> > >
> > > This work is still ongoing, currently we target to upstream on DPDK
> > > 24.03
> > >
> >
> > If you won't have driver yet, do you have a way to test these commands?
> > Or is this implementation just theoretical at this stage?

I may not get your point in my previous reply, to verify the correctness of the new commands before the driver is ready, 
Actually, I added some dump code in port_flow_create to verify each prog action and its arguments' size , value

> 
> Yes, internally, we are very close to have an implementation that will leverage
> this new API, The JSON file loaded by CPFL PMD contains the mapping rule
> that direct PMD how to handling  an action_prog,  currently we didn't see any
> gap of the new API.
> 
> >
> >
> > >>
> > >>
> > >>> ---
> > >>>
> > >>> v4:
> > >>> - be more generous on the max size of name and value.
> > >>>
> > >>> v3:
> > >>> - refine struct action_prog_data
> > >>> - enlarge the max size
> > >>>
> > >>> v2:
> > >>> - fix title
> > >>> - minor coding style refine.
> > >>>
> > >>>  app/test-pmd/cmdline_flow.c | 232
> > >>> ++++++++++++++++++++++++++++++++++++
> > >>>  1 file changed, 232 insertions(+)
> > >>>
> > >>
> > >> Hi Qi,
> > >>
> > >> Can you please update documentation too,
> > >> `doc/guides/testpmd_app_ug/testpmd_funcs.rst`, `Flow rules
> > >> management` section.
> > >
> > > Sure.
> > >
> > >>
> > >>
> > >>> diff --git a/app/test-pmd/cmdline_flow.c
> > >>> b/app/test-pmd/cmdline_flow.c index 21828c144c..ae5556e704 100644
> > >>> --- a/app/test-pmd/cmdline_flow.c
> > >>> +++ b/app/test-pmd/cmdline_flow.c
> > >>> @@ -719,6 +719,13 @@ enum index {
> > >>>  	ACTION_IPV6_EXT_PUSH,
> > >>>  	ACTION_IPV6_EXT_PUSH_INDEX,
> > >>>  	ACTION_IPV6_EXT_PUSH_INDEX_VALUE,
> > >>> +	ACTION_PROG,
> > >>> +	ACTION_PROG_NAME,
> > >>> +	ACTION_PROG_NAME_STRING,
> > >>> +	ACTION_PROG_ARGUMENTS,
> > >>> +	ACTION_PROG_ARG_NAME,
> > >>> +	ACTION_PROG_ARG_VALUE,
> > >>> +	ACTION_PROG_ARG_END,
> > >>>  };
> > >>>
> > >>>  /** Maximum size for pattern in struct rte_flow_item_raw. */ @@
> > >>> -749,6 +756,23 @@ struct action_rss_data {
> > >>>  	uint16_t queue[ACTION_RSS_QUEUE_NUM];  };
> > >>>
> > >>> +#define ACTION_PROG_NAME_SIZE_MAX 256 #define
> > >> ACTION_PROG_ARG_NUM_MAX
> > >>> +16 #define ACTION_PROG_ARG_VALUE_SIZE_MAX 64
> > >>> +
> > >>> +/** Storage for struct rte_flow_action_prog including external data.
> > >>> +*/ struct action_prog_data {
> > >>> +	struct rte_flow_action_prog conf;
> > >>> +	struct {
> > >>> +		char name[ACTION_PROG_NAME_SIZE_MAX];
> > >>> +		struct rte_flow_action_prog_argument
> > >> args[ACTION_PROG_ARG_NUM_MAX];
> > >>> +		struct {
> > >>> +			char names[ACTION_PROG_NAME_SIZE_MAX];
> > >>> +			uint8_t
> > >> value[ACTION_PROG_ARG_VALUE_SIZE_MAX];
> > >>> +		} arg_data[ACTION_PROG_ARG_NUM_MAX];
> > >>> +	} data;
> > >>> +};
> > >>> +
> > >>>  /** Maximum data size in struct rte_flow_action_raw_encap. */
> > >>> #define ACTION_RAW_ENCAP_MAX_DATA 512  #define
> > >> RAW_ENCAP_CONFS_MAX_NUM
> > >>> 8 @@ -2169,6 +2193,7 @@ static const enum index next_action[] = {
> > >>>  	ACTION_QUOTA_QU,
> > >>>  	ACTION_IPV6_EXT_REMOVE,
> > >>>  	ACTION_IPV6_EXT_PUSH,
> > >>> +	ACTION_PROG,
> > >>>  	ZERO,
> > >>>  };
> > >>>
> > >>> @@ -2510,6 +2535,13 @@ static const enum index
> > >> action_represented_port[] = {
> > >>>  	ZERO,
> > >>>  };
> > >>>
> > >>> +static const enum index action_prog[] = {
> > >>> +	ACTION_PROG_NAME,
> > >>> +	ACTION_PROG_ARGUMENTS,
> > >>> +	ACTION_NEXT,
> > >>> +	ZERO,
> > >>> +};
> > >>> +
> > >>>  static int parse_set_raw_encap_decap(struct context *, const
> > >>> struct token
> > *,
> > >>>  				     const char *, unsigned int,
> > >>>  				     void *, unsigned int);
> > >>> @@ -2786,6 +2818,18 @@ static int
> > >>>  parse_qu_mode_name(struct context *ctx, const struct token *token,
> > >>>  		   const char *str, unsigned int len, void *buf,
> > >>>  		   unsigned int size);
> > >>> +static int
> > >>> +parse_vc_action_prog(struct context *, const struct token *,
> > >>> +		     const char *, unsigned int, void *,
> > >>> +		     unsigned int);
> > >>> +static int
> > >>> +parse_vc_action_prog_arg_name(struct context *, const struct token *,
> > >>> +			      const char *, unsigned int, void *,
> > >>> +			      unsigned int);
> > >>> +static int
> > >>> +parse_vc_action_prog_arg_value(struct context *, const struct token *,
> > >>> +			       const char *, unsigned int, void *,
> > >>> +			       unsigned int);
> > >>>  static int comp_none(struct context *, const struct token *,
> > >>>  		     unsigned int, char *, unsigned int);  static int
> > >>> comp_boolean(struct context *, const struct token *, @@ -7518,6
> > >>> +7562,48 @@ static const struct token token_list[] = {
> > >>>  		.args = ARGS(ARGS_ENTRY(struct rte_flow_item_tx_queue,
> > >>>  					tx_queue)),
> > >>>  	},
> > >>> +	[ACTION_PROG] = {
> > >>> +		.name = "prog",
> > >>> +		.help = "match a programmable action",
> > >>> +		.priv = PRIV_ACTION(PROG, sizeof(struct action_prog_data)),
> > >>> +		.next = NEXT(action_prog),
> > >>> +		.call = parse_vc_action_prog,
> > >>> +	},
> > >>> +	[ACTION_PROG_NAME] = {
> > >>> +		.name = "name",
> > >>> +		.help = "programble action name",
> > >>>
> > >>
> > >> Can you please remind me again what was the 'name' filed of "struct
> > >> rte_flow_action_prog" was for?
> > >
> > > The 'name' field serves as a means for the driver to identify an
> > > action
> > schema, enabling it to verify if the number of parameters and the size
> > of each parameter value align with the P4 definition.
> > > Subsequently, the driver translates these values into
> > > hardware-specific
> > configurations. If there is a misalignment, the PMD will return a failure.
> > >
> >
> > As I understand it is used for kind of unique action handler, but we
> > have rte_flow handler already, I am still not clear why an action
> > handler is required.
> >
> > Why driver is not using rte flow handler?
> 
> Not sure if I have understood your question correctly.
> But, the "name" field you're referring to is not related with an action handler,
> it's more like an action type.
> Just like 'RTE_FLOW_ACTION_TYPE_XXX,' which is predefined, but this action
> type is defined as device-specific.
> 
> >
> > Again driver implementation would clear more the intended usage.
> 
> At this time, I hope above sample testpmd command can be helpful in this
> matter.
> 
> >
> > >>
> > >>
> > >>> +		.next = NEXT(action_prog,
> > >> NEXT_ENTRY(ACTION_PROG_NAME_STRING)),
> > >>> +		.args = ARGS(ARGS_ENTRY(struct action_prog_data,
> > >> data.name)),
> > >>> +	},
> > >>> +	[ACTION_PROG_NAME_STRING] = {
> > >>> +		.name = "{string}",
> > >>> +		.type = "STRING",
> > >>> +		.help = "programmable action name string",
> > >>> +		.call = parse_string0,
> > >>> +	},
> > >>> +	[ACTION_PROG_ARGUMENTS] = {
> > >>> +		.name = "arguments",
> > >>> +		.help = "programmable action name",
> > >>> +		.next = NEXT(action_prog,
> > >> NEXT_ENTRY(ACTION_PROG_ARG_NAME)),
> > >>> +		.call = parse_vc_conf,
> > >>> +	},
> > >>> +	[ACTION_PROG_ARG_NAME] = {
> > >>> +		.name = "{string}",
> > >>> +		.help = "programmable action argument name",
> > >>> +		.next = NEXT(NEXT_ENTRY(ACTION_PROG_ARG_VALUE)),
> > >>> +		.call = parse_vc_action_prog_arg_name,
> > >>> +	},
> > >>> +	[ACTION_PROG_ARG_VALUE] = {
> > >>> +		.name = "{hex}",
> > >>> +		.help = "programmable action argument value",
> > >>> +		.next = NEXT(NEXT_ENTRY(ACTION_PROG_ARG_END,
> > >> ACTION_PROG_ARG_NAME)),
> > >>> +		.call = parse_vc_action_prog_arg_value,
> > >>> +	},
> > >>> +	[ACTION_PROG_ARG_END] = {
> > >>> +		.name = "end",
> > >>> +		.help = "end of the programmable action arguments",
> > >>> +	},
> > >>> +
> > >>>
> > >>
> > >> Does this means two 'end' required if multiple args provided, like:
> > >> prog name "name" arguments field0 03FF field1 1 end / end I am
> > >> aware there is variable length of key/value, and need a marker to
> > >> stop, but this end specific for action is not used,
> > >
> > >  Actually I borrowed the idea from the queue group in the RSS action:
> > >
> > > 'actions rss queues 0 1 2 3 end / end ..."
> > >
> > > The difference is that the 'end' check was previously hidden within
> > > the
> > 'parse_vc_action_rss_queue' function, while in my implementation, I've
> > defined it as a distinct state.
> > >
> >
> >
> > My concern is if ACTION_xxx_END tokens be confusing or redundant if we
> > have more of them.
> >
> > Is there a benefit to have it as token, comparing to have it in the
> > parser functions as RSS does?
> 
> In my opinion, adding more 'ACTION_xxx_END' won't negatively impact code
> readability.
> It enhances the transparency of the syntax and may even facilitate potential
> automation.